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

[John Snow]

On 10/06/2016 03:44 AM, Kevin Wolf wrote:
> Am 05.10.2016 um 20:49 hat John Snow geschrieben:
> > On 10/05/2016 09:43 AM, Kevin Wolf wrote:
> > > Am 01.10.2016 um 00:00 hat John Snow geschrieben:
> > > > @@ -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.
>
> Yes, I agree. The use of block jobs in replication is rather broken and
> we should change it one way or another. But I'd prefer to do so
> explicitly instead of doing it as a side-effect of a patch like this
> one.

I can always split this patch out and CC Wen, Eric, Markus et al and 
adjust the commit message to be explicit.

> > 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.
>
> Actually, there is at least one other reason why the block jobs in
> replication are a bad a idea as they are today: Job naming. Currently
> they use a fixed string, conflicting with the user-controlled job
> namespace and with itself (i.e. restricting replication to a single
> disk).
>
> And are we really prepared to handle cases where the user decides to
> pause, complete or cancel an internal job?
>
> I think we should really hide them from the user. And maybe the way to
> do so isn't a bool job->user flag, but actually job->id = NULL. Then it
> would work the same way as named/internal BlockBackends do and we would
> get rid of the naming problem, too.
>
> Kevin

Mirrors "internal bitmaps," too.

I can rig it such that if a job has no ID, it will cease to show up via 
query and no longer emit events.

Downside: Whether or not a device is busy or can accept another job 
becomes opaque to the management layer.

--js