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. BertoThanks, 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.
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
signature.asc
Description: PGP signature