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

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

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