[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 10/11] qemu-img: Set the ID of the block job
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v4 10/11] qemu-img: Set the ID of the block job in img_commit() |
Date: |
Tue, 5 Jul 2016 17:56:09 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 |
On 05.07.2016 16:29, Alberto Garcia wrote:
> img_commit() creates a block job without an ID. This is no longer
> allowed now that we require it to be unique and well-formed. We were
> solving this by having a fallback in block_job_create(), but now that
> we extended the API of commit_active_start() we can finally set an
> explicit ID and revert that change.
>
> Signed-off-by: Alberto Garcia <address@hidden>
> Reviewed-by: Max Reitz <address@hidden>
> Reviewed-by: Kevin Wolf <address@hidden>
> ---
> blockjob.c | 6 ------
> qemu-img.c | 2 +-
> 2 files changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/blockjob.c b/blockjob.c
> index 511c0db..3b9cec7 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -132,12 +132,6 @@ void *block_job_create(const char *job_id, const
> BlockJobDriver *driver,
>
> if (job_id == NULL) {
> job_id = bdrv_get_device_name(bs);
> - /* Assign a default ID if the BDS does not have a device
> - * name. We'll get rid of this soon when we finish extending
> - * the API of all commands that create block jobs. */
> - if (job_id[0] == '\0') {
> - job_id = "default_job";
> - }
I stand by my R-b, but as a remark to what you said for v3: I can't
imagine how this function can be called in a way that job_id will be
empty here (after this patch), and this is why I proposed an
assert(job_id[0] != '\0'). It'd probably be a mistake on our part if
such a case could happen (which is why an assertion would be fine).
However, gracefully returning an error is of course fine, too. The issue
I take with this is that the error we'd be returning is "Invalid job ID
''", which isn't very helpful (it should be "No job ID specified, and no
default available").
In any case, since I can't imagine the job_id being empty here and since
we'll handle that case gracefully in case it should occur, I won't
object (and my R-b stands).
Max
> }
>
> if (!id_wellformed(job_id)) {
> diff --git a/qemu-img.c b/qemu-img.c
> index a78f598..521724c 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -921,7 +921,7 @@ static int img_commit(int argc, char **argv)
> .bs = bs,
> };
>
> - commit_active_start(NULL, bs, base_bs, 0, BLOCKDEV_ON_ERROR_REPORT,
> + commit_active_start("commit", bs, base_bs, 0, BLOCKDEV_ON_ERROR_REPORT,
> common_block_job_cb, &cbi, &local_err);
> if (local_err) {
> goto done;
>
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v4 00/11] Allow creating block jobs with a user-defined ID, Alberto Garcia, 2016/07/05
- [Qemu-devel] [PATCH v4 03/11] blockjob: Add block_job_get(), Alberto Garcia, 2016/07/05
- [Qemu-devel] [PATCH v4 08/11] stream: Add 'job-id' parameter to 'block-stream', Alberto Garcia, 2016/07/05
- [Qemu-devel] [PATCH v4 05/11] blockjob: Add 'job_id' parameter to block_job_create(), Alberto Garcia, 2016/07/05
- [Qemu-devel] [PATCH v4 06/11] mirror: Add 'job-id' parameter to 'blockdev-mirror' and 'drive-mirror', Alberto Garcia, 2016/07/05
- [Qemu-devel] [PATCH v4 01/11] stream: Fix prototype of stream_start(), Alberto Garcia, 2016/07/05
- [Qemu-devel] [PATCH v4 10/11] qemu-img: Set the ID of the block job in img_commit(), Alberto Garcia, 2016/07/05
- Re: [Qemu-devel] [PATCH v4 10/11] qemu-img: Set the ID of the block job in img_commit(),
Max Reitz <=
- [Qemu-devel] [PATCH v4 04/11] block: Use block_job_get() in find_block_job(), Alberto Garcia, 2016/07/05
- [Qemu-devel] [PATCH v4 11/11] blockjob: Update description of the 'device' field in the QMP API, Alberto Garcia, 2016/07/05
- [Qemu-devel] [PATCH v4 02/11] blockjob: Update description of the 'id' field, Alberto Garcia, 2016/07/05
- [Qemu-devel] [PATCH v4 09/11] commit: Add 'job-id' parameter to 'block-commit', Alberto Garcia, 2016/07/05
- [Qemu-devel] [PATCH v4 07/11] backup: Add 'job-id' parameter to 'blockdev-backup' and 'drive-backup', Alberto Garcia, 2016/07/05
- Re: [Qemu-devel] [PATCH v4 00/11] Allow creating block jobs with a user-defined ID, Kevin Wolf, 2016/07/07