[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2] add --cache, --serial, --sharable and --pciaddress options(was Re: [PATCH 1/3] add --cache option for attach-disk)



On Tue, Jul 12, 2011 at 11:50:51AM -0600, Eric Blake wrote:
> On 07/12/2011 01:47 AM, Hu Tao wrote:
> >>>  
> >>> +    ignore_value(vshCommandOptString(cmd, "cache", &cache));
> >>
> >> Not so nice.
> >>
> >> --cache ''
> >>
> >> will make vshCommandOptString return -1, because that usage is a virsh
> >> usage error and should be diagnosed as such up front, rather than
> >> accidentally passing cache='' through the XML to the libvirt API.
> > 
> > I found that in the case of --cache '' vshCommandOptString returns 0,
> > with cache(3rd parameter) unchanged. So can we safely ignore value or do
> > I miss something?
> 
> vshCommandOptString currently returns:
> 1 if the option was provided and non-empty, or provided and empty and
> VSH_OFLAG_EMPTY_OK
> 0 if the option was not provided, or provided but empty
> -1 if the option was not provided but VSH_OFLAG_REQ
> 
> --cache isn't marked VSH_OFLAG_REQ, so we won't get -1.  And we didn't
> pass VSH_OFLAG_EMPTY_OK, so an empty string returns 0 instead of 1,
> leaving the variable NULL the same as if --cache had not been provided.
> 
> But your code would be the first instance of using ignore_value on
> vshCommandOptString.  I'm starting to think that's a bug in the
> implementation, and that vshCommandOptString should instead return:
> 
> 1 if the option was provided and non-empty, or provided and empty and
> VSH_OFLAG_EMPTY_OK
> 0 if the option was not provided
> -1 if the option was not provided but VSH_OFLAG_REQ, or provided and
> empty and not VSH_OFLAG_EMPTY_OK

Sounds good.

> 
> in which case, it _does_ become important to check for errors.
> > @@ -10209,6 +10254,10 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
> >          goto cleanup;
> >      }
> >  
> > +    ignore_value(vshCommandOptString(cmd, "cache", &cache));
> > +    ignore_value(vshCommandOptString(cmd, "serial", &serial));
> > +    ignore_value(vshCommandOptString(cmd, "pciaddress", &strpciaddr));
> 
> At any rate, we already have lots of existing code that looks like:
> 
> if (vshCommandOptString(cmd, "cache", &cache) < 0 ||
>     vshCommandOptString(cmd, "serial", &serial) < 0 ||
>     vshCommandOptString(cmd, "pciaddress", &strpciaddr) < 0) {
>     vshError(ctl, "%s", _("missing argument"));
>     goto out;
> }

Yes, the code should looks like this. I didn't noticed VSH_OFLAG_REQ(and
friends). Will update in v3.

-- 
Thanks,
Hu Tao


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]