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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
