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

[Manos Pitsidianakis]

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.

> > 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