| Message ID | fdae47df8af588bd661cfff0b6b61aba7344fe8d.1465459496.git.berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09.06.2016 10:20, 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. This > patch uses block_job_next() and iterate directly over the block jobs. > > In addition to that we want to identify jobs primarily by their ID, so > this patch updates find_block_job() to allow IDs too. Only one of ID > and device name can be specified when looking for a block job. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > blockdev.c | 66 +++++++++++++++++++++++++++++++++----------------------------- > 1 file changed, 35 insertions(+), 31 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index 52ec4ae..bd0d5a1 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -3689,48 +3689,52 @@ 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, > - Error **errp) > +/* Get a block job using its ID or device name and acquire its AioContext */ I think it would be good if this comment could mention that only either of ID and device name may be specified. Not strictly necessary, though, since that check is done right at the start of the function. > +static BlockJob *find_block_job(const char *id, const char *device, > + AioContext **aio_context, Error **errp) > { > - BlockBackend *blk; > - BlockDriverState *bs; > + BlockJob *job = NULL; > > *aio_context = NULL; > > - blk = blk_by_name(device); > - if (!blk) { > - goto notfound; > + if ((id && device) || (!id && !device)) { > + error_setg(errp, "Only one of ID or device name " Maybe s/Only/Exactly/. Or it could just be an assertion. > + "must be specified when looking for a block job"); > + return NULL; > } > > - *aio_context = blk_get_aio_context(blk); > - aio_context_acquire(*aio_context); > - > - if (!blk_is_available(blk)) { > - goto notfound; > + if (id) { > + job = block_job_get(id); > + } else { > + BlockJob *j; > + for (j = block_job_next(NULL); j; j = block_job_next(j)) { > + if (!strcmp(device, j->device)) { > + if (job) { > + /* This scenario is currently not possible, but it > + * could theoretically happen in the future. */ > + error_setg(errp, "More than one job on device '%s', " > + "use the job ID instead", device); > + return NULL; > + } > + job = j; > + } > + } > } > - bs = blk_bs(blk); > > - if (!bs->job) { > - goto notfound; > + if (job) { > + *aio_context = blk_get_aio_context(job->blk); > + aio_context_acquire(*aio_context); > + } else { > + error_setg(errp, "Block job '%s' not found", id ? id : device); This changes the error class if device is set. Not sure if that is bad, but keeping the old behavior should be simple (unless you're sure it's fine). Also, but this is a personal opinion, I'd make the error path more explicit, i.e.: if (!job) { /* ... */ return NULL; } *aio_context = /* ... */ /* ...*/ > } > > - 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; > } > What's keeping me from giving an R-b is that I don't know whether changing the error class will be fine. If you say it is, OK then. But just keeping the old behavior should be simple enough, too. Max
On 20.06.2016 18:40, Max Reitz wrote: > On 09.06.2016 10:20, 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. This >> patch uses block_job_next() and iterate directly over the block jobs. >> >> In addition to that we want to identify jobs primarily by their ID, so >> this patch updates find_block_job() to allow IDs too. Only one of ID >> and device name can be specified when looking for a block job. >> >> Signed-off-by: Alberto Garcia <berto@igalia.com> >> --- >> blockdev.c | 66 +++++++++++++++++++++++++++++++++----------------------------- >> 1 file changed, 35 insertions(+), 31 deletions(-) >> >> diff --git a/blockdev.c b/blockdev.c >> index 52ec4ae..bd0d5a1 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -3689,48 +3689,52 @@ void qmp_blockdev_mirror(const char *device, const char *target, [...] >> +static BlockJob *find_block_job(const char *id, const char *device, >> + AioContext **aio_context, Error **errp) >> { >> - BlockBackend *blk; >> - BlockDriverState *bs; >> + BlockJob *job = NULL; >> >> *aio_context = NULL; >> >> - blk = blk_by_name(device); >> - if (!blk) { >> - goto notfound; >> + if ((id && device) || (!id && !device)) { >> + error_setg(errp, "Only one of ID or device name " > > Maybe s/Only/Exactly/. Or it could just be an assertion. Message from reviewing-patch-10-future-me: No, it should not be an assertion. Max >> + "must be specified when looking for a block job"); >> + return NULL; >> } >>
On 06/09/2016 02:20 AM, 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. This > patch uses block_job_next() and iterate directly over the block jobs. > > In addition to that we want to identify jobs primarily by their ID, so > this patch updates find_block_job() to allow IDs too. Only one of ID > and device name can be specified when looking for a block job. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > blockdev.c | 66 +++++++++++++++++++++++++++++++++----------------------------- > 1 file changed, 35 insertions(+), 31 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index 52ec4ae..bd0d5a1 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -3689,48 +3689,52 @@ 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, > - Error **errp) > +/* Get a block job using its ID or device name and acquire its AioContext */ > +static BlockJob *find_block_job(const char *id, const char *device, > + AioContext **aio_context, Error **errp) Can this signature just be const char *id_or_device, rather than two parameters,... > { > - BlockBackend *blk; > - BlockDriverState *bs; > + BlockJob *job = NULL; > > *aio_context = NULL; > > - blk = blk_by_name(device); > - if (!blk) { > - goto notfound; > + if ((id && device) || (!id && !device)) { > + error_setg(errp, "Only one of ID or device name " > + "must be specified when looking for a block job"); > + return NULL; > } If you keep this check, you could simplify it to: if (!id == !device) { ...at which point you don't need this check,... > > - *aio_context = blk_get_aio_context(blk); > - aio_context_acquire(*aio_context); > - > - if (!blk_is_available(blk)) { > - goto notfound; > + if (id) { > + job = block_job_get(id); > + } else { ...and this would become: job = block_job_get(id); if (!job) {
On Mon 20 Jun 2016 08:53:08 PM CEST, Eric Blake wrote: >> +static BlockJob *find_block_job(const char *id, const char *device, >> + AioContext **aio_context, Error **errp) > > Can this signature just be const char *id_or_device, rather than two > parameters,... But what if there's a name clash? Berto
On Mon 20 Jun 2016 06:40:31 PM CEST, Max Reitz wrote: >> + error_setg(errp, "Block job '%s' not found", id ? id : device); > > This changes the error class if device is set. Not sure if that is > bad, but keeping the old behavior should be simple (unless you're sure > it's fine). You're right, I'll fix it. > Also, but this is a personal opinion, I'd make the error path more > explicit, i.e.: > > if (!job) { > /* ... */ > return NULL; > } Yeah, why not. Berto
On 06/21/2016 06:27 AM, Alberto Garcia wrote: > On Mon 20 Jun 2016 08:53:08 PM CEST, Eric Blake wrote: >>> +static BlockJob *find_block_job(const char *id, const char *device, >>> + AioContext **aio_context, Error **errp) >> >> Can this signature just be const char *id_or_device, rather than two >> parameters,... > > But what if there's a name clash? All jobs have an id. And legacy users never set an id, so the id of legacy jobs will always be generated (and you can tell by the # character in the name that it was a generated id). Basically, I'm proposing a hierarchical lookup: If the id matches, then you use it. If the id doesn't match, then do a device lookup name. New code that knows to set the id should never pass anything but valid ids, so we only have to worry about the case of new code trying to check the status of a job that no longer exists and accidentally getting the status of an unrelated job that happened to belong to a device with the same name - but any new code shouldn't be that stupid as to use job ids that match device ids.
On Tue 21 Jun 2016 11:36:24 PM CEST, Eric Blake <eblake@redhat.com> wrote: > On 06/21/2016 06:27 AM, Alberto Garcia wrote: >> On Mon 20 Jun 2016 08:53:08 PM CEST, Eric Blake wrote: >>>> +static BlockJob *find_block_job(const char *id, const char *device, >>>> + AioContext **aio_context, Error **errp) >>> >>> Can this signature just be const char *id_or_device, rather than two >>> parameters,... >> >> But what if there's a name clash? > > All jobs have an id. And legacy users never set an id, so the id of > legacy jobs will always be generated (and you can tell by the # > character in the name that it was a generated id). Basically, I'm > proposing a hierarchical lookup: If the id matches, then you use it. If > the id doesn't match, then do a device lookup name. New code that knows > to set the id should never pass anything but valid ids, so we only have > to worry about the case of new code trying to check the status of a job > that no longer exists and accidentally getting the status of an > unrelated job that happened to belong to a device with the same name - > but any new code shouldn't be that stupid as to use job ids that match > device ids. Yeah, it *shouldn't*, but is it a good idea to leave this door open just to have a slightly simpler internal API? Berto
diff --git a/blockdev.c b/blockdev.c index 52ec4ae..bd0d5a1 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3689,48 +3689,52 @@ 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, - Error **errp) +/* Get a block job using its ID or device name and acquire its AioContext */ +static BlockJob *find_block_job(const char *id, const char *device, + AioContext **aio_context, Error **errp) { - BlockBackend *blk; - BlockDriverState *bs; + BlockJob *job = NULL; *aio_context = NULL; - blk = blk_by_name(device); - if (!blk) { - goto notfound; + if ((id && device) || (!id && !device)) { + error_setg(errp, "Only one of ID or device name " + "must be specified when looking for a block job"); + return NULL; } - *aio_context = blk_get_aio_context(blk); - aio_context_acquire(*aio_context); - - if (!blk_is_available(blk)) { - goto notfound; + if (id) { + job = block_job_get(id); + } else { + BlockJob *j; + for (j = block_job_next(NULL); j; j = block_job_next(j)) { + if (!strcmp(device, j->device)) { + if (job) { + /* This scenario is currently not possible, but it + * could theoretically happen in the future. */ + error_setg(errp, "More than one job on device '%s', " + "use the job ID instead", device); + return NULL; + } + job = j; + } + } } - bs = blk_bs(blk); - if (!bs->job) { - goto notfound; + if (job) { + *aio_context = blk_get_aio_context(job->blk); + aio_context_acquire(*aio_context); + } else { + error_setg(errp, "Block job '%s' not found", id ? id : device); } - 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) { AioContext *aio_context; - BlockJob *job = find_block_job(device, &aio_context, errp); + BlockJob *job = find_block_job(NULL, device, &aio_context, errp); if (!job) { return; @@ -3744,7 +3748,7 @@ void qmp_block_job_cancel(const char *device, bool has_force, bool force, Error **errp) { AioContext *aio_context; - BlockJob *job = find_block_job(device, &aio_context, errp); + BlockJob *job = find_block_job(NULL, device, &aio_context, errp); if (!job) { return; @@ -3769,7 +3773,7 @@ out: void qmp_block_job_pause(const char *device, Error **errp) { AioContext *aio_context; - BlockJob *job = find_block_job(device, &aio_context, errp); + BlockJob *job = find_block_job(NULL, device, &aio_context, errp); if (!job || job->user_paused) { return; @@ -3784,7 +3788,7 @@ void qmp_block_job_pause(const char *device, Error **errp) void qmp_block_job_resume(const char *device, Error **errp) { AioContext *aio_context; - BlockJob *job = find_block_job(device, &aio_context, errp); + BlockJob *job = find_block_job(NULL, device, &aio_context, errp); if (!job || !job->user_paused) { return; @@ -3799,7 +3803,7 @@ void qmp_block_job_resume(const char *device, Error **errp) void qmp_block_job_complete(const char *device, Error **errp) { AioContext *aio_context; - BlockJob *job = find_block_job(device, &aio_context, errp); + BlockJob *job = find_block_job(NULL, device, &aio_context, errp); if (!job) { return;
find_block_job() looks for a block backend with a specified name, checks whether it has a block job and acquires its AioContext. This patch uses block_job_next() and iterate directly over the block jobs. In addition to that we want to identify jobs primarily by their ID, so this patch updates find_block_job() to allow IDs too. Only one of ID and device name can be specified when looking for a block job. Signed-off-by: Alberto Garcia <berto@igalia.com> --- blockdev.c | 66 +++++++++++++++++++++++++++++++++----------------------------- 1 file changed, 35 insertions(+), 31 deletions(-)