| Message ID | 3d3617592f14762be20abff8d4a3937e79b133dc.1465459496.git.berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09.06.2016 10:20, Alberto Garcia wrote: > Currently the way to look for a specific block job is to iterate the > list manually using block_job_next(). > > Since we want to be able to identify a job primarily by its ID it > makes sense to have a function that does just that. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > blockjob.c | 13 +++++++++++++ > include/block/blockjob.h | 10 ++++++++++ > 2 files changed, 23 insertions(+) Reviewed-by: Max Reitz <mreitz@redhat.com> Just to throw out a suggestion, I'm not sure how useful it will be after the rest of the series: Would it make sense to prevent name clashes between block job IDs and device IDs (i.e. BlockBackend names)? If we did that, this function could be used to identify a block job both based on its name and its device. Just a suggestion, though, I don't think it will be necessary. Legacy applications will always create jobs with auto-generated IDs that cannot clash with BB names anyway. If we require applications that do name their block jobs manually to always use this ID when specifying a block job, everything will be fine even if block job and BB names do clash. Max
On 06/20/2016 10:24 AM, Max Reitz wrote: > On 09.06.2016 10:20, Alberto Garcia wrote: >> Currently the way to look for a specific block job is to iterate the >> list manually using block_job_next(). >> >> Since we want to be able to identify a job primarily by its ID it >> makes sense to have a function that does just that. >> >> Signed-off-by: Alberto Garcia <berto@igalia.com> >> --- >> blockjob.c | 13 +++++++++++++ >> include/block/blockjob.h | 10 ++++++++++ >> 2 files changed, 23 insertions(+) > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > Just to throw out a suggestion, I'm not sure how useful it will be after > the rest of the series: > > Would it make sense to prevent name clashes between block job IDs and > device IDs (i.e. BlockBackend names)? If we did that, this function > could be used to identify a block job both based on its name and its device. Adding the restriction now, and then relaxing it later, is easier than keeping it relaxed and wishing we could tighten it later. > > Just a suggestion, though, I don't think it will be necessary. Legacy > applications will always create jobs with auto-generated IDs that cannot > clash with BB names anyway. If we require applications that do name > their block jobs manually to always use this ID when specifying a block > job, everything will be fine even if block job and BB names do clash. But I think you're right here - legacy users will be just fine (at most one job per device, never using the id but also never clashing an id with device names), and new users should just always assign a job id and use that rather than relying on device names. So I don't think we need the extra restriction of merging device and job id namespaces into one.
On 20.06.2016 20:48, Eric Blake wrote: > On 06/20/2016 10:24 AM, Max Reitz wrote: >> On 09.06.2016 10:20, Alberto Garcia wrote: >>> Currently the way to look for a specific block job is to iterate the >>> list manually using block_job_next(). >>> >>> Since we want to be able to identify a job primarily by its ID it >>> makes sense to have a function that does just that. >>> >>> Signed-off-by: Alberto Garcia <berto@igalia.com> >>> --- >>> blockjob.c | 13 +++++++++++++ >>> include/block/blockjob.h | 10 ++++++++++ >>> 2 files changed, 23 insertions(+) >> >> Reviewed-by: Max Reitz <mreitz@redhat.com> >> >> Just to throw out a suggestion, I'm not sure how useful it will be after >> the rest of the series: >> >> Would it make sense to prevent name clashes between block job IDs and >> device IDs (i.e. BlockBackend names)? If we did that, this function >> could be used to identify a block job both based on its name and its device. > > Adding the restriction now, and then relaxing it later, is easier than > keeping it relaxed and wishing we could tighten it later. > >> >> Just a suggestion, though, I don't think it will be necessary. Legacy >> applications will always create jobs with auto-generated IDs that cannot >> clash with BB names anyway. If we require applications that do name >> their block jobs manually to always use this ID when specifying a block >> job, everything will be fine even if block job and BB names do clash. > > But I think you're right here - legacy users will be just fine (at most > one job per device, never using the id but also never clashing an id > with device names), and new users should just always assign a job id and > use that rather than relying on device names. So I don't think we need > the extra restriction of merging device and job id namespaces into one. Yes, also, after having reviewed (most of) the rest of the series, I've seen that Berto has added the ID as a separate parameter of all block job commands. Therefore, there really is no need to restrict the job ID namespace. Max
On Mon 20 Jun 2016 06:24:30 PM CEST, Max Reitz wrote: >> Currently the way to look for a specific block job is to iterate the >> list manually using block_job_next(). >> >> Since we want to be able to identify a job primarily by its ID it >> makes sense to have a function that does just that. >> >> Signed-off-by: Alberto Garcia <berto@igalia.com> >> --- >> blockjob.c | 13 +++++++++++++ >> include/block/blockjob.h | 10 ++++++++++ >> 2 files changed, 23 insertions(+) > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > Just to throw out a suggestion, I'm not sure how useful it will be > after the rest of the series: > > Would it make sense to prevent name clashes between block job IDs and > device IDs (i.e. BlockBackend names)? If we did that, this function > could be used to identify a block job both based on its name and its > device. I considered that. Apart from the reasons that have been mentioned, there's also the problem that the user doesn't have control over the device name when creating jobs, so for example you could not start a new job on a device with the same name as an existing job, or two jobs on the same device. These two cases are a bit far-fetched (the latter is anyway not supported yet), but they hinted me that it's better to make the use of the job ID explicit and leave the device name for older clients. Berto
diff --git a/blockjob.c b/blockjob.c index 4d42987..a4a1caf 100644 --- a/blockjob.c +++ b/blockjob.c @@ -61,6 +61,19 @@ BlockJob *block_job_next(BlockJob *job) return QLIST_NEXT(job, job_list); } +BlockJob *block_job_get(const char *id) +{ + BlockJob *job; + + QLIST_FOREACH(job, &block_jobs, job_list) { + if (!strcmp(id, job->id)) { + return job; + } + } + + return NULL; +} + void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs, int64_t speed, BlockCompletionFunc *cb, void *opaque, Error **errp) diff --git a/include/block/blockjob.h b/include/block/blockjob.h index 1533006..46d2af2 100644 --- a/include/block/blockjob.h +++ b/include/block/blockjob.h @@ -191,6 +191,16 @@ struct BlockJob { BlockJob *block_job_next(BlockJob *job); /** + * block_job_get: + * @id: The id of the block job. + * + * Get the block job identified by @id (which must not be %NULL). + * + * Returns the requested job, or %NULL if it doesn't exist. + */ +BlockJob *block_job_get(const char *id); + +/** * block_job_create: * @job_type: The class object for the newly-created job. * @bs: The block
Currently the way to look for a specific block job is to iterate the list manually using block_job_next(). Since we want to be able to identify a job primarily by its ID it makes sense to have a function that does just that. Signed-off-by: Alberto Garcia <berto@igalia.com> --- blockjob.c | 13 +++++++++++++ include/block/blockjob.h | 10 ++++++++++ 2 files changed, 23 insertions(+)