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; } -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org