[libvirt] [PATCH 1/2]virsh: enable attatch-disk command option '--mode' accept two parameters

[Peter Krempa]

On 10/15/13 11:41, Chen Hanxiao wrote:
> 
> 
>> -----Original Message-----
>> From: Peter Krempa [mailto:pkrempa at redhat.com]
>> Sent: Tuesday, October 15, 2013 4:58 PM
>> On 10/15/13 05:54, Chen Hanxiao wrote:
>>> From: Chen Hanxiao <chenhanxiao at cn.fujitsu.com>
>>>
>>> Current disk could accept both 'readonly' and 'shareable'
>>> at the same time.
>>> But '--mode' could only accept one parameter.
>>>
>>> This patch enables '--mode' accept parameters like examples below:
>>>
>>> virsh # attach-disk domain /home/1.img sdd --mode shareable,readonly
>>>
>>>      if (mode) {
>>> -        if (STRNEQ(mode, "readonly") && STRNEQ(mode, "shareable")) {
>>> +        if (!(STRPREFIX(mode, "readonly") ||
>>> +            STRPREFIX(mode, "shareable"))) {
>>
>> This won't work if you use "--mode readonly,asdf". Also indentation of
>> the second line is off.
> 
> If we use "--mode readonly,asdf", only 'readonly' will be recognized as
> valid parameter. 

'readonly' is valid, but as that condition is only checking the prefix
of the @mode string to be 'readonly' or 'shareable' then you can write
anything after that and the check won't trigger. If you are going to use
a comma separated list here, you need to tokenize the string and check
every item in the array.

> 
>>
>>>              vshError(ctl, _("No support for %s in command
>> 'attach-disk'"),
>>>                       mode);
>>>              goto cleanup;
>>> @@ -600,8 +601,23 @@ cmdAttachDisk(vshControl *ctl, const vshCmd
>> *cmd)
>>>                            (isFile) ? "file" : "dev",
>>>                            source);
>>>      virBufferAsprintf(&buf, "  <target dev='%s'/>\n", target);
>>> -    if (mode)
>>> -        virBufferAsprintf(&buf, "  <%s/>\n", mode);
>>> +    if (mode) {
>>> +        if (STRPREFIX(mode, "readonly")) {
>>> +            virBufferAddLit(&buf, "  <readonly/>\n");
>>> +        } else {
>>> +            virBufferAddLit(&buf, "  <shareable/>\n");
>>> +        }
>>> +
>>> +        char *rest;
>>> +        if ((rest = strchr(mode, ','))) {
>>> +            rest++;
>>> +            if (STRPREFIX(rest, "readonly")) {
>>> +                virBufferAddLit(&buf, "  <readonly/>\n");
>>> +            } else if (STRPREFIX(rest, "shareable")) {
>>> +                virBufferAddLit(&buf, "  <shareable/>\n");
>>> +            }
>>> +        }
>>> +    }
>>
>> Hmmm, this is a very convoluted way to do stuff. I would recommend doing
>> the sanity check right and then you can do either:
>>
>> 	if (mode &&
>> 	    strstr(mode, "readonly"))
>> 		virBufferAddLit(&buf, "  <readonly/>\n");
>>
>> 	if (mode &&
>>  	    strstr(mode, "shareable"))
>> 		virBufferAddLit...
>>
> 
> If we use strstr(), --mode XXshareableXX will take effect.
> I try to let --mode accept: 	(readonly as A, shareable as B)

If you make the argument check above bulletproof, which it isn't right
now it will not bother you.

> A
> B
> A,B[xxxx]
> B,A[xxxx]
> A,A[xxxx]
> B,B[xxxx]
> 
>> or tokenize the string properly (see vshStringToArray) and output the
>> elements in a loop
> 
> I will check this and see. Thanks for you hints.
>>
>>>
>>>      if (serial)
>>>          virBufferAsprintf(&buf, "  <serial>%s</serial>\n", serial);

Peter

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