| Message ID | c7436026ecaf4bf0c96187427bc186a117da1bf0.1467386530.git.berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
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 <berto@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). Anyway, nothing's really wrong, so: Reviewed-by: Max Reitz <mreitz@redhat.com> > + return NULL; > + } > + > + *aio_context = blk_get_aio_context(job->blk); > aio_context_acquire(*aio_context); > > - if (!blk_is_available(blk)) { > - goto notfound; > - } > - bs = blk_bs(blk); > - > - if (!bs->job) { > - goto notfound; > - } > - > - return bs->job; > - > -notfound: > - error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE, > - "No active block job on device '%s'", device); > - if (*aio_context) { > - aio_context_release(*aio_context); > - *aio_context = NULL; > - } > - return NULL; > + return job; > } > > void qmp_block_job_set_speed(const char *device, int64_t speed, Error **errp) >
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 <berto@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. Kevin
On Sat 02 Jul 2016 04:02:11 PM CEST, Max Reitz wrote: >> +/* 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? I think you're right, I'll use an assertion instead. I assume I can keep your R-b if I just change that ? Berto
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 <berto@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
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; } - *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); + return NULL; + } + + *aio_context = blk_get_aio_context(job->blk); aio_context_acquire(*aio_context); - if (!blk_is_available(blk)) { - goto notfound; - } - bs = blk_bs(blk); - - if (!bs->job) { - goto notfound; - } - - return bs->job; - -notfound: - error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE, - "No active block job on device '%s'", device); - if (*aio_context) { - aio_context_release(*aio_context); - *aio_context = NULL; - } - return NULL; + return job; } void qmp_block_job_set_speed(const char *device, int64_t speed, Error **errp)
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 <berto@igalia.com> --- blockdev.c | 43 ++++++++++++++++--------------------------- 1 file changed, 16 insertions(+), 27 deletions(-)