From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37573) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bK4Vs-0007SW-Jp for qemu-devel@nongnu.org; Mon, 04 Jul 2016 10:06:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bK4Vm-0000XF-JD for qemu-devel@nongnu.org; Mon, 04 Jul 2016 10:06:16 -0400 Date: Mon, 4 Jul 2016 15:05:58 +0100 From: "Daniel P. Berrange" Message-ID: <20160704140558.GG3763@redhat.com> Reply-To: "Daniel P. Berrange" References: <05898d8b-4c7d-f2bc-d128-68566a8d6308@redhat.com> <20160704132314.GF5399@noname.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160704132314.GF5399@noname.redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 04/11] block: Use block_job_get() in find_block_job() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Max Reitz , Alberto Garcia , qemu-block@nongnu.org, Jeff Cody , qemu-devel@nongnu.org, John Snow 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 > > > --- > > > 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 :|