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