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). Anyway, nothing's really wrong, so: Reviewed-by: Max Reitz <mre...@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) >
signature.asc
Description: OpenPGP digital signature