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 <be...@igalia.com>
> > > ---
> > > 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 :|
