[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 12:58:54 +0200 |
User-agent: |
Notmuch/0.18.2 (http://notmuchmail.org) Emacs/24.4.1 (i586-pc-linux-gnu) |
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 4/6] block: remove legacy I/O throttling, (continued)
Re: [Qemu-block] [Qemu-devel] [PATCH v2 0/6] block: remove legacy I/O throttling, Eric Blake, 2017/08/09