Re: [Qemu-devel] [PATCH v3 04/11] block: Use block_job_get() in find_block_job()

[Daniel P. Berrange]

On Mon, Jul 04, 2016 at 03:23:14PM +0200, Kevin Wolf wrote:
> Am 02.07.2016 um 16:02 hat Max Reitz geschrieben:
> > On 01.07.2016 17:52, Alberto Garcia wrote:
> > > find_block_job() looks for a block backend with a specified name,
> > > checks whether it has a block job and acquires its AioContext.
> > > 
> > > We want to identify jobs by their ID and not by the block backend
> > > they're attached to, so this patch ignores the backends altogether and
> > > gets the job directly. Apart from making the code simpler, this will
> > > allow us to find block jobs once they start having user-specified IDs.
> > > 
> > > To ensure backward compatibility we keep ERROR_CLASS_DEVICE_NOT_ACTIVE
> > > as the error class if the job doesn't exist. In subsequent patches
> > > we'll also need to keep the device name as the default job ID if the
> > > user doesn't specify a different one.
> > > 
> > > Signed-off-by: Alberto Garcia <address@hidden>
> > > ---
> > >  blockdev.c | 43 ++++++++++++++++---------------------------
> > >  1 file changed, 16 insertions(+), 27 deletions(-)
> > > 
> > > diff --git a/blockdev.c b/blockdev.c
> > > index 3a104a0..8cedb60 100644
> > > --- a/blockdev.c
> > > +++ b/blockdev.c
> > > @@ -3704,42 +3704,31 @@ void qmp_blockdev_mirror(const char *device, 
> > > const char *target,
> > >      aio_context_release(aio_context);
> > >  }
> > >  
> > > -/* Get the block job for a given device name and acquire its AioContext 
> > > */
> > > -static BlockJob *find_block_job(const char *device, AioContext 
> > > **aio_context,
> > > +/* Get a block job using its ID and acquire its AioContext */
> > > +static BlockJob *find_block_job(const char *id, AioContext **aio_context,
> > >                                  Error **errp)
> > >  {
> > > -    BlockBackend *blk;
> > > -    BlockDriverState *bs;
> > > +    BlockJob *job;
> > >  
> > >      *aio_context = NULL;
> > >  
> > > -    blk = blk_by_name(device);
> > > -    if (!blk) {
> > > -        goto notfound;
> > > +    if (!id) {
> > > +        error_setg(errp, "Unspecified job ID when looking for a block 
> > > job");
> > > +        return NULL;
> > >      }
> > 
> > Why no plain assertion? Do you expect callers who may pass a NULL ID?
> > 
> > >  
> > > -    *aio_context = blk_get_aio_context(blk);
> > > +    job = block_job_get(id);
> > > +
> > > +    if (!job) {
> > > +        error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE,
> > > +                  "Block job '%s' not found", id);
> > 
> > This error class seems a bit weird now... I know I advocated for it in
> > v2, but that was because you could actually specifically pass a device
> > name to find block jobs on that device, but now you're just looking for
> > block jobs with a certain ID (that happens to default to the device name).
> 
> libvirt uses the error class, so I don't think we can drop it.

libvirt checks for the following when seeing block job errors.

    if (qemuMonitorJSONErrorIsClass(error, "DeviceNotActive")) {
        virReportError(VIR_ERR_OPERATION_INVALID,
                       _("No active operation on device: %s"), device);
    } else if (qemuMonitorJSONErrorIsClass(error, "DeviceInUse")) {
        virReportError(VIR_ERR_OPERATION_FAILED,
                       _("Device %s in use"), device);
    } else if (qemuMonitorJSONErrorIsClass(error, "NotSupported")) {
        virReportError(VIR_ERR_OPERATION_INVALID,
                       _("Operation is not supported for device: %s"), device);
    } else if (qemuMonitorJSONErrorIsClass(error, "CommandNotFound")) {
        virReportError(VIR_ERR_OPERATION_INVALID,
                       _("Command '%s' is not found"), cmd_name);
    } else {
        virReportError(VIR_ERR_INTERNAL_ERROR,
                       _("Unexpected error: (%s) '%s'"),
                       NULLSTR(virJSONValueObjectGetString(error, "class")),
                       NULLSTR(virJSONValueObjectGetString(error, "desc")));
    }

So yes we use it, but if you changed it to a different error class it
won't neccessarily break libvirt - we'd just end up in the final else
case, and report a different error code + message. It is possible that
this might upset an app using libvirt that checked the error code but
its fairly slim chance.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|