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
