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

[Eric Blake]

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110712/3297b10b/attachment-0001.sig>