Re: [libvirt] [PATCH 1/2]virsh: enable attatch-disk command option '--mode' accept two parameters
-----Original Message-----
From: Peter Krempa [mailto:pkrempa@redhat.com]
Sent: Tuesday, October 15, 2013 5:48 PM
To: Chen Hanxiao; libvir-list(a)redhat.com
Subject: Re: [libvirt] [PATCH 1/2]virsh: enable attatch-disk command
option
'--mode' accept two parameters
On 10/15/13 11:41, Chen Hanxiao wrote:
>
>
>> -----Original Message-----
>> From: Peter Krempa [mailto:pkrempa@redhat.com]
>> Sent: Tuesday, October 15, 2013 4:58 PM
>> On 10/15/13 05:54, Chen Hanxiao wrote:
>>> From: Chen Hanxiao <chenhanxiao(a)cn.fujitsu.com>
>> 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.
Thanks for your kindly advice.
If we decide to keep this patch, I'll finish it as you mentioned above.
Thanks.
>
>>
>>> + } 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.
Peter