Re: [Qemu-devel] [PATCH v2 3/6] block: require job-id when device is a node name
On Tue 22 Aug 2017 12:31:26 PM CEST, Manos Pitsidianakis wrote:
> On Tue, Aug 22, 2017 at 11:57:28AM +0200, Alberto Garcia wrote:
>>On Mon 21 Aug 2017 05:39:48 PM CEST, Manos Pitsidianakis wrote:
> -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;
> }
When BLOCK_JOB_INTERNAL is set, then job_id must be NULL...
> -
> +} else {
> +/* Require job-id. */
> +assert(job_id);
> if (!id_wellformed(job_id)) {
> error_setg(errp, "Invalid job ID '%s'", job_id);
> return NULL;
> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
> index f13ad05c0d..ff906808a6 100644
> --- a/include/block/blockjob_int.h
> +++ b/include/block/blockjob_int.h
> @@ -112,8 +112,7 @@ 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.
...but here you say that it must not be NULL.
And since job_id can be NULL in some cases I think I'd replace the
assert(job_id) that you added to block_job_create() with a normal
pointer check + error_setg().
> @@ -93,9 +93,6 @@ 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 */
> -job[0] = do_test_id(blk[0], NULL, false);
> -
If you decide to handle NULL ids by returning and error instead of
asserting, we should keep a couple of tests for that scenario.
Berto
>>>
>>> Thanks, I will change that. What cases are you thinking of besides
>>> internal jobs though?
>>
>>Both cases (external job with a NULL id and internal job with non-NULL
>>id), checking that block_job_create() does return an error.
>
> I thought we handled the external job case by requiring job_id is set
> before calling block_job_create(). I will check my patch again.
Yes, that appears to be checked correctly, I don't think you can call
block_job_create() directly with a NULL id for external block job.
But I also don't see how you can create an internal job with a non-NULL
id, so you could assert() there as well.
Either assert on both places or use error_setg() in both places. I think
I prefer the latter.
Berto
Re: [Qemu-devel] [PATCH v2 3/6] block: require job-id when device is a node name
On Tue, Aug 22, 2017 at 11:57:28AM +0200, Alberto Garcia wrote:
On Mon 21 Aug 2017 05:39:48 PM CEST, Manos Pitsidianakis wrote:
-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;
}
When BLOCK_JOB_INTERNAL is set, then job_id must be NULL...
-
+} else {
+/* Require job-id. */
+assert(job_id);
if (!id_wellformed(job_id)) {
error_setg(errp, "Invalid job ID '%s'", job_id);
return NULL;
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index f13ad05c0d..ff906808a6 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -112,8 +112,7 @@ 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.
...but here you say that it must not be NULL.
And since job_id can be NULL in some cases I think I'd replace the
assert(job_id) that you added to block_job_create() with a normal
pointer check + error_setg().
@@ -93,9 +93,6 @@ 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 */
-job[0] = do_test_id(blk[0], NULL, false);
-
If you decide to handle NULL ids by returning and error instead of
asserting, we should keep a couple of tests for that scenario.
Berto
Thanks, I will change that. What cases are you thinking of besides
internal jobs though?
Both cases (external job with a NULL id and internal job with non-NULL
id), checking that block_job_create() does return an error.
I thought we handled the external job case by requiring job_id is set
before calling block_job_create(). I will check my patch again.
And should documentation of block_job_create reflect that internal
jobs have job_id == NULL?
Yes, it should document the restrictions of the job_id parameter.
Berto
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v2 3/6] block: require job-id when device is a node name
On Mon 21 Aug 2017 05:39:48 PM CEST, Manos Pitsidianakis wrote:
>>> -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;
>>> }
>>
>>When BLOCK_JOB_INTERNAL is set, then job_id must be NULL...
>>
>>> -
>>> +} else {
>>> +/* Require job-id. */
>>> +assert(job_id);
>>> if (!id_wellformed(job_id)) {
>>> error_setg(errp, "Invalid job ID '%s'", job_id);
>>> return NULL;
>>> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
>>> index f13ad05c0d..ff906808a6 100644
>>> --- a/include/block/blockjob_int.h
>>> +++ b/include/block/blockjob_int.h
>>> @@ -112,8 +112,7 @@ 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.
>>
>>...but here you say that it must not be NULL.
>>
>>And since job_id can be NULL in some cases I think I'd replace the
>>assert(job_id) that you added to block_job_create() with a normal
>>pointer check + error_setg().
>>
>>> @@ -93,9 +93,6 @@ 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 */
>>> -job[0] = do_test_id(blk[0], NULL, false);
>>> -
>>
>>If you decide to handle NULL ids by returning and error instead of
>>asserting, we should keep a couple of tests for that scenario.
>>
>>Berto
>>
>
> Thanks, I will change that. What cases are you thinking of besides
> internal jobs though?
Both cases (external job with a NULL id and internal job with non-NULL
id), checking that block_job_create() does return an error.
> And should documentation of block_job_create reflect that internal
> jobs have job_id == NULL?
Yes, it should document the restrictions of the job_id parameter.
Berto
Re: [Qemu-devel] [PATCH v2 3/6] block: require job-id when device is a node name
On Mon, Aug 21, 2017 at 05:05:30PM +0200, Alberto Garcia wrote:
On Wed 09 Aug 2017 04:02:53 PM CEST, Manos Pitsidianakis wrote:
@@ -622,20 +622,14 @@ 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;
}
When BLOCK_JOB_INTERNAL is set, then job_id must be NULL...
-
+} else {
+/* Require job-id. */
+assert(job_id);
if (!id_wellformed(job_id)) {
error_setg(errp, "Invalid job ID '%s'", job_id);
return NULL;
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index f13ad05c0d..ff906808a6 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -112,8 +112,7 @@ 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.
...but here you say that it must not be NULL.
And since job_id can be NULL in some cases I think I'd replace the
assert(job_id) that you added to block_job_create() with a normal
pointer check + error_setg().
@@ -93,9 +93,6 @@ 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 */
-job[0] = do_test_id(blk[0], NULL, false);
-
If you decide to handle NULL ids by returning and error instead of
asserting, we should keep a couple of tests for that scenario.
Berto
Thanks, I will change that. What cases are you thinking of besides
internal jobs though? And should documentation of block_job_create
reflect that internal jobs have job_id == NULL?
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v2 3/6] block: require job-id when device is a node name
On Wed 09 Aug 2017 04:02:53 PM CEST, Manos Pitsidianakis wrote:
> @@ -622,20 +622,14 @@ 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;
> }
When BLOCK_JOB_INTERNAL is set, then job_id must be NULL...
> -
> +} else {
> +/* Require job-id. */
> +assert(job_id);
> if (!id_wellformed(job_id)) {
> error_setg(errp, "Invalid job ID '%s'", job_id);
> return NULL;
> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
> index f13ad05c0d..ff906808a6 100644
> --- a/include/block/blockjob_int.h
> +++ b/include/block/blockjob_int.h
> @@ -112,8 +112,7 @@ 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.
...but here you say that it must not be NULL.
And since job_id can be NULL in some cases I think I'd replace the
assert(job_id) that you added to block_job_create() with a normal
pointer check + error_setg().
> @@ -93,9 +93,6 @@ 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 */
> -job[0] = do_test_id(blk[0], NULL, false);
> -
If you decide to handle NULL ids by returning and error instead of
asserting, we should keep a couple of tests for that scenario.
Berto
[Qemu-devel] [PATCH v2 3/6] block: require job-id when device is a node name
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
---
blockdev.c | 65 +++-
blockjob.c | 16 ---
include/block/blockjob_int.h | 3 +-
tests/test-blockjob.c| 10 ++-
4 files changed, 67 insertions(+), 27 deletions(-)
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;
