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