From patchwork Fri Aug 25 13:23:28 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Manos Pitsidianakis X-Patchwork-Id: 9922017 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 81BAB600C5 for ; Fri, 25 Aug 2017 13:29:09 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 74FAF204BE for ; Fri, 25 Aug 2017 13:29:09 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 69BD2223A5; Fri, 25 Aug 2017 13:29:09 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id B729B28138 for ; Fri, 25 Aug 2017 13:29:08 +0000 (UTC) Received: from localhost ([::1]:53323 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dlEfc-0001XF-14 for patchwork-qemu-devel@patchwork.kernel.org; Fri, 25 Aug 2017 09:29:08 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38353) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dlEbQ-0006kc-N0 for qemu-devel@nongnu.org; Fri, 25 Aug 2017 09:24:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dlEbP-0003Qc-1h for qemu-devel@nongnu.org; Fri, 25 Aug 2017 09:24:48 -0400 Received: from smtp1.ntua.gr ([2001:648:2000:de::183]:40220) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dlEbJ-0003NF-Fu; Fri, 25 Aug 2017 09:24:41 -0400 Received: from mail.ntua.gr (carp0.noc.ntua.gr [147.102.222.60]) (authenticated bits=0) by smtp1.ntua.gr (8.15.2/8.15.2) with ESMTPSA id v7PDO8cR025201 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 25 Aug 2017 16:24:09 +0300 (EEST) (envelope-from el13635@mail.ntua.gr) X-Authentication-Warning: smtp1.ntua.gr: Host carp0.noc.ntua.gr [147.102.222.60] claimed to be mail.ntua.gr From: Manos Pitsidianakis To: qemu-devel Date: Fri, 25 Aug 2017 16:23:28 +0300 Message-Id: <20170825132332.6734-4-el13635@mail.ntua.gr> X-Mailer: git-send-email 2.11.0 In-Reply-To: <20170825132332.6734-1-el13635@mail.ntua.gr> References: <20170825132332.6734-1-el13635@mail.ntua.gr> X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2001:648:2000:de::183 Subject: [Qemu-devel] [PATCH v3 3/7] block: require job-id when device is a node name X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Kevin Wolf , Alberto Garcia , Stefan Hajnoczi , qemu-block Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP With implicit filter nodes on the top of the graph it is not possible to generate job-ids with the name of the device in block_job_create() anymore, since the job's bs will not be a child_root. Instead we can require that job-id is not NULL in block_job_create(), and check that a job-id has been set in the callers of block_job_create() in blockdev.c. It is more consistent to require an explicit job-id when the device parameter in the job creation command, eg { "execute": "drive-backup", "arguments": { "device": "drive0", "sync": "full", "target": "backup.img" } } is not a BlockBackend name, instead of automatically getting it from the root BS if device is a node name. That information is lost after calling block_job_create(), so we can do it in its caller instead. Signed-off-by: Manos Pitsidianakis Reviewed-by: Alberto Garcia Reviewed-by: Kevin Wolf --- include/block/blockjob_int.h | 4 +-- blockdev.c | 65 +++++++++++++++++++++++++++++++++++++++----- blockjob.c | 19 ++++++------- tests/test-blockjob.c | 9 +++--- 4 files changed, 72 insertions(+), 25 deletions(-) diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h index f13ad05c0d..a4ab7f3d59 100644 --- a/include/block/blockjob_int.h +++ b/include/block/blockjob_int.h @@ -112,8 +112,8 @@ struct BlockJobDriver { /** * block_job_create: - * @job_id: The id of the newly-created job, or %NULL to have one - * generated automatically. + * @job_id: The id of the newly-created job, must be non %NULL for external + * jobs and %NULL for internal jobs. * @job_type: The class object for the newly-created job. * @bs: The block * @perm, @shared_perm: Permissions to request for @bs diff --git a/blockdev.c b/blockdev.c index fc7b65c3f0..6ffa5b0b04 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3004,6 +3004,16 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, return; } + /* Always require a job-id when device is a node name */ + if (!has_job_id) { + if (blk_by_name(device)) { + job_id = device; + } else { + error_setg(errp, "An explicit job ID is required for this node"); + return; + } + } + /* Skip implicit filter nodes */ bs = bdrv_get_first_explicit(bs); @@ -3058,7 +3068,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device, /* backing_file string overrides base bs filename */ base_name = has_backing_file ? backing_file : base_name; - stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name, + stream_start(job_id, bs, base_bs, base_name, has_speed ? speed : 0, on_error, &local_err); if (local_err) { error_propagate(errp, local_err); @@ -3117,6 +3127,16 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device, /* Skip implicit filter nodes */ bs = bdrv_get_first_explicit(bs); + /* Always require a job-id when device is a node name */ + if (!has_job_id) { + if (blk_by_name(device)) { + job_id = device; + } else { + error_setg(errp, "An explicit job ID is required for this node"); + return; + } + } + aio_context = bdrv_get_aio_context(bs); aio_context_acquire(aio_context); @@ -3171,7 +3191,7 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device, " but 'top' is the active layer"); goto out; } - commit_active_start(has_job_id ? job_id : NULL, bs, base_bs, + commit_active_start(job_id, bs, base_bs, BLOCK_JOB_DEFAULT, speed, on_error, filter_node_name, NULL, NULL, false, &local_err); } else { @@ -3179,7 +3199,7 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device, if (bdrv_op_is_blocked(overlay_bs, BLOCK_OP_TYPE_COMMIT_TARGET, errp)) { goto out; } - commit_start(has_job_id ? job_id : NULL, bs, base_bs, top_bs, speed, + commit_start(job_id, bs, base_bs, top_bs, speed, on_error, has_backing_file ? backing_file : NULL, filter_node_name, &local_err); } @@ -3220,7 +3240,13 @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn, backup->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS; } if (!backup->has_job_id) { - backup->job_id = NULL; + /* Always require a job-id when device is a node name */ + if (blk_by_name(backup->device)) { + backup->job_id = backup->device; + } else { + error_setg(errp, "An explicit job ID is required for this node"); + return NULL; + } } if (!backup->has_compress) { backup->compress = false; @@ -3366,7 +3392,13 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, backup->on_target_error = BLOCKDEV_ON_ERROR_REPORT; } if (!backup->has_job_id) { - backup->job_id = NULL; + /* Always require a job-id when device is a node name */ + if (blk_by_name(backup->device)) { + backup->job_id = backup->device; + } else { + error_setg(errp, "An explicit job ID is required for this node"); + return NULL; + } } if (!backup->has_compress) { backup->compress = false; @@ -3509,6 +3541,16 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp) return; } + /* Always require a job-id when device is a node name */ + if (!arg->has_job_id) { + if (blk_by_name(arg->device)) { + arg->job_id = arg->device; + } else { + error_setg(errp, "An explicit job ID is required for this node"); + return; + } + } + /* Skip implicit filter nodes */ bs = bdrv_get_first_explicit(bs); @@ -3624,7 +3666,7 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp) bdrv_set_aio_context(target_bs, aio_context); - blockdev_mirror_common(arg->has_job_id ? arg->job_id : NULL, bs, target_bs, + blockdev_mirror_common(arg->job_id, bs, target_bs, arg->has_replaces, arg->replaces, arg->sync, backing_mode, arg->has_speed, arg->speed, arg->has_granularity, arg->granularity, @@ -3674,12 +3716,21 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id, return; } + /* Always require a job-id when device is a node name */ + if (!has_job_id) { + if (blk_by_name(device)) { + job_id = device; + } else { + error_setg(errp, "An explicit job ID is required for this node"); + return; + } + } aio_context = bdrv_get_aio_context(bs); aio_context_acquire(aio_context); bdrv_set_aio_context(target_bs, aio_context); - blockdev_mirror_common(has_job_id ? job_id : NULL, bs, target_bs, + blockdev_mirror_common(job_id, bs, target_bs, has_replaces, replaces, sync, backing_mode, has_speed, speed, has_granularity, granularity, diff --git a/blockjob.c b/blockjob.c index 70a78188b7..1231be5ce3 100644 --- a/blockjob.c +++ b/blockjob.c @@ -622,20 +622,17 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver, return NULL; } - if (job_id == NULL && !(flags & BLOCK_JOB_INTERNAL)) { - job_id = bdrv_get_device_name(bs); - if (!*job_id) { - error_setg(errp, "An explicit job ID is required for this node"); - return NULL; - } - } - - if (job_id) { - if (flags & BLOCK_JOB_INTERNAL) { + if (flags & BLOCK_JOB_INTERNAL) { + if (job_id) { error_setg(errp, "Cannot specify job ID for internal block job"); return NULL; } - + } else { + /* Require job-id. */ + if (!job_id) { + error_setg(errp, "A job ID must be specified"); + return NULL; + } if (!id_wellformed(job_id)) { error_setg(errp, "Invalid job ID '%s'", job_id); return NULL; diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c index 23bdf1a932..fc41ea2147 100644 --- a/tests/test-blockjob.c +++ b/tests/test-blockjob.c @@ -93,7 +93,7 @@ static void test_job_ids(void) blk[1] = create_blk("drive1"); blk[2] = create_blk("drive2"); - /* No job ID provided and the block backend has no name */ + /* No job ID provided */ job[0] = do_test_id(blk[0], NULL, false); /* These are all invalid job IDs */ @@ -119,16 +119,15 @@ static void test_job_ids(void) block_job_early_fail(job[0]); job[1] = do_test_id(blk[1], "id0", true); - /* No job ID specified, defaults to the backend name ('drive1') */ block_job_early_fail(job[1]); - job[1] = do_test_id(blk[1], NULL, true); + job[1] = do_test_id(blk[1], "drive1", true); /* Duplicate job ID */ job[2] = do_test_id(blk[2], "drive1", false); - /* The ID of job[2] would default to 'drive2' but it is already in use */ + /* The ID of job[2] is already in use */ job[0] = do_test_id(blk[0], "drive2", true); - job[2] = do_test_id(blk[2], NULL, false); + job[2] = do_test_id(blk[2], "drive2", false); /* This one is valid */ job[2] = do_test_id(blk[2], "id_2", true);