qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Alberto Garcia
Subject: Re: [Qemu-block] [PATCH v2 3/6] block: require job-id when device is a node name
Date: Tue, 22 Aug 2017 11:57:28 +0200
User-agent: Notmuch/0.18.2 (http://notmuchmail.org) Emacs/24.4.1 (i586-pc-linux-gnu)

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]