Re: [Qemu-block] [Qemu-devel] [PATCH v2 02/11] blockjob: centralize QMP event emissions

[John Snow]

On 10/05/2016 09:43 AM, Kevin Wolf wrote:
> Am 01.10.2016 um 00:00 hat John Snow geschrieben:
> > There's no reason to leave this to blockdev; we can do it in blockjobs
> > directly and get rid of an extra callback for most users.
> >
> > Signed-off-by: John Snow <address@hidden>
> > ---
> >  blockdev.c | 37 ++++++-------------------------------
> >  blockjob.c | 16 ++++++++++++++--
> >  2 files changed, 20 insertions(+), 33 deletions(-)
> >
> > diff --git a/blockdev.c b/blockdev.c
> > index 29c6561..03200e7 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -2957,31 +2957,6 @@ out:
> >      aio_context_release(aio_context);
> >  }
> >
> > -static void block_job_cb(void *opaque, int ret)
> > -{
> > -    /* Note that this function may be executed from another AioContext besides
> > -     * the QEMU main loop.  If you need to access anything that assumes the
> > -     * QEMU global mutex, use a BH or introduce a mutex.
> > -     */
> > -
> > -    BlockDriverState *bs = opaque;
> > -    const char *msg = NULL;
> > -
> > -    trace_block_job_cb(bs, bs->job, ret);
>
> This trace event is removed from the code, but not from trace-events.
>
> > -
> > -    assert(bs->job);
> > -
> > -    if (ret < 0) {
> > -        msg = strerror(-ret);
> > -    }
> > -
> > -    if (block_job_is_cancelled(bs->job)) {
> > -        block_job_event_cancelled(bs->job);
> > -    } else {
> > -        block_job_event_completed(bs->job, msg);
>
> block_job_event_cancelled/completed can become static now.
>
> > -    }
> > -}
> > -
> >  void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
> >                        bool has_base, const char *base,
> >                        bool has_backing_file, const char *backing_file,
> > @@ -3033,7 +3008,7 @@ void qmp_block_stream(bool has_job_id, const char 
> > *job_id, const char *device,
> >      base_name = has_backing_file ? backing_file : base_name;
> >
> >      stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
> > -                 has_speed ? speed : 0, on_error, block_job_cb, bs, 
> > &local_err);
> > +                 has_speed ? speed : 0, on_error, NULL, bs, &local_err);
>
> Passing cb == NULL, but opaque != NULL is harmless, but feels odd.

Yes.

> And actually this is the only caller of stream_start, so the parameters
> could just be dropped.

OK. I left the parameters in on purpose, but they can be re-added in the 
future if desired.

(Hm, maybe as part of a CommonJobOpts parameter someday?)

> >      if (local_err) {
> >          error_propagate(errp, local_err);
> >          goto out;
> > @@ -3136,10 +3111,10 @@ void qmp_block_commit(bool has_job_id, const char 
> > *job_id, const char *device,
> >              goto out;
> >          }
> >          commit_active_start(has_job_id ? job_id : NULL, bs, base_bs, speed,
> > -                            on_error, block_job_cb, bs, &local_err, false);
> > +                            on_error, NULL, bs, &local_err, false);
>
> Here we have an additional caller in block/replication.c and qemu-img,
> so the parameters must stay. For qemu-img, nothing changes. For
> replication, the block job events are added as a side effect.
>
> Not sure if we want to emit such events for an internal block job, but
> if we do want the change, it should be explicit.

Hmm, do we want to make it so some jobs are invisible and others are 
not? Because as it stands right now, neither case is strictly true. We 
only emit cancelled/completed events if it was started via QMP, however 
we do emit events for error and ready regardless of who started the job.

That didn't seem particularly consistent to me; either all events should 
be controlled by the job layer itself or none of them should be.

I opted for "all."

For "internal" jobs that did not previously emit any events, is it not 
true that these jobs still appear in the block job list and are 
effectively public regardless? I'd argue that these messages may be of 
value for management utilities who are still blocked by these jobs 
whether or not they are 'internal' or not.

I'll push for keeping it mandatory and explicit. If it becomes a 
problem, we can always add a 'silent' job property that silences ALL qmp 
events, including all completion, error, and ready notices.

I've CC'd Wen Congyang and Eric Blake to talk me down if they wish.

> >      } else {
> >          commit_start(has_job_id ? job_id : NULL, bs, base_bs, top_bs, speed,
> > -                     on_error, block_job_cb, bs,
> > +                     on_error, NULL, bs,
> >                       has_backing_file ? backing_file : NULL, &local_err);
>
> Like stream_start, drop the parameters.
>
> >      }
> >      if (local_err != NULL) {
> > @@ -3260,7 +3235,7 @@ static void do_drive_backup(DriveBackup *backup, 
> > BlockJobTxn *txn, Error **errp)
> >
> >      backup_start(backup->job_id, bs, target_bs, backup->speed, backup->sync,
> >                   bmap, backup->compress, backup->on_source_error,
> > -                 backup->on_target_error, block_job_cb, bs, txn, &local_err);
> > +                 backup->on_target_error, NULL, bs, txn, &local_err);
> >      bdrv_unref(target_bs);
> >      if (local_err != NULL) {
> >          error_propagate(errp, local_err);
> > @@ -3330,7 +3305,7 @@ void do_blockdev_backup(BlockdevBackup *backup, 
> > BlockJobTxn *txn, Error **errp)
> >      }
> >      backup_start(backup->job_id, bs, target_bs, backup->speed, backup->sync,
> >                   NULL, backup->compress, backup->on_source_error,
> > -                 backup->on_target_error, block_job_cb, bs, txn, &local_err);
> > +                 backup->on_target_error, NULL, bs, txn, &local_err);
>
> Backup is another job used by replication, too. Same question as above.
>
> >      if (local_err != NULL) {
> >          error_propagate(errp, local_err);
> >      }
> > @@ -3410,7 +3385,7 @@ static void blockdev_mirror_common(const char *job_id, 
> > BlockDriverState *bs,
> >                   has_replaces ? replaces : NULL,
> >                   speed, granularity, buf_size, sync, backing_mode,
> >                   on_source_error, on_target_error, unmap,
> > -                 block_job_cb, bs, errp);
> > +                 NULL, bs, errp);
>
> And again, the parameters can be dropped.
>
> Kevin