Re: [Qemu-devel] [PATCH v2 3/6] block: require job-id when device is a node name

[Manos Pitsidianakis <el13635@mail.ntua.gr>]

[-- Attachment #1: Type: text/plain, Size: 2403 bytes --]

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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]