Add an 'id' field to block jobs
cover Add an 'id' field to block jobs
Hello all,
Block jobs are currently identified by the name of the block backend of the BDS where the job was started.
This series adds a new 'id' field that is specific to the block job and guaranteed to be unique and always present.
The patches can be summarized as follows:
- 1: Fix the prototype of stream_start(), that is going to be touched by a later patch in the series.
- 2-5: Split BlockJob's 'id' field into 'id' and 'device' and adjust the internal APIs to allow using the job ID.
- 6-9: Add parameters to allow setting the job ID to all QMP commands that create block jobs (block-stream, block-commit, ...)
- 10-14: Add parameters to allow using the job ID to all QMP command that manipulate block jobs (cancel, pause, complete, ...).
- 15: Expose the ID field in BlockJobInfo and all BLOCK_JOB_* events.
Things to do:
- Add new tests specific to job IDs.
- Add job ID support to HMP commands.
The series applies on top of Max's block branch (commit 3cc2f35a59) but it should work on master as well.
Questions, comments, etc, are welcome.
Thanks!
Berto
Alberto Garcia (15):
stream: Fix prototype of stream_start()
blockjob: Decouple the ID from the device name in the BlockJob struct
blockjob: Add block_job_get()
block: Simplify find_block_job() and make it accept a job ID
blockjob: Add 'job_id' parameter to block_job_create()
mirror: Add 'job-id' parameter to 'blockdev-mirror' and 'drive-mirror'
backup: Add 'job-id' parameter to 'blockdev-backup' and 'drive-backup'
stream: Add 'job-id' parameter to 'block-stream'
stream: Add 'job-id' parameter to 'block-commit'
blockjob: Add 'id' parameter to 'block-job-set-speed'
blockjob: Add 'id' parameter to 'block-job-cancel'
blockjob: Add 'id' parameter to 'block-job-pause'
blockjob: Add 'id' parameter to 'block-job-resume'
blockjob: Add 'id' parameter to 'block-job-complete'
blockjob: Add 'id' field to 'BlockJobInfo' and all BLOCK_JOB_* events
block/backup.c | 9 +-- block/commit.c | 7 +- block/mirror.c | 18 ++--- block/stream.c | 12 ++-- blockdev.c | 164 ++++++++++++++++++++++++++------------------- blockjob.c | 43 ++++++++++-- docs/qmp-events.txt | 4 ++ hmp.c | 16 ++--- include/block/block_int.h | 47 ++++++++----- include/block/blockjob.h | 30 +++++++-- include/qemu/id.h | 1 + qapi/block-core.json | 91 +++++++++++++++++++------ qemu-img.c | 2 +- qmp-commands.hx | 32 +++++---- tests/qemu-iotests/095 | 2 +- tests/qemu-iotests/095.out | 2 +- tests/qemu-iotests/124 | 3 +- tests/qemu-iotests/141 | 6 +- tests/qemu-iotests/141.out | 14 ++-- tests/qemu-iotests/144 | 1 + tests/qemu-iotests/144.out | 4 +- tests/test-blockjob-txn.c | 2 +- util/id.c | 1 + 23 files changed, 334 insertions(+), 177 deletions(-)
Signature · 1 line
2.8.1
PATCH 1/15 stream: Fix prototype of stream_start()
'stream-start' has a parameter called 'backing-file', which is the string to be written to bs->backing when the job finishes.
In the stream_start() implementation it is called 'backing_file_str', but it the prototype in the header file it is called 'base_id'.
This patch fixes it so the name is the same in both cases and is consistent with other cases (like commit_start()).
--- include/block/block_int.h | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
include/block/block_int.h +6 -5
@@ -610,8 +610,8 @@ int is_windows_drive(const char *filename);* @bs: Block device to operate on. * @base: Block device that will become the new base, or %NULL to * flatten the whole backing file chain onto @bs. -* @base_id: The file name that will be written to @bs as the new -* backing file if the job completes. Ignored if @base is %NULL. +* @backing_file_str: The file name that will be written to @bs as the +* the new backing file if the job completes. Ignored if @base is %NULL. * @speed: The maximum speed, in bytes per second, or 0 for unlimited. * @on_error: The action to take upon error. * @cb: Completion function for the job.
@@ -622,11 +622,12 @@ int is_windows_drive(const char *filename);* in @bs, but allocated in any image between @base and @bs (both * exclusive) will be written to @bs. At the end of a successful * streaming job, the backing file of @bs will be changed to -* @base_id in the written image and to @base in the live BlockDriverState. +* @backing_file_str in the written image and to @base in the live +* BlockDriverState. */ void stream_start (BlockDriverState *bs,BlockDriverState *base, -const char *base_id,int64_t speed,BlockdevOnError on_error, -BlockCompletionFunc *cb, +const char *backing_file_str,int64_t speed, +BlockdevOnError on_error,BlockCompletionFunc *cb,void *opaque,Error **errp);/**
Signature · 1 line
2.8.1
Re: [Qemu-devel] [PATCH 01/15] stream: Fix prototype of stream_start()
Quoted 6 lines · Alberto Garcia
'stream-start' has a parameter called 'backing-file', which is the string to be written to
bs->backingwhen the job finishes.In the
stream_start()implementation it is called 'backing_file_str', but it the prototype in the header file it is called 'base_id'.
Quoted 7 lines
This patch fixes it so the name is the same in both cases and is consistent with other cases (like
commit_start()).Signed-off-by Alberto Garcia <[email protected]>--- include/block/block_int.h | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
PATCH 2/15 blockjob: Decouple the ID from the device name in the BlockJob struct
In reply to Alberto GarciaBlock jobs are identified by the name of the BlockBackend of the BDS where the job was started.
We want block jobs to have unique, arbitrary identifiers that are not tied to a block device, so this patch decouples the ID from the device name in the BlockJob structure.
The ID is generated automatically for the moment, in later patches we'll allow the user to set it.
--- blockjob.c | 15 +++++++++------ include/block/blockjob.h | 12 ++++++++---- include/qemu/id.h | 1 + util/id.c | 1 + 4 files changed, 19 insertions(+), 10 deletions(-)
blockjob.c +9 -6
@@ -33,6 +33,7 @@#include "qapi/qmp/qerror.h" #include "qapi/qmp/qjson.h" #include "qemu/coroutine.h" +#include "qemu/id.h" #include "qmp-commands.h" #include "qemu/timer.h" #include "qapi-event.h"
@@ -82,7 +83,8 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,bdrv_op_unblock (bs,BLOCK_OP_TYPE_DATAPLANE , job->blocker); job->driver = driver; - job->id =g_strdup (bdrv_get_device_name (bs)); + job->device =g_strdup (bdrv_get_device_name (bs)); + job->id =id_generate (ID_JOB ); job->blk = blk; job->cb = cb; job->opaque = opaque;
@@ -119,6 +121,7 @@ void block_job_unref(BlockJob *job)bdrv_op_unblock_all (bs, job->blocker);blk_unref (job->blk);error_free (job->blocker); +g_free (job->device);g_free (job->id);QLIST_REMOVE (job, job_list);g_free (job);
@@ -389,7 +392,7 @@ BlockJobInfo *block_job_query(BlockJob *job) {BlockJobInfo *info =g_new0 (BlockJobInfo,1 ); info->type =g_strdup (BlockJobType_lookup[job->driver->job_type]); - info->device =g_strdup (job->id); + info->device =g_strdup (job->device); info->len = job->len; info->busy = job->busy; info->paused = job->pause_count >0 ;
@@ -411,7 +414,7 @@ static void block_job_iostatus_set_err(BlockJob *job, int error)void block_job_event_cancelled (BlockJob *job) { qapi_event_send_block_job_cancelled(job->driver->job_type, - job->id, + job->device, job->len, job->offset, job->speed,
@@ -421,7 +424,7 @@ void block_job_event_cancelled(BlockJob *job)void block_job_event_completed (BlockJob *job,const char *msg) { qapi_event_send_block_job_completed(job->driver->job_type, - job->id, + job->device, job->len, job->offset, job->speed,
@@ -435,7 +438,7 @@ void block_job_event_ready(BlockJob *job) job->ready = true;qapi_event_send_block_job_ready (job->driver->job_type, - job->id, + job->device, job->len, job->offset, job->speed, &error_abort);
@@ -463,7 +466,7 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err,default :abort (); } -qapi_event_send_block_job_error (job->id, +qapi_event_send_block_job_error (job->device, is_read ?IO_OPERATION_TYPE_READ :IO_OPERATION_TYPE_WRITE , action, &error_abort);
include/block/blockjob.h +8 -4
@@ -85,14 +85,18 @@ struct BlockJob {BlockBackend *blk;/** -* The ID of the block job. Currently the BlockBackend name of the BDS -* owning the job at the time when the job is started. -* -* TODO Decouple block job IDs from BlockBackend names +* The ID of the block job. */ char *id;/** +* BlockBackend name of the BDS owning the job at the time when +* the job is started. For compatibility with clients that don't +* support the ID field. +*/ +char *device; + +/** * The coroutine that executes the job. If not NULL, it is * reentered when busy is false and the job is cancelled. */
include/qemu/id.h +1 -0
@@ -4,6 +4,7 @@typedef enum IdSubSystems {ID_QDEV ,ID_BLOCK , +ID_JOB ,ID_MAX /* last element, used as array size */ }IdSubSystems ;
util/id.c +1 -0
@@ -34,6 +34,7 @@ bool id_wellformed(const char *id)static const char *const id_subsys_str[ID_MAX ] = { [ID_QDEV ] ="qdev" , [ID_BLOCK ] ="block" , + [ID_JOB ] ="job" , };/*
Signature · 1 line
2.8.1
Re: [Qemu-devel] [PATCH 02/15] blockjob: Decouple the ID from the device name in the BlockJob struct
Quoted 10 lines · Alberto Garcia
Block jobs are identified by the name of the BlockBackend of the BDS where the job was started.
We want block jobs to have unique, arbitrary identifiers that are not tied to a block device, so this patch decouples the ID from the device name in the BlockJob structure.
The ID is generated automatically for the moment, in later patches we'll allow the user to set it.
Quoted 7 lines
Signed-off-by Alberto Garcia <[email protected]>--- blockjob.c | 15 +++++++++------ include/block/blockjob.h | 12 ++++++++---- include/qemu/id.h | 1 + util/id.c | 1 + 4 files changed, 19 insertions(+), 10 deletions(-)
I suppose this patch tries to "silently" (i.e. not visibly) introduce this new ID for now? If so, there is one instance of job->id left that should probably be changed to job->device (in block_job_complete()).
Max
Re: [Qemu-devel] [PATCH 02/15] blockjob: Decouple the ID from the device name in the BlockJob struct
Quoted 3 lines · Max Reitz
I suppose this patch tries to "silently" (i.e. not visibly) introduce this new ID for now? If so, there is one instance of
job->idleft that should probably be changed tojob->device(inblock_job_complete()).
I decided to leave that one unchanged because it's an error message, it doesn't have any other practical impact and it would need to be renamed back to job->id anyway.
But now that you mentioned it I see that the error message actually says "...the block job for device '%s'...", so I think I'd rather leave job->id and change the error message instead.
Berto
PATCH 3/15 blockjob: Add block_job_get()
In reply to Alberto GarciaCurrently 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.
--- blockjob.c | 13 +++++++++++++ include/block/blockjob.h | 10 ++++++++++ 2 files changed, 23 insertions(+)
blockjob.c +13 -0
@@ -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)
include/block/blockjob.h +10 -0
@@ -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
Signature · 1 line
2.8.1
Re: [Qemu-devel] [PATCH 03/15] blockjob: Add block_job_get()
Quoted 11 lines · Alberto Garcia
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 <[email protected]>--- blockjob.c | 13 +++++++++++++ include/block/blockjob.h | 10 ++++++++++ 2 files changed, 23 insertions(+)
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
Re: [Qemu-devel] [PATCH 03/15] blockjob: Add block_job_get()
Quoted 13 lines · Max Reitz
Quoted 11 lines · Alberto Garcia
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 <[email protected]>--- blockjob.c | 13 +++++++++++++ include/block/blockjob.h | 10 ++++++++++ 2 files changed, 23 insertions(+)
Quoted 8 lines
Reviewed-by Max Reitz <[email protected]>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.
Quoted 6 lines
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.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 03/15] blockjob: Add block_job_get()
Quoted 33 lines · Eric Blake
Quoted 13 lines · Max Reitz
Quoted 11 lines · Alberto Garcia
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 <[email protected]>--- blockjob.c | 13 +++++++++++++ include/block/blockjob.h | 10 ++++++++++ 2 files changed, 23 insertions(+)Quoted 8 lines
Reviewed-by Max Reitz <[email protected]>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.
Quoted 6 lines
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.
Quoted 5 lines
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
Re: [Qemu-devel] [PATCH 03/15] blockjob: Add block_job_get()
In reply to Max ReitzQuoted 14 lines · Max Reitz
Quoted 11 lines
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 <[email protected]>--- blockjob.c | 13 +++++++++++++ include/block/blockjob.h | 10 ++++++++++ 2 files changed, 23 insertions(+)Reviewed-by Max Reitz <[email protected]>
Quoted 7 lines
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
PATCH 4/15 block: Simplify find_block_job() and make it accept a job ID
In reply to Alberto Garciafind_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.
--- blockdev.c | 66 +++++++++++++++++++++++++++++++++----------------------------- 1 file changed, 35 insertions(+), 31 deletions(-)
blockdev.c +35 -31
@@ -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 ;
Signature · 1 line
2.8.1
Re: [Qemu-devel] [PATCH 04/15] block: Simplify find_block_job() and make it accept a job ID
Quoted 18 lines · Alberto Garcia
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 usesblock_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 <[email protected]>--- blockdev.c | 66 +++++++++++++++++++++++++++++++++----------------------------- 1 file changed, 35 insertions(+), 31 deletions(-)blockdev.c +0
-0
blockdev.c void qmp_blockdev_mirror(const char *device, const char *target, · line 3692
Quoted 7 lines
blockdev.c +1
-3@@ -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.
Quoted 4 lines
blockdev.c +2
-1+static BlockJob *find_block_job (const char *id,const char *device, +AioContext **aio_context,Error **errp) { -BlockBackend *blk;
blockdev.c void qmp_blockdev_mirror(const char *device, const char *target, · line 3701
Quoted 10 lines
blockdev.c +3
-4-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.
Quoted 25 lines
blockdev.c +18
-5+ "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; + } + }
blockdev.c void qmp_blockdev_mirror(const char *device, const char *target, · line 3728
Quoted 10 lines
blockdev.c +5
-3} - 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 = /* ... */
/* ...*/
Quoted 5 lines
blockdev.c +0
-3} -return bs->job; - -notfound:
blockdev.c void qmp_blockdev_mirror(const char *device, const char *target, · line 3733
Quoted 10 lines
blockdev.c +1
-7-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
Re: [Qemu-devel] [PATCH 04/15] block: Simplify find_block_job() and make it accept a job ID
Quoted 19 lines · Max Reitz
Quoted 18 lines · Alberto Garcia
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 usesblock_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 <[email protected]>--- blockdev.c | 66 +++++++++++++++++++++++++++++++++----------------------------- 1 file changed, 35 insertions(+), 31 deletions(-)blockdev.c +0
-0@@ -3689,48 +3689,52 @@ void qmp_blockdev_mirror(const char *device, const char *target,
[...]
Quoted 9 lines
Quoted 9 lines
blockdev.c +3
-2+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 ;
Quoted 7 lines
blockdev.c void qmp_blockdev_mirror(const char *device, const char *target, · line 3701
Quoted 5 lines
blockdev.c +2
-3- 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
Quoted 4 lines
Quoted 4 lines
blockdev.c +2
-0+ "must be specified when looking for a block job"); +return NULL ; }
Re: [Qemu-devel] [PATCH 04/15] block: Simplify find_block_job() and make it accept a job ID
In reply to Max ReitzQuoted 5 lines · Max Reitz
Quoted 1 line
+
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.
Quoted 7 lines
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
Re: [Qemu-devel] [PATCH 04/15] block: Simplify find_block_job() and make it accept a job ID
In reply to Alberto GarciaQuoted 18 lines · Alberto Garcia
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 usesblock_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 <[email protected]>--- blockdev.c | 66 +++++++++++++++++++++++++++++++++----------------------------- 1 file changed, 35 insertions(+), 31 deletions(-)blockdev.c +0
-0
blockdev.c void qmp_blockdev_mirror(const char *device, const char *target, · line 3694
Quoted 9 lines
blockdev.c +3
-3@@ -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,...
Quoted 5 lines
blockdev.c +1
-2{ -BlockBackend *blk; -BlockDriverState *bs; +BlockJob *job =NULL ;
blockdev.c void qmp_blockdev_mirror(const char *device, const char *target, · line 3704
Quoted 10 lines
blockdev.c +4
-3*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,...
blockdev.c void qmp_blockdev_mirror(const char *device, const char *target, · line 3708
Quoted 9 lines
blockdev.c +3
-5- *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) {
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 04/15] block: Simplify find_block_job() and make it accept a job ID
Quoted 5 lines · Eric Blake
blockdev.c void qmp_blockdev_mirror(const char *device, const char *target, · line 3694
Quoted 2 lines
blockdev.c +2
-0+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
Re: [Qemu-devel] [PATCH 04/15] block: Simplify find_block_job() and make it accept a job ID
Quoted 8 lines · Alberto Garcia
Quoted 5 lines · Eric Blake
blockdev.c void qmp_blockdev_mirror(const char *device, const char *target, · line 3694
Quoted 2 lines
blockdev.c +2
-0+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.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 04/15] block: Simplify find_block_job() and make it accept a job ID
Quoted 10 lines · Eric Blake
Quoted 8 lines · Alberto Garcia
Quoted 5 lines · Eric Blake
blockdev.c void qmp_blockdev_mirror(const char *device, const char *target, · line 3694
Quoted 2 lines
blockdev.c +2
-0+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?
Quoted 11 lines
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
PATCH 5/15 blockjob: Add 'job_id' parameter to block_job_create()
In reply to Alberto GarciaJob IDs are generated automatically when a new job is created. This patch adds a new 'job_id' parameter to let the caller provide one instead. In this case the ID is verified to be unique and well-formed.
--- block/backup.c | 3 ++- block/commit.c | 2 +- block/mirror.c | 2 +- block/stream.c | 2 +- blockjob.c | 19 +++++++++++++++---- include/block/blockjob.h | 8 +++++--- tests/test-blockjob-txn.c | 2 +- 7 files changed, 26 insertions(+), 12 deletions(-)
block/backup.c +2 -1
@@ -536,7 +536,8 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, goto error; } - job =block_job_create (&backup_job_driver, bs, speed, cb, opaque, errp); + job =block_job_create (NULL , &backup_job_driver, bs, speed, + cb, opaque, errp);if (!job) { goto error; }
block/commit.c +1 -1
@@ -236,7 +236,7 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,return ; } - s =block_job_create (&commit_job_driver, bs, speed, cb, opaque, errp); + s =block_job_create (NULL , &commit_job_driver, bs, speed, cb, opaque, errp);if (!s) {return ; }
block/mirror.c +1 -1
@@ -824,7 +824,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, buf_size =DEFAULT_MIRROR_BUF_SIZE ; } - s =block_job_create (driver, bs, speed, cb, opaque, errp); + s =block_job_create (NULL , driver, bs, speed, cb, opaque, errp);if (!s) {return ; }
block/stream.c +1 -1
@@ -226,7 +226,7 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base, {StreamBlockJob *s; - s =block_job_create (&stream_job_driver, bs, speed, cb, opaque, errp); + s =block_job_create (NULL , &stream_job_driver, bs, speed, cb, opaque, errp);if (!s) {return ; }
blockjob.c +15 -4
@@ -74,9 +74,9 @@ BlockJob *block_job_get(const char *id)return NULL ; } -void *block_job_create (const BlockJobDriver *driver,BlockDriverState *bs, -int64_t speed,BlockCompletionFunc *cb, -void *opaque,Error **errp) +void *block_job_create (const char *job_id,const BlockJobDriver *driver, +BlockDriverState *bs,int64_t speed, +BlockCompletionFunc *cb,void *opaque,Error **errp) {BlockBackend *blk;BlockJob *job;
@@ -86,6 +86,17 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,return NULL ; } +if (job_id) { +if (!id_wellformed (job_id)) { +error_setg (errp,"Invalid job ID '%s'" , job_id); +return NULL ; + } +if (block_job_get (job_id)) { +error_setg (errp,"Job ID '%s' already in use" , job_id); +return NULL ; + } + } + blk =blk_new ();blk_insert_bs (blk, bs);
@@ -97,7 +108,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs, job->driver = driver; job->device =g_strdup (bdrv_get_device_name (bs)); - job->id =id_generate (ID_JOB ); + job->id = job_id ?g_strdup (job_id) :id_generate (ID_JOB ); job->blk = blk; job->cb = cb; job->opaque = opaque;
include/block/blockjob.h +5 -3
@@ -202,6 +202,8 @@ BlockJob *block_job_get(const char *id);/** * block_job_create: +* @job_id: The id of the newly-created job, or %NULL to have one +* generated automatically. * @job_type: The class object for the newly-created job. * @bs: The block * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
@@ -218,9 +220,9 @@ BlockJob *block_job_get(const char *id);* This function is not part of the public job interface; it should be * called from a wrapper that is specific to the job type. */ -void *block_job_create (const BlockJobDriver *driver,BlockDriverState *bs, -int64_t speed,BlockCompletionFunc *cb, -void *opaque,Error **errp); +void *block_job_create (const char *job_id,const BlockJobDriver *driver, +BlockDriverState *bs,int64_t speed, +BlockCompletionFunc *cb,void *opaque,Error **errp);/** * block_job_sleep_ns:
tests/test-blockjob-txn.c +1 -1
@@ -95,7 +95,7 @@ static BlockJob *test_block_job_start(unsigned int iterations, data =g_new0 (TestBlockJobCBData,1 ); bs =bdrv_new (); - s =block_job_create (&test_block_job_driver, bs,0 , test_block_job_cb, + s =block_job_create (NULL , &test_block_job_driver, bs,0 , test_block_job_cb, data, &error_abort); s->iterations = iterations; s->use_timer = use_timer;
Signature · 1 line
2.8.1
Re: [Qemu-devel] [PATCH 05/15] blockjob: Add 'job_id' parameter to block_job_create()
Quoted 5 lines · Alberto Garcia
Job IDs are generated automatically when a new job is created. This patch adds a new '
job_id' parameter to let the caller provide one instead. In this case the ID is verified to be unique and well-formed.Signed-off-by Alberto Garcia <[email protected]>
Quoted 9 lines
--- block/backup.c | 3 ++- block/commit.c | 2 +- block/mirror.c | 2 +- block/stream.c | 2 +- blockjob.c | 19 +++++++++++++++---- include/block/blockjob.h | 8 +++++--- tests/test-blockjob-txn.c | 2 +- 7 files changed, 26 insertions(+), 12 deletions(-)
PATCH 6/15 mirror: Add 'job-id' parameter to 'blockdev-mirror' and 'drive-mirror'
In reply to Alberto GarciaThis patch adds a new optional 'job-id' parameter to 'blockdev-mirror' and 'drive-mirror', allowing the user to specify the ID of the block job to be created.
The HMP 'drive_mirror' command remains unchanged.
--- block/mirror.c | 14 +++++++------- blockdev.c | 15 ++++++++------- hmp.c | 2 +- include/block/block_int.h | 6 ++++-- qapi/block-core.json | 10 +++++++--- qmp-commands.hx | 8 +++++--- 6 files changed, 32 insertions(+), 23 deletions(-)
block/mirror.c +7 -7
@@ -795,8 +795,8 @@ static const BlockJobDriver commit_active_job_driver = { .complete = mirror_complete, }; -static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, - const char *replaces, +static void mirror_start_job(const char *job_id, BlockDriverState *bs, + BlockDriverState *target, const char *replaces, int64_t speed, uint32_t granularity, int64_t buf_size, BlockdevOnError on_source_error,
@@ -824,7 +824,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, buf_size =DEFAULT_MIRROR_BUF_SIZE ; } - s =block_job_create (NULL , driver, bs, speed, cb, opaque, errp); + s =block_job_create (job_id, driver, bs, speed, cb, opaque, errp);if (!s) {return ; }
@@ -856,8 +856,8 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,qemu_coroutine_enter (s->common.co, s); } -void mirror_start(BlockDriverState *bs, BlockDriverState *target, - const char *replaces, +void mirror_start(const char *job_id, BlockDriverState *bs, + BlockDriverState *target, const char *replaces, int64_t speed, uint32_t granularity, int64_t buf_size, MirrorSyncMode mode, BlockdevOnError on_source_error, BlockdevOnError on_target_error,
@@ -874,7 +874,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, } is_none_mode = mode ==MIRROR_SYNC_MODE_NONE ; base = mode ==MIRROR_SYNC_MODE_TOP ?backing_bs (bs) :NULL ; -mirror_start_job (bs, target, replaces, +mirror_start_job (job_id, bs, target, replaces, speed, granularity, buf_size, on_source_error, on_target_error, unmap, cb, opaque, errp, &mirror_job_driver, is_none_mode, base);
@@ -922,7 +922,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, } } -mirror_start_job (bs, base,NULL , speed,0 ,0 , +mirror_start_job (NULL , bs, base,NULL , speed,0 ,0 , on_error, on_error, false, cb, opaque, &local_err, &commit_active_job_driver, false, base);if (local_err) {
blockdev.c +8 -7
@@ -3412,7 +3412,7 @@ void qmp_blockdev_backup(const char *device, const char *target,/* Parameter check and block job starting for drive mirroring. * Caller should hold @device and @target's aio context (must be the same). **/ -static void blockdev_mirror_common(BlockDriverState *bs, +static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs, BlockDriverState *target, bool has_replaces, const char *replaces, enum MirrorSyncMode sync,
@@ -3471,15 +3471,15 @@ static void blockdev_mirror_common(BlockDriverState *bs,/* pass the node name to replace to mirror start since it's loose coupling * and will allow to check whether the node still exist at mirror completion */ -mirror_start (bs, target, +mirror_start (job_id, bs, target, has_replaces ? replaces :NULL , speed, granularity, buf_size, sync, on_source_error, on_target_error, unmap, block_job_cb, bs, errp); } -void qmp_drive_mirror(const char *device, const char *target, - bool has_format, const char *format, +void qmp_drive_mirror(bool has_job_id, const char *job_id, const char *device, + const char *target, bool has_format, const char *format, bool has_node_name, const char *node_name, bool has_replaces, const char *replaces, enum MirrorSyncMode sync,
@@ -3616,7 +3616,7 @@ void qmp_drive_mirror(const char *device, const char *target,bdrv_set_aio_context (target_bs, aio_context); - blockdev_mirror_common(bs, target_bs, + blockdev_mirror_common(has_job_id ? job_id : NULL, bs, target_bs, has_replaces, replaces, sync, has_speed, speed, has_granularity, granularity,
@@ -3633,7 +3633,8 @@ out:aio_context_release (aio_context); } -void qmp_blockdev_mirror(const char *device, const char *target, +void qmp_blockdev_mirror(bool has_job_id, const char *job_id, + const char *device, const char *target, bool has_replaces, const char *replaces, MirrorSyncMode sync, bool has_speed, int64_t speed,
@@ -3673,7 +3674,7 @@ void qmp_blockdev_mirror(const char *device, const char *target,bdrv_set_aio_context (target_bs, aio_context); - blockdev_mirror_common(bs, target_bs, + blockdev_mirror_common(has_job_id ? job_id : NULL, bs, target_bs, has_replaces, replaces, sync, has_speed, speed, has_granularity, granularity,
hmp.c +1 -1
@@ -1093,7 +1093,7 @@ void hmp_drive_mirror(Monitor *mon, const QDict *qdict) mode =NEW_IMAGE_MODE_ABSOLUTE_PATHS ; } - qmp_drive_mirror(device, filename, !!format, format, + qmp_drive_mirror(false, NULL, device, filename, !!format, format, false, NULL, false, NULL, full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP, true, mode, false, 0, false, 0, false, 0,
include/block/block_int.h +4 -2
@@ -665,6 +665,8 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, void *opaque, Error **errp);/* * mirror_start: +* @job_id: The id of the newly-created job, or %NULL to have one +* generated automatically. * @bs: Block device to operate on. * @target: Block device to write to. * @replaces: Block graph node name to replace once the mirror is done. Can
@@ -685,8 +687,8 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,* manually completed. At the end of a successful mirroring job, * @bs will be switched to read from @target. */ -void mirror_start(BlockDriverState *bs, BlockDriverState *target, - const char *replaces, +void mirror_start(const char *job_id, BlockDriverState *bs, + BlockDriverState *target, const char *replaces, int64_t speed, uint32_t granularity, int64_t buf_size, MirrorSyncMode mode, BlockdevOnError on_source_error, BlockdevOnError on_target_error,
qapi/block-core.json +7 -3
@@ -1108,6 +1108,8 @@ # # Start mirroring a block device's writes to a new destination. # +# @job-id: #optional identifier for the newly-created block job (Since 2.7) +# # @device: the name of the device whose writes should be mirrored. # # @target: the target of the new image. If the file exists, or if it
@@ -1160,8 +1162,8 @@ # Since 1.3 ## { 'command': 'drive-mirror', - 'data': { 'device': 'str', 'target': 'str', '*format': 'str', - '*node-name': 'str', '*replaces': 'str', + 'data': { '*job-id': 'str', 'device': 'str', 'target': 'str', + '*format': 'str', '*node-name': 'str', '*replaces': 'str', 'sync': 'MirrorSyncMode', '*mode': 'NewImageMode', '*speed': 'int', '*granularity': 'uint32', '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
@@ -1243,6 +1245,8 @@ # # Start mirroring a block device's writes to a new destination. # +# @job-id: #optional identifier for the newly-created block job (Since 2.7) +# # @device: the name of the device whose writes should be mirrored. # # @target: the id or node-name of the block device to mirror to. This mustn't be
@@ -1279,7 +1283,7 @@ # Since 2.6 ## { 'command': 'blockdev-mirror', - 'data': { 'device': 'str', 'target': 'str', + 'data': { '*job-id': 'str', 'device': 'str', 'target': 'str', '*replaces': 'str', 'sync': 'MirrorSyncMode', '*speed': 'int', '*granularity': 'uint32',
qmp-commands.hx +5 -3
@@ -1629,8 +1629,8 @@ EQMP { .name = "drive-mirror", - .args_type = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?," - "node-name:s?,replaces:s?," + .args_type = "job-id:s?,sync:s,device:B,target:s,speed:i?,mode:s?," + "format:s?,node-name:s?,replaces:s?," "on-source-error:s?,on-target-error:s?," "unmap:b?," "granularity:i?,buf-size:i?",
@@ -1650,6 +1650,7 @@ of the source. Arguments: +- "job-id": identifier for the newly-created block job (json-string, optional) - "device": device name to operate on (json-string) - "target": name of new image file (json-string) - "format": format of new image (json-string, optional)
@@ -1693,7 +1694,7 @@ EQMP { .name = "blockdev-mirror", - .args_type = "sync:s,device:B,target:B,replaces:s?,speed:i?," + .args_type = "job-id:s?,sync:s,device:B,target:B,replaces:s?,speed:i?," "on-source-error:s?,on-target-error:s?," "granularity:i?,buf-size:i?", .mhandler.cmd_new = qmp_marshal_blockdev_mirror,
@@ -1708,6 +1709,7 @@ specifies the target of mirror operation. Arguments: +- "job-id": identifier for the newly-created block job (json-string, optional) - "device": device name to operate on (json-string) - "target": device name to mirror to (json-string) - "replaces": the block driver node name to replace when finished
Signature · 1 line
2.8.1
Re: [Qemu-devel] [PATCH 06/15] mirror: Add 'job-id' parameter to 'blockdev-mirror' and 'drive-mirror'
Quoted 7 lines · Alberto Garcia
This patch adds a new optional 'job-id' parameter to 'blockdev-mirror' and 'drive-mirror', allowing the user to specify the ID of the block job to be created.
The HMP '
drive_mirror' command remains unchanged.Signed-off-by Alberto Garcia <[email protected]>
Quoted 8 lines
--- block/mirror.c | 14 +++++++------- blockdev.c | 15 ++++++++------- hmp.c | 2 +- include/block/block_int.h | 6 ++++-- qapi/block-core.json | 10 +++++++--- qmp-commands.hx | 8 +++++--- 6 files changed, 32 insertions(+), 23 deletions(-)
Re: [Qemu-devel] [PATCH 06/15] mirror: Add 'job-id' parameter to 'blockdev-mirror' and 'drive-mirror'
Quoted 17 lines · Max Reitz
Quoted 7 lines · Alberto Garcia
This patch adds a new optional 'job-id' parameter to 'blockdev-mirror' and 'drive-mirror', allowing the user to specify the ID of the block job to be created.
The HMP '
drive_mirror' command remains unchanged.Signed-off-by Alberto Garcia <[email protected]>Quoted 8 lines
--- block/mirror.c | 14 +++++++------- blockdev.c | 15 ++++++++------- hmp.c | 2 +- include/block/block_int.h | 6 ++++-- qapi/block-core.json | 10 +++++++--- qmp-commands.hx | 8 +++++--- 6 files changed, 32 insertions(+), 23 deletions(-)
Quoted 1 line
Reviewed-by Max Reitz <[email protected]>
Forgot to mention: The patch will need a rebase, but the necessary changes are rather obvious.
Max
Re: [Qemu-devel] [PATCH 06/15] mirror: Add 'job-id' parameter to 'blockdev-mirror' and 'drive-mirror'
In reply to Alberto GarciaQuoted 8 lines · Alberto Garcia
This patch adds a new optional 'job-id' parameter to 'blockdev-mirror' and 'drive-mirror', allowing the user to specify the ID of the block job to be created.
The HMP '
drive_mirror' command remains unchanged.Signed-off-by Alberto Garcia <[email protected]>---
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 7/15 backup: Add 'job-id' parameter to 'blockdev-backup' and 'drive-backup'
In reply to Alberto GarciaThis patch adds a new optional 'job-id' parameter to 'blockdev-backup' and 'drive-backup', allowing the user to specify the ID of the block job to be created.
The HMP 'drive_backup' command remains unchanged.
--- block/backup.c | 8 ++++---- blockdev.c | 43 ++++++++++++++++++++++++------------------- hmp.c | 2 +- include/block/block_int.h | 8 +++++--- qapi/block-core.json | 10 +++++++--- qmp-commands.hx | 8 +++++--- 6 files changed, 46 insertions(+), 33 deletions(-)
block/backup.c +4 -4
@@ -468,9 +468,9 @@ static void coroutine_fn backup_run(void *opaque)block_job_defer_to_main_loop (&job->common, backup_complete, data); } -void backup_start(BlockDriverState *bs, BlockDriverState *target, - int64_t speed, MirrorSyncMode sync_mode, - BdrvDirtyBitmap *sync_bitmap, +void backup_start(const char *job_id, BlockDriverState *bs, + BlockDriverState *target, int64_t speed, + MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap, BlockdevOnError on_source_error, BlockdevOnError on_target_error, BlockCompletionFunc *cb, void *opaque,
@@ -536,7 +536,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, goto error; } - job =block_job_create (NULL , &backup_job_driver, bs, speed, + job =block_job_create (job_id, &backup_job_driver, bs, speed, cb, opaque, errp);if (!job) { goto error;
blockdev.c +24 -19
115 lines not on this page · raw message
hmp.c +1 -1
@@ -1123,7 +1123,7 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict) mode =NEW_IMAGE_MODE_ABSOLUTE_PATHS ; } -qmp_drive_backup (device, filename, !!format, format, +qmp_drive_backup (false,NULL , device, filename, !!format, format, full ?MIRROR_SYNC_MODE_FULL :MIRROR_SYNC_MODE_TOP , true, mode, false,0 , false,NULL , false,0 , false,0 , &err);
include/block/block_int.h +5 -3
@@ -698,6 +698,8 @@ void mirror_start(const char *job_id, BlockDriverState *bs,/* * backup_start: +* @job_id: The id of the newly-created job, or %NULL to have one +* generated automatically. * @bs: Block device to operate on. * @target: Block device to write to. * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
@@ -712,9 +714,9 @@ void mirror_start(const char *job_id, BlockDriverState *bs,* Start a backup operation on @bs. Clusters in @bs are written to @target * until the job is cancelled or manually completed. */ -void backup_start(BlockDriverState *bs, BlockDriverState *target, - int64_t speed, MirrorSyncMode sync_mode, - BdrvDirtyBitmap *sync_bitmap, +void backup_start(const char *job_id, BlockDriverState *bs, + BlockDriverState *target, int64_t speed, + MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap, BlockdevOnError on_source_error, BlockdevOnError on_target_error, BlockCompletionFunc *cb, void *opaque,
qapi/block-core.json +7 -3
@@ -866,6 +866,8 @@ ## # @DriveBackup # +# @job-id: #optional identifier for the newly-created block job (Since 2.7) +# # @device: the name of the device which should be copied. # # @target: the target of the new image. If the file exists, or if it
@@ -903,8 +905,8 @@ # Since: 1.6 ## { 'struct': 'DriveBackup', - 'data': { 'device': 'str', 'target': 'str', '*format': 'str', - 'sync': 'MirrorSyncMode', '*mode': 'NewImageMode', + 'data': { '*job-id': 'str', 'device': 'str', 'target': 'str', + '*format': 'str', 'sync': 'MirrorSyncMode', '*mode': 'NewImageMode', '*speed': 'int', '*bitmap': 'str', '*on-source-error': 'BlockdevOnError', '*on-target-error': 'BlockdevOnError' } }
@@ -912,6 +914,8 @@ ## # @BlockdevBackup # +# @job-id: #optional identifier for the newly-created block job (Since 2.7) +# # @device: the name of the device which should be copied. # # @target: the name of the backup target device.
@@ -938,7 +942,7 @@ # Since: 2.3 ## { 'struct': 'BlockdevBackup', - 'data': { 'device': 'str', 'target': 'str', + 'data': { '*job-id': 'str', 'device': 'str', 'target': 'str', 'sync': 'MirrorSyncMode', '*speed': 'int', '*on-source-error': 'BlockdevOnError',
qmp-commands.hx +5 -3
@@ -1185,8 +1185,8 @@ EQMP { .name = "drive-backup", - .args_type = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?," - "bitmap:s?,on-source-error:s?,on-target-error:s?", + .args_type = "job-id:s?,sync:s,device:B,target:s,speed:i?,mode:s?," + "format:s?,bitmap:s?,on-source-error:s?,on-target-error:s?", .mhandler.cmd_new = qmp_marshal_drive_backup, },
@@ -1202,6 +1202,7 @@ block-job-cancel command. Arguments: +- "job-id": identifier for the newly-created block job (json-string, optional) - "device": the name of the device which should be copied. (json-string) - "target": the target of the new image. If the file exists, or if it is a
@@ -1239,7 +1240,7 @@ EQMP { .name = "blockdev-backup", - .args_type = "sync:s,device:B,target:B,speed:i?," + .args_type = "job-id:s?,sync:s,device:B,target:B,speed:i?," "on-source-error:s?,on-target-error:s?", .mhandler.cmd_new = qmp_marshal_blockdev_backup, },
@@ -1253,6 +1254,7 @@ as backup target. Arguments: +- "job-id": identifier for the newly-created block job (json-string, optional) - "device": the name of the device which should be copied. (json-string) - "target": the name of the backup target device. (json-string)
Signature · 1 line
2.8.1
Re: [Qemu-devel] [PATCH 07/15] backup: Add 'job-id' parameter to 'blockdev-backup' and 'drive-backup'
Quoted 7 lines · Alberto Garcia
This patch adds a new optional 'job-id' parameter to 'blockdev-backup' and 'drive-backup', allowing the user to specify the ID of the block job to be created.
The HMP '
drive_backup' command remains unchanged.Signed-off-by Alberto Garcia <[email protected]>
Quoted 8 lines
--- block/backup.c | 8 ++++---- blockdev.c | 43 ++++++++++++++++++++++++------------------- hmp.c | 2 +- include/block/block_int.h | 8 +++++--- qapi/block-core.json | 10 +++++++--- qmp-commands.hx | 8 +++++--- 6 files changed, 46 insertions(+), 33 deletions(-)
PATCH 8/15 stream: Add 'job-id' parameter to 'block-stream'
In reply to Alberto GarciaThis patch adds a new optional 'job-id' parameter to 'block-stream', allowing the user to specify the ID of the block job to be created.
The HMP 'block_stream' command remains unchanged.
--- block/stream.c | 12 ++++++------ blockdev.c | 6 +++--- hmp.c | 2 +- include/block/block_int.h | 10 ++++++---- qapi/block-core.json | 7 +++++-- qmp-commands.hx | 3 ++- 6 files changed, 23 insertions(+), 17 deletions(-)
block/stream.c +6 -6
@@ -218,15 +218,15 @@ static const BlockJobDriver stream_job_driver = { .set_speed = stream_set_speed, }; -void stream_start (BlockDriverState *bs,BlockDriverState *base, -const char *backing_file_str,int64_t speed, -BlockdevOnError on_error, -BlockCompletionFunc *cb, -void *opaque,Error **errp) +void stream_start (const char *job_id,BlockDriverState *bs, +BlockDriverState *base,const char *backing_file_str, +int64_t speed,BlockdevOnError on_error, +BlockCompletionFunc *cb,void *opaque,Error **errp) {StreamBlockJob *s; - s =block_job_create (NULL , &stream_job_driver, bs, speed, cb, opaque, errp); + s =block_job_create (job_id, &stream_job_driver, bs, speed, + cb, opaque, errp);if (!s) {return ; }
blockdev.c +3 -3
@@ -2998,7 +2998,7 @@ static void block_job_cb(void *opaque, int ret) } } -void qmp_block_stream(const char *device, +void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, bool has_base, const char *base, bool has_backing_file, const char *backing_file, bool has_speed, int64_t speed,
@@ -3057,8 +3057,8 @@ void qmp_block_stream(const char *device,/* backing_file string overrides base bs filename */ base_name = has_backing_file ? backing_file : base_name; -stream_start (bs, base_bs, base_name, has_speed ? speed :0 , - on_error, block_job_cb, bs, &local_err); +stream_start (has_job_id ? job_id :NULL , bs, base_bs, base_name, + has_speed ? speed :0 , on_error, block_job_cb, bs, &local_err);if (local_err) {error_propagate (errp, local_err); goto out;
hmp.c +1 -1
@@ -1481,7 +1481,7 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)const char *base =qdict_get_try_str (qdict,"base" );int64_t speed =qdict_get_try_int (qdict,"speed" ,0 ); -qmp_block_stream (device, base !=NULL , base, false,NULL , +qmp_block_stream (false,NULL , device, base !=NULL , base, false,NULL ,qdict_haskey (qdict,"speed" ), speed, true,BLOCKDEV_ON_ERROR_REPORT , &error);
include/block/block_int.h +6 -4
@@ -607,6 +607,8 @@ int is_windows_drive(const char *filename);/** * stream_start: +* @job_id: The id of the newly-created job, or %NULL to have one +* generated automatically. * @bs: Block device to operate on. * @base: Block device that will become the new base, or %NULL to * flatten the whole backing file chain onto @bs.
@@ -625,10 +627,10 @@ int is_windows_drive(const char *filename);* @backing_file_str in the written image and to @base in the live * BlockDriverState. */ -void stream_start (BlockDriverState *bs,BlockDriverState *base, -const char *backing_file_str,int64_t speed, -BlockdevOnError on_error,BlockCompletionFunc *cb, -void *opaque,Error **errp); +void stream_start (const char *job_id,BlockDriverState *bs, +BlockDriverState *base,const char *backing_file_str, +int64_t speed,BlockdevOnError on_error, +BlockCompletionFunc *cb,void *opaque,Error **errp);/** * commit_start:
qapi/block-core.json +5 -2
@@ -1421,6 +1421,8 @@ # On successful completion the image file is updated to drop the backing file # and the BLOCK_JOB_COMPLETED event is emitted. # +# @job-id: #optional identifier for the newly-created block job (Since 2.7) +# # @device: the device name # # @base: #optional the common backing file name
@@ -1452,8 +1454,9 @@ # Since: 1.1 ## { 'command': 'block-stream', - 'data': { 'device': 'str', '*base': 'str', '*backing-file': 'str', - '*speed': 'int', '*on-error': 'BlockdevOnError' } } + 'data': { '*job-id': 'str', 'device': 'str', '*base': 'str', + '*backing-file': 'str', '*speed': 'int', + '*on-error': 'BlockdevOnError' } } ## # @block-job-set-speed:
qmp-commands.hx +2 -1
@@ -1079,7 +1079,7 @@ EQMP { .name = "block-stream", - .args_type = "device:B,base:s?,speed:o?,backing-file:s?,on-error:s?", + .args_type = "job-id:s?,device:B,base:s?,speed:o?,backing-file:s?,on-error:s?", .mhandler.cmd_new = qmp_marshal_block_stream, },
@@ -1091,6 +1091,7 @@ Copy data from a backing file into a block device. Arguments: +- "job-id": identifier for the newly-created block job (json-string, optional) - "device": The device's ID, must be unique (json-string) - "base": The file name of the backing image above which copying starts (json-string, optional)
Signature · 1 line
2.8.1
Re: [Qemu-devel] [PATCH 08/15] stream: Add 'job-id' parameter to 'block-stream'
Quoted 6 lines · Alberto Garcia
This patch adds a new optional 'job-id' parameter to 'block-stream', allowing the user to specify the ID of the block job to be created.
The HMP '
block_stream' command remains unchanged.Signed-off-by Alberto Garcia <[email protected]>
Quoted 8 lines
--- block/stream.c | 12 ++++++------ blockdev.c | 6 +++--- hmp.c | 2 +- include/block/block_int.h | 10 ++++++---- qapi/block-core.json | 7 +++++-- qmp-commands.hx | 3 ++- 6 files changed, 23 insertions(+), 17 deletions(-)
PATCH 9/15 stream: Add 'job-id' parameter to 'block-commit'
In reply to Alberto GarciaThis patch adds a new optional 'job-id' parameter to 'block-commit', allowing the user to specify the ID of the block job to be created.
--- block/commit.c | 7 ++++--- block/mirror.c | 6 +++--- blockdev.c | 9 +++++---- include/block/block_int.h | 16 ++++++++++------ qapi/block-core.json | 4 +++- qemu-img.c | 2 +- qmp-commands.hx | 3 ++- 7 files changed, 28 insertions(+), 19 deletions(-)
block/commit.c +4 -3
@@ -211,8 +211,8 @@ static const BlockJobDriver commit_job_driver = { .set_speed = commit_set_speed, }; -void commit_start (BlockDriverState *bs,BlockDriverState *base, -BlockDriverState *top,int64_t speed, +void commit_start (const char *job_id,BlockDriverState *bs, +BlockDriverState *base,BlockDriverState *top,int64_t speed,BlockdevOnError on_error,BlockCompletionFunc *cb,void *opaque,const char *backing_file_str,Error **errp) {
@@ -236,7 +236,8 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,return ; } - s =block_job_create (NULL , &commit_job_driver, bs, speed, cb, opaque, errp); + s =block_job_create (job_id, &commit_job_driver, bs, speed, + cb, opaque, errp);if (!s) {return ; }
block/mirror.c +3 -3
@@ -880,8 +880,8 @@ void mirror_start(const char *job_id, BlockDriverState *bs, &mirror_job_driver, is_none_mode, base); } -void commit_active_start (BlockDriverState *bs,BlockDriverState *base, -int64_t speed, +void commit_active_start (const char *job_id,BlockDriverState *bs, +BlockDriverState *base,int64_t speed,BlockdevOnError on_error,BlockCompletionFunc *cb,void *opaque,Error **errp)
@@ -922,7 +922,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, } } -mirror_start_job (NULL , bs, base,NULL , speed,0 ,0 , +mirror_start_job (job_id, bs, base,NULL , speed,0 ,0 , on_error, on_error, false, cb, opaque, &local_err, &commit_active_job_driver, false, base);if (local_err) {
blockdev.c +5 -4
@@ -3070,7 +3070,7 @@ out:aio_context_release (aio_context); } -void qmp_block_commit(const char *device, +void qmp_block_commit(bool has_job_id, const char *job_id, const char *device, bool has_base, const char *base, bool has_top, const char *top, bool has_backing_file, const char *backing_file,
@@ -3161,10 +3161,11 @@ void qmp_block_commit(const char *device, " but 'top' is the active layer"); goto out; } -commit_active_start (bs, base_bs, speed, on_error, block_job_cb, - bs, &local_err); +commit_active_start (has_job_id ? job_id :NULL , bs, base_bs, speed, + on_error, block_job_cb, bs, &local_err); }else { -commit_start (bs, base_bs, top_bs, speed, on_error, block_job_cb, bs, +commit_start (has_job_id ? job_id :NULL , bs, base_bs, top_bs, speed, + on_error, block_job_cb, bs, has_backing_file ? backing_file :NULL , &local_err); }if (local_err !=NULL ) {
include/block/block_int.h +10 -6
@@ -634,6 +634,8 @@ void stream_start(const char *job_id, BlockDriverState *bs,/** * commit_start: +* @job_id: The id of the newly-created job, or %NULL to have one +* generated automatically. * @bs: Active block device. * @top: Top block device to be committed. * @base: Block device that will be written into, and become the new top.
@@ -645,12 +647,14 @@ void stream_start(const char *job_id, BlockDriverState *bs,* @errp: Error object. * */ -void commit_start (BlockDriverState *bs,BlockDriverState *base, -BlockDriverState *top,int64_t speed, -BlockdevOnError on_error,BlockCompletionFunc *cb, -void *opaque,const char *backing_file_str,Error **errp); +void commit_start (const char *job_id,BlockDriverState *bs, +BlockDriverState *base,BlockDriverState *top,int64_t speed, +BlockdevOnError on_error,BlockCompletionFunc *cb, +void *opaque,const char *backing_file_str,Error **errp);/** * commit_active_start: +* @job_id: The id of the newly-created job, or %NULL to have one +* generated automatically. * @bs: Active block device to be committed. * @base: Block device that will be written into, and become the new top. * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
@@ -660,8 +664,8 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,* @errp: Error object. * */ -void commit_active_start (BlockDriverState *bs,BlockDriverState *base, -int64_t speed, +void commit_active_start (const char *job_id,BlockDriverState *bs, +BlockDriverState *base,int64_t speed,BlockdevOnError on_error,BlockCompletionFunc *cb,void *opaque,Error **errp);
qapi/block-core.json +3 -1
@@ -1008,6 +1008,8 @@ # Live commit of data from overlay image nodes into backing nodes - i.e., # writes data between 'top' and 'base' into 'base'. # +# @job-id: #optional identifier for the newly-created block job (Since 2.7) +# # @device: the name of the device # # @base: #optional The file name of the backing image to write data into.
@@ -1059,7 +1061,7 @@ # ## { 'command': 'block-commit', - 'data': { 'device': 'str', '*base': 'str', '*top': 'str', + 'data': { '*job-id': 'str', 'device': 'str', '*base': 'str', '*top': 'str', '*backing-file': 'str', '*speed': 'int' } } ##
qemu-img.c +1 -1
@@ -910,7 +910,7 @@ static int img_commit(int argc, char **argv) .bs = bs, }; -commit_active_start (bs, base_bs,0 ,BLOCKDEV_ON_ERROR_REPORT , +commit_active_start (NULL , bs, base_bs,0 ,BLOCKDEV_ON_ERROR_REPORT , common_block_job_cb, &cbi, &local_err);if (local_err) { goto done;
qmp-commands.hx +2 -1
@@ -1123,7 +1123,7 @@ EQMP { .name = "block-commit", - .args_type = "device:B,base:s?,top:s?,backing-file:s?,speed:o?", + .args_type = "job-id:s?,device:B,base:s?,top:s?,backing-file:s?,speed:o?", .mhandler.cmd_new = qmp_marshal_block_commit, },
@@ -1136,6 +1136,7 @@ data between 'top' and 'base' into 'base'. Arguments: +- "job-id": identifier for the newly-created block job (json-string, optional) - "device": The device's ID, must be unique (json-string) - "base": The file name of the backing image to write data into. If not specified, this is the deepest backing image
Signature · 1 line
2.8.1
Re: [Qemu-devel] [PATCH 09/15] stream: Add 'job-id' parameter to 'block-commit'
Quoted 4 lines · Alberto Garcia
This patch adds a new optional 'job-id' parameter to 'block-commit', allowing the user to specify the ID of the block job to be created.
Signed-off-by Alberto Garcia <[email protected]>
Quoted 9 lines
--- block/commit.c | 7 ++++--- block/mirror.c | 6 +++--- blockdev.c | 9 +++++---- include/block/block_int.h | 16 ++++++++++------ qapi/block-core.json | 4 +++- qemu-img.c | 2 +- qmp-commands.hx | 3 ++- 7 files changed, 28 insertions(+), 19 deletions(-)
With the commit message fixed to read s/^stream/commit/:
PATCH 10/15 blockjob: Add 'id' parameter to 'block-job-set-speed'
In reply to Alberto GarciaThis patch allows the 'block-job-set-speed' command to identify the job by either its ID or its device name. The latter becomes now optional since the ID alone is enough to identify the job.
The HMP 'block_job_set_speed' command remains unchanged.
--- blockdev.c | 7 +++++-- hmp.c | 2 +- qapi/block-core.json | 9 +++++++-- qmp-commands.hx | 2 +- 4 files changed, 14 insertions(+), 6 deletions(-)
blockdev.c +5 -2
@@ -3738,10 +3738,13 @@ static BlockJob *find_block_job(const char *id, const char *device,return job; } -void qmp_block_job_set_speed (const char *device,int64_t speed,Error **errp) +void qmp_block_job_set_speed (bool has_id,const char *id,bool has_device, +const char *device,int64_t speed,Error **errp) {AioContext *aio_context; -BlockJob *job =find_block_job (NULL , device, &aio_context, errp); +BlockJob *job =find_block_job (has_id ? id :NULL , + has_device ? device :NULL , + &aio_context, errp);if (!job) {return ;
hmp.c +1 -1
@@ -1494,7 +1494,7 @@ void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict)const char *device =qdict_get_str (qdict,"device" );int64_t value =qdict_get_int (qdict,"speed" ); -qmp_block_job_set_speed (device, value, &error); +qmp_block_job_set_speed (false,NULL , true, device, value, &error);hmp_handle_error (mon, &error); }
qapi/block-core.json +7 -2
@@ -1469,7 +1469,12 @@ # # Throttling can be disabled by setting the speed to 0. # -# @device: the device name +# The job must be identified with the @id or @device parameters, but +# only one of them must be set. +# +# @id: #optional the job identifier. (Since 2.7) +# +# @device: #optional the device name. # # @speed: the maximum speed, in bytes per second, or 0 for unlimited. # Defaults to 0.
@@ -1480,7 +1485,7 @@ # Since: 1.1 ## { 'command': 'block-job-set-speed', - 'data': { 'device': 'str', 'speed': 'int' } } + 'data': { '*id': 'str', '*device': 'str', 'speed': 'int' } } ## # @block-job-cancel:
qmp-commands.hx +1 -1
@@ -1284,7 +1284,7 @@ EQMP { .name = "block-job-set-speed", - .args_type = "device:B,speed:o", + .args_type = "id:s?,device:B?,speed:o", .mhandler.cmd_new = qmp_marshal_block_job_set_speed, },
Signature · 1 line
2.8.1
Re: [Qemu-devel] [PATCH 10/15] blockjob: Add 'id' parameter to 'block-job-set-speed'
Quoted 6 lines · Alberto Garcia
This patch allows the 'block-job-set-speed' command to identify the job by either its ID or its device name. The latter becomes now optional since the ID alone is enough to identify the job.
The HMP '
block_job_set_speed' command remains unchanged.
Quoted 7 lines
Signed-off-by Alberto Garcia <[email protected]>--- blockdev.c | 7 +++++-- hmp.c | 2 +- qapi/block-core.json | 9 +++++++-- qmp-commands.hx | 2 +- 4 files changed, 14 insertions(+), 6 deletions(-)
PATCH 11/15 blockjob: Add 'id' parameter to 'block-job-cancel'
In reply to Alberto GarciaThis patch allows the 'block-job-cancel' command to identify the job by either its ID or its device name. The latter becomes now optional since the ID alone is enough to identify the job.
The HMP 'block_job_cancel' command remains unchanged.
--- blockdev.c | 7 +++++-- hmp.c | 2 +- qapi/block-core.json | 10 ++++++++-- qmp-commands.hx | 2 +- 4 files changed, 15 insertions(+), 6 deletions(-)
blockdev.c +5 -2
@@ -3754,11 +3754,14 @@ void qmp_block_job_set_speed(bool has_id, const char *id, bool has_device,aio_context_release (aio_context); } -void qmp_block_job_cancel (const char *device, +void qmp_block_job_cancel (bool has_id,const char *id, +bool has_device,const char *device,bool has_force,bool force,Error **errp) {AioContext *aio_context; -BlockJob *job =find_block_job (NULL , device, &aio_context, errp); +BlockJob *job =find_block_job (has_id ? id :NULL , + has_device ? device :NULL , + &aio_context, errp);if (!job) {return ;
hmp.c +1 -1
@@ -1505,7 +1505,7 @@ void hmp_block_job_cancel(Monitor *mon, const QDict *qdict)const char *device =qdict_get_str (qdict,"device" );bool force =qdict_get_try_bool (qdict,"force" , false); -qmp_block_job_cancel (device, true, force, &error); +qmp_block_job_cancel (false,NULL , true, device, true, force, &error);hmp_handle_error (mon, &error); }
qapi/block-core.json +8 -2
@@ -1505,7 +1505,12 @@ # operation can be started at a later time to finish copying all data from the # backing file. # -# @device: the device name +# The job must be identified with the @id or @device parameters, but +# only one of them must be set. +# +# @id: #optional the job identifier (Since 2.7) +# +# @device: #optional the device name # # @force: #optional whether to allow cancellation of a paused job (default # false). Since 1.3.
@@ -1515,7 +1520,8 @@ # # Since: 1.1 ## -{ 'command': 'block-job-cancel', 'data': { 'device': 'str', '*force': 'bool' } } +{ 'command': 'block-job-cancel', + 'data': { '*id': 'str', '*device': 'str', '*force': 'bool' } } ## # @block-job-pause:
qmp-commands.hx +1 -1
@@ -1290,7 +1290,7 @@ EQMP { .name = "block-job-cancel", - .args_type = "device:B,force:b?", + .args_type = "id:s?,device:B?,force:b?", .mhandler.cmd_new = qmp_marshal_block_job_cancel, }, {
Signature · 1 line
2.8.1
Re: [Qemu-devel] [PATCH 11/15] blockjob: Add 'id' parameter to 'block-job-cancel'
Quoted 6 lines · Alberto Garcia
This patch allows the 'block-job-cancel' command to identify the job by either its ID or its device name. The latter becomes now optional since the ID alone is enough to identify the job.
The HMP '
block_job_cancel' command remains unchanged.
Quoted 7 lines
Signed-off-by Alberto Garcia <[email protected]>--- blockdev.c | 7 +++++-- hmp.c | 2 +- qapi/block-core.json | 10 ++++++++-- qmp-commands.hx | 2 +- 4 files changed, 15 insertions(+), 6 deletions(-)
PATCH 12/15 blockjob: Add 'id' parameter to 'block-job-pause'
In reply to Alberto GarciaThis patch allows the 'block-job-pause' command to identify the job by either its ID or its device name. The latter becomes now optional since the ID alone is enough to identify the job.
The HMP 'block_job_pause' command remains unchanged.
--- blockdev.c | 7 +++++-- hmp.c | 2 +- qapi/block-core.json | 9 +++++++-- qmp-commands.hx | 2 +- 4 files changed, 14 insertions(+), 6 deletions(-)
blockdev.c +5 -2
@@ -3783,10 +3783,13 @@ out:aio_context_release (aio_context); } -void qmp_block_job_pause (const char *device,Error **errp) +void qmp_block_job_pause (bool has_id,const char *id, +bool has_device,const char *device,Error **errp) {AioContext *aio_context; -BlockJob *job =find_block_job (NULL , device, &aio_context, errp); +BlockJob *job =find_block_job (has_id ? id :NULL , + has_device ? device :NULL , + &aio_context, errp);if (!job || job->user_paused) {return ;
hmp.c +1 -1
@@ -1515,7 +1515,7 @@ void hmp_block_job_pause(Monitor *mon, const QDict *qdict)Error *error =NULL ;const char *device =qdict_get_str (qdict,"device" ); -qmp_block_job_pause (device, &error); +qmp_block_job_pause (false,NULL , true, device, &error);hmp_handle_error (mon, &error); }
qapi/block-core.json +7 -2
@@ -1537,14 +1537,19 @@ # the operation is actually paused. Cancelling a paused job automatically # resumes it. # -# @device: the device name +# The job must be identified with the @id or @device parameters, but +# only one of them must be set. +# +# @id: #optional the job identifier. (Since 2.7) +# +# @device: #optional the device name # # Returns: Nothing on success # If no background operation is active on this device, DeviceNotActive # # Since: 1.3 ## -{ 'command': 'block-job-pause', 'data': { 'device': 'str' } } +{ 'command': 'block-job-pause', 'data': { '*id': 'str', '*device': 'str' } } ## # @block-job-resume:
qmp-commands.hx +1 -1
@@ -1295,7 +1295,7 @@ EQMP }, { .name = "block-job-pause", - .args_type = "device:B", + .args_type = "id:s?,device:B?", .mhandler.cmd_new = qmp_marshal_block_job_pause, }, {
Signature · 1 line
2.8.1
Re: [Qemu-devel] [PATCH 12/15] blockjob: Add 'id' parameter to 'block-job-pause'
Quoted 6 lines · Alberto Garcia
This patch allows the 'block-job-pause' command to identify the job by either its ID or its device name. The latter becomes now optional since the ID alone is enough to identify the job.
The HMP '
block_job_pause' command remains unchanged.
Quoted 7 lines
Signed-off-by Alberto Garcia <[email protected]>--- blockdev.c | 7 +++++-- hmp.c | 2 +- qapi/block-core.json | 9 +++++++-- qmp-commands.hx | 2 +- 4 files changed, 14 insertions(+), 6 deletions(-)
PATCH 13/15 blockjob: Add 'id' parameter to 'block-job-resume'
In reply to Alberto GarciaThis patch allows the 'block-job-resume' command to identify the job by either its ID or its device name. The latter becomes now optional since the ID alone is enough to identify the job.
The HMP 'block_job_resume' command remains unchanged.
--- blockdev.c | 7 +++++-- hmp.c | 2 +- qapi/block-core.json | 7 ++++++- qmp-commands.hx | 2 +- 4 files changed, 13 insertions(+), 5 deletions(-)
blockdev.c +5 -2
@@ -3801,10 +3801,13 @@ void qmp_block_job_pause(bool has_id, const char *id,aio_context_release (aio_context); } -void qmp_block_job_resume (const char *device,Error **errp) +void qmp_block_job_resume (bool has_id,const char *id, +bool has_device,const char *device,Error **errp) {AioContext *aio_context; -BlockJob *job =find_block_job (NULL , device, &aio_context, errp); +BlockJob *job =find_block_job (has_id ? id :NULL , + has_device ? device :NULL , + &aio_context, errp);if (!job || !job->user_paused) {return ;
hmp.c +1 -1
@@ -1525,7 +1525,7 @@ void hmp_block_job_resume(Monitor *mon, const QDict *qdict)Error *error =NULL ;const char *device =qdict_get_str (qdict,"device" ); -qmp_block_job_resume (device, &error); +qmp_block_job_resume (false,NULL , true, device, &error);hmp_handle_error (mon, &error); }
qapi/block-core.json +6 -1
@@ -1562,6 +1562,11 @@ # # This command also clears the error status of the job. # +# The job must be identified with the @id or @device parameters, but +# only one of them must be set. +# +# @id: #optional the job identifier. (Since 2.7) +# # @device: the device name # # Returns: Nothing on success
@@ -1569,7 +1574,7 @@ # # Since: 1.3 ## -{ 'command': 'block-job-resume', 'data': { 'device': 'str' } } +{ 'command': 'block-job-resume', 'data': { '*id': 'str', '*device': 'str' } } ## # @block-job-complete:
qmp-commands.hx +1 -1
@@ -1300,7 +1300,7 @@ EQMP }, { .name = "block-job-resume", - .args_type = "device:B", + .args_type = "id:s?,device:B?", .mhandler.cmd_new = qmp_marshal_block_job_resume, }, {
Signature · 1 line
2.8.1
Re: [Qemu-devel] [PATCH 13/15] blockjob: Add 'id' parameter to 'block-job-resume'
Quoted 6 lines · Alberto Garcia
This patch allows the 'block-job-resume' command to identify the job by either its ID or its device name. The latter becomes now optional since the ID alone is enough to identify the job.
The HMP '
block_job_resume' command remains unchanged.
Quoted 7 lines
Signed-off-by Alberto Garcia <[email protected]>--- blockdev.c | 7 +++++-- hmp.c | 2 +- qapi/block-core.json | 7 ++++++- qmp-commands.hx | 2 +- 4 files changed, 13 insertions(+), 5 deletions(-)
PATCH 14/15 blockjob: Add 'id' parameter to 'block-job-complete'
In reply to Alberto GarciaThis patch allows the 'block-job-complete' command to identify the job by either its ID or its device name. The latter becomes now optional since the ID alone is enough to identify the job.
The HMP 'block_job_complete' command remains unchanged.
--- blockdev.c | 7 +++++-- hmp.c | 2 +- qapi/block-core.json | 7 ++++++- qmp-commands.hx | 2 +- 4 files changed, 13 insertions(+), 5 deletions(-)
blockdev.c +5 -2
@@ -3819,10 +3819,13 @@ void qmp_block_job_resume(bool has_id, const char *id,aio_context_release (aio_context); } -void qmp_block_job_complete (const char *device,Error **errp) +void qmp_block_job_complete (bool has_id,const char *id, +bool has_device,const char *device,Error **errp) {AioContext *aio_context; -BlockJob *job =find_block_job (NULL , device, &aio_context, errp); +BlockJob *job =find_block_job (has_id ? id :NULL , + has_device ? device :NULL , + &aio_context, errp);if (!job) {return ;
hmp.c +1 -1
@@ -1535,7 +1535,7 @@ void hmp_block_job_complete(Monitor *mon, const QDict *qdict)Error *error =NULL ;const char *device =qdict_get_str (qdict,"device" ); -qmp_block_job_complete (device, &error); +qmp_block_job_complete (false,NULL , true, device, &error);hmp_handle_error (mon, &error); }
qapi/block-core.json +6 -1
@@ -1593,6 +1593,11 @@ # # A cancelled or paused job cannot be completed. # +# The job must be identified with the @id or @device parameters, but +# only one of them must be set. +# +# @id: #optional the job identifier. (Since 2.7) +# # @device: the device name # # Returns: Nothing on success
@@ -1600,7 +1605,7 @@ # # Since: 1.3 ## -{ 'command': 'block-job-complete', 'data': { 'device': 'str' } } +{ 'command': 'block-job-complete', 'data': { '*id': 'str', '*device': 'str' } } ## # @BlockdevDiscardOptions
qmp-commands.hx +1 -1
@@ -1305,7 +1305,7 @@ EQMP }, { .name = "block-job-complete", - .args_type = "device:B", + .args_type = "id:s?,device:B?", .mhandler.cmd_new = qmp_marshal_block_job_complete, }, {
Signature · 1 line
2.8.1
Re: [Qemu-devel] [PATCH 14/15] blockjob: Add 'id' parameter to 'block-job-complete'
Quoted 6 lines · Alberto Garcia
This patch allows the 'block-job-complete' command to identify the job by either its ID or its device name. The latter becomes now optional since the ID alone is enough to identify the job.
The HMP '
block_job_complete' command remains unchanged.
Quoted 7 lines
Signed-off-by Alberto Garcia <[email protected]>--- blockdev.c | 7 +++++-- hmp.c | 2 +- qapi/block-core.json | 7 ++++++- qmp-commands.hx | 2 +- 4 files changed, 13 insertions(+), 5 deletions(-)
PATCH 15/15 blockjob: Add 'id' field to 'BlockJobInfo' and all BLOCK_JOB_* events
In reply to Alberto GarciaNow that all jobs have an ID and all QMP commands that create and operate on block jobs can specify one we can finally expose the ID in all BLOCK_JOB_* events and the BlockJobInfo structure.
This modifies the output of several iotests, but now we have full control of the job IDs so this patch updates the affected ones.
--- blockjob.c | 6 +++++- docs/qmp-events.txt | 4 ++++ qapi/block-core.json | 18 ++++++++++++++++-- tests/qemu-iotests/095 | 2 +- tests/qemu-iotests/095.out | 2 +- tests/qemu-iotests/124 | 3 ++- tests/qemu-iotests/141 | 6 +++++- tests/qemu-iotests/141.out | 14 +++++++------- tests/qemu-iotests/144 | 1 + tests/qemu-iotests/144.out | 4 ++-- 10 files changed, 44 insertions(+), 16 deletions(-)
blockjob.c +5 -1
@@ -416,6 +416,7 @@ BlockJobInfo *block_job_query(BlockJob *job) {BlockJobInfo *info =g_new0 (BlockJobInfo,1 ); info->type =g_strdup (BlockJobType_lookup[job->driver->job_type]); + info->id =g_strdup (job->id); info->device =g_strdup (job->device); info->len = job->len; info->busy = job->busy;
@@ -438,6 +439,7 @@ static void block_job_iostatus_set_err(BlockJob *job, int error)void block_job_event_cancelled (BlockJob *job) { qapi_event_send_block_job_cancelled(job->driver->job_type, + job->id, job->device, job->len, job->offset,
@@ -448,6 +450,7 @@ void block_job_event_cancelled(BlockJob *job)void block_job_event_completed (BlockJob *job,const char *msg) { qapi_event_send_block_job_completed(job->driver->job_type, + job->id, job->device, job->len, job->offset,
@@ -462,6 +465,7 @@ void block_job_event_ready(BlockJob *job) job->ready = true; qapi_event_send_block_job_ready(job->driver->job_type, + job->id, job->device, job->len, job->offset,
@@ -490,7 +494,7 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err,default :abort (); } -qapi_event_send_block_job_error (job->device, +qapi_event_send_block_job_error (job->id, job->device, is_read ?IO_OPERATION_TYPE_READ :IO_OPERATION_TYPE_WRITE , action, &error_abort);
docs/qmp-events.txt +4 -0
@@ -92,6 +92,7 @@ Data: - "type": Job type (json-string; "stream" for image streaming "commit" for block commit) +- "id": Job identifier (json-string) - "device": Device name (json-string) - "len": Maximum progress value (json-int) - "offset": Current progress value (json-int)
@@ -116,6 +117,7 @@ Data: - "type": Job type (json-string; "stream" for image streaming "commit" for block commit) +- "id": Job identifier (json-string) - "device": Device name (json-string) - "len": Maximum progress value (json-int) - "offset": Current progress value (json-int)
@@ -143,6 +145,7 @@ Emitted when a block job encounters an error. Data: +- "id": job identifier (json-string) - "device": device name (json-string) - "operation": I/O operation (json-string, "read" or "write") - "action": action that has been taken, it's one of the following (json-string):
@@ -167,6 +170,7 @@ Data: - "type": Job type (json-string; "stream" for image streaming "commit" for block commit) +- "id": Job identifier (json-string) - "device": Device name (json-string) - "len": Maximum progress value (json-int) - "offset": Current progress value (json-int)
qapi/block-core.json +16 -2
@@ -713,6 +713,8 @@ # # @type: the job type ('stream' for image streaming) # +# @id: the job identifier +# # @device: the block device name # # @len: the maximum progress value
@@ -734,7 +736,7 @@ # Since: 1.1 ## { 'struct': 'BlockJobInfo', - 'data': {'type': 'str', 'device': 'str', 'len': 'int', + 'data': {'type': 'str', 'id': 'str', 'device': 'str', 'len': 'int', 'offset': 'int', 'busy': 'bool', 'paused': 'bool', 'speed': 'int', 'io-status': 'BlockDeviceIoStatus', 'ready': 'bool'} }
@@ -2443,6 +2445,8 @@ # # @type: job type # +# @id: the job identifier +# # @device: device name # # @len: maximum progress value
@@ -2461,6 +2465,7 @@ ## { 'event': 'BLOCK_JOB_COMPLETED', 'data': { 'type' : 'BlockJobType', + 'id' : 'str', 'device': 'str', 'len' : 'int', 'offset': 'int',
@@ -2474,6 +2479,8 @@ # # @type: job type # +# @id: the job identifier +# # @device: device name # # @len: maximum progress value
@@ -2487,6 +2494,7 @@ ## { 'event': 'BLOCK_JOB_CANCELLED', 'data': { 'type' : 'BlockJobType', + 'id' : 'str', 'device': 'str', 'len' : 'int', 'offset': 'int',
@@ -2497,6 +2505,8 @@ # # Emitted when a block job encounters an error # +# @id: the job identifier +# # @device: device name # # @operation: I/O operation
@@ -2506,7 +2516,8 @@ # Since: 1.3 ## { 'event': 'BLOCK_JOB_ERROR', - 'data': { 'device' : 'str', + 'data': { 'id' : 'str', + 'device' : 'str', 'operation': 'IoOperationType', 'action' : 'BlockErrorAction' } }
@@ -2517,6 +2528,8 @@ # # @type: job type # +# @id: the job identifier +# # @device: device name # # @len: maximum progress value
@@ -2533,6 +2546,7 @@ ## { 'event': 'BLOCK_JOB_READY', 'data': { 'type' : 'BlockJobType', + 'id' : 'str', 'device': 'str', 'len' : 'int', 'offset': 'int',
tests/qemu-iotests/095 +1 -1
@@ -71,7 +71,7 @@ h=$QEMU_HANDLE _send_qemu_cmd $h "{ 'execute': 'qmp_capabilities' }" "return" _send_qemu_cmd $h "{ 'execute': 'block-commit', - 'arguments': { 'device': 'test', + 'arguments': { 'job-id': 'testjob', 'device': 'test', 'top': '"${TEST_IMG}.snp1"' } }" "BLOCK_JOB_COMPLETED" _cleanup_qemu
tests/qemu-iotests/095.out +1 -1
@@ -12,7 +12,7 @@ virtual size: 5.0M (5242880 bytes) {"return": {}} {"return": {}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "test", "len": 104857600, "offset": 104857600, "speed": 0, "type": "commit"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "test", "len": 104857600, "offset": 104857600, "speed": 0, "type": "commit", "id": "testjob"}} === Base image info after commit and resize === image: TEST_DIR/t.IMGFMT.base
tests/qemu-iotests/124 +2 -1
@@ -50,7 +50,7 @@ def transaction_bitmap_clear(node, name, **kwargs): def transaction_drive_backup(device, target, **kwargs): return transaction_action('drive-backup', device=device, target=target, - **kwargs) + job_id=device, **kwargs) class Bitmap:
@@ -479,6 +479,7 @@ class TestIncrementalBackup(TestIncrementalBackupBase): self.assertFalse(self.wait_qmp_backup(drive1['id'])) error = self.vm.event_wait('BLOCK_JOB_ERROR') self.assert_qmp(error, 'data', {'device': drive1['id'], + 'id': drive1['id'], 'action': 'report', 'operation': 'read'}) self.assertFalse(self.vm.get_qmp_events(wait=False))
tests/qemu-iotests/141 +5 -1
@@ -102,6 +102,7 @@ echo test_blockjob \ "{'execute': 'drive-backup', 'arguments': {'device': 'drv0', + 'job-id': 'backup1', 'target': '$TEST_DIR/o.$IMGFMT', 'format': '$IMGFMT', 'sync': 'none'}}" \
@@ -118,6 +119,7 @@ echo test_blockjob \ "{'execute': 'drive-mirror', 'arguments': {'device': 'drv0', + 'job-id': 'mirror1', 'target': '$TEST_DIR/o.$IMGFMT', 'format': '$IMGFMT', 'sync': 'none'}}" \
@@ -134,7 +136,7 @@ echo test_blockjob \ "{'execute': 'block-commit', - 'arguments': {'device': 'drv0'}}" \ + 'arguments': {'device': 'drv0', 'job-id': 'commit1'}}" \ 'BLOCK_JOB_READY' \ 'BLOCK_JOB_COMPLETED'
@@ -151,6 +153,7 @@ $QEMU_IO -c 'write 0 1M' "$TEST_DIR/m.$IMGFMT" | _filter_qemu_io test_blockjob \ "{'execute': 'block-commit', 'arguments': {'device': 'drv0', + 'job-id': 'commit2', 'top': '$TEST_DIR/m.$IMGFMT', 'speed': 1}}" \ 'return' \
@@ -173,6 +176,7 @@ $QEMU_IO -c 'write 0 1M' "$TEST_DIR/b.$IMGFMT" | _filter_qemu_io test_blockjob \ "{'execute': 'block-stream', 'arguments': {'device': 'drv0', + 'job-id': 'stream1', 'speed': 1}}" \ 'return' \ 'BLOCK_JOB_CANCELLED'
tests/qemu-iotests/141.out +7 -7
@@ -11,7 +11,7 @@ Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t. {"return": {}} {"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: backup"}} {"return": {}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 1048576, "offset": 0, "speed": 0, "type": "backup"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 1048576, "offset": 0, "speed": 0, "type": "backup", "id": "backup1"}} {"return": {}} === Testing drive-mirror ===
@@ -19,20 +19,20 @@ Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t. {"return": {}} Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT {"return": {}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "mirror"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "mirror", "id": "mirror1"}} {"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: mirror"}} {"return": {}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "mirror"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "mirror", "id": "mirror1"}} {"return": {}} === Testing active block-commit === {"return": {}} {"return": {}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "commit", "id": "commit1"}} {"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: commit"}} {"return": {}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "commit", "id": "commit1"}} {"return": {}} === Testing non-active block-commit ===
@@ -43,7 +43,7 @@ wrote 1048576/1048576 bytes at offset 0 {"return": {}} {"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: commit"}} {"return": {}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 1048576, "offset": 524288, "speed": 1, "type": "commit"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 1048576, "offset": 524288, "speed": 1, "type": "commit", "id": "commit2"}} {"return": {}} === Testing block-stream ===
@@ -54,6 +54,6 @@ wrote 1048576/1048576 bytes at offset 0 {"return": {}} {"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: stream"}} {"return": {}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 1048576, "offset": 524288, "speed": 1, "type": "stream"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 1048576, "offset": 524288, "speed": 1, "type": "stream", "id": "stream1"}} {"return": {}} *** done
tests/qemu-iotests/144 +1 -0
@@ -85,6 +85,7 @@ echo # Block commit on active layer, push the new overlay into base _send_qemu_cmd $h "{ 'execute': 'block-commit', 'arguments': { + 'job-id': 'commit1', 'device': 'virtio0' } }" "READY"
tests/qemu-iotests/144.out +2 -2
@@ -13,9 +13,9 @@ Formatting 'TEST_DIR/tmp.qcow2', fmt=qcow2 size=536870912 backing_file=TEST_DIR/ === Performing block-commit on active layer === {"return": {}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "virtio0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "virtio0", "len": 0, "offset": 0, "speed": 0, "type": "commit", "id": "commit1"}} {"return": {}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "virtio0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "virtio0", "len": 0, "offset": 0, "speed": 0, "type": "commit", "id": "commit1"}} === Performing Live Snapshot 2 ===
Signature · 1 line
2.8.1
Re: [Qemu-devel] [PATCH 15/15] blockjob: Add 'id' field to 'BlockJobInfo' and all BLOCK_JOB_* events
Quoted 8 lines · Alberto Garcia
Now that all jobs have an ID and all QMP commands that create and operate on block jobs can specify one we can finally expose the ID in all BLOCK_JOB_* events and the BlockJobInfo structure.
This modifies the output of several iotests, but now we have full control of the job IDs so this patch updates the affected ones.
Signed-off-by Alberto Garcia <[email protected]>
Quoted 12 lines
--- blockjob.c | 6 +++++- docs/qmp-events.txt | 4 ++++ qapi/block-core.json | 18 ++++++++++++++++-- tests/qemu-iotests/095 | 2 +- tests/qemu-iotests/095.out | 2 +- tests/qemu-iotests/124 | 3 ++- tests/qemu-iotests/141 | 6 +++++- tests/qemu-iotests/141.out | 14 +++++++------- tests/qemu-iotests/144 | 1 + tests/qemu-iotests/144.out | 4 ++-- 10 files changed, 44 insertions(+), 16 deletions(-)
Looks good, but in the meantime iotest 156 has been added which needs the same treatment as the rest.
Also, I personally wouldn't mind making use of the IDs where trivial. For example, you can just replace the "'device': 'virtio0'" by "'id':
'commit1'" in the block-job-complete invocation in 144. Optional, of course, but would make sense.
Max
Re: [Qemu-devel] [PATCH 15/15] blockjob: Add 'id' field to 'BlockJobInfo' and all BLOCK_JOB_* events
Quoted 4 lines · Max Reitz
Also, I personally wouldn't mind making use of the IDs where trivial. For example, you can just replace the "'device': 'virtio0'" by "'id':
'commit1'" in the block-job-complete invocation in 144. Optional, of course, but would make sense.
My plan was to leave this patch with the minimum necessary changes and then write additional tests for the job ID with all possible scenarios.
Berto