Re: [Qemu-block] [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-block] [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-block] [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-block] [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-block] [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
