Re: [Qemu-block] [PATCH 22/42] job: Move BlockJobCreateFlags to Job

[Kevin Wolf]

Am 16.05.2018 um 21:13 hat Eric Blake geschrieben:
> On 05/09/2018 11:26 AM, Kevin Wolf wrote:
> > This renames the BlockJobCreateFlags constants, moves a few JOB_INTERNAL
> > checks to job_create() and the auto_{finalize,dismiss} fields from
> > BlockJob to Job.
> 
> Do we still want to allow auto-finalize on all jobs, or should we keep it
> just for legacy support on block jobs?  Even more so for auto-dismiss (if
> you use the legacy interface, that's what you expect to happen; but other
> than for legacy block jobs, any sane management app is going to request
> auto-dismiss be false, so why expose it to generic jobs?)
> 
> Of course, it may still be easiest to plumb up auto- actions in the internal
> code (where it has to work to keep legacy block jobs from breaking, but no
> new callers have to enable it), so my argument may not apply until you
> actually expose a QMP interface for generic jobs.

This series doesn't expose it anyway. We can later decide whether we
want to add it or not. A sophisticated management tool like libvirt that
needs to manage individual nodes and cope with daemon restarts will
never make use of them, but they might still be useful in hand-crafted
scripts for defined special cases.

> > +++ b/block/mirror.c
> > @@ -1282,7 +1282,7 @@ void mirror_start(const char *job_id, 
> > BlockDriverState *bs,
> >       }
> >       is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
> >       base = mode == MIRROR_SYNC_MODE_TOP ? backing_bs(bs) : NULL;
> > -    mirror_start_job(job_id, bs, BLOCK_JOB_DEFAULT, target, replaces,
> > +    mirror_start_job(job_id, bs, JOB_DEFAULT, target, replaces,
> >                        speed, granularity, buf_size, backing_mode,
> >                        on_source_error, on_target_error, unmap, NULL, NULL,
> >                        &mirror_job_driver, is_none_mode, base, false,
> 
> Just an observation that this is a lot of parameters; would using boxed QAPI
> types make these calls any more obvious?  But that's a separate cleanup.

I'm not sure if we have a QAPI type that matches this. But maybe it
could become a QAPI struct and very few extra parameters.

Kevin