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

[Chen Hanxiao]

> -----Original Message-----
> From: Peter Krempa [mailto:pkrempa at redhat.com]
> Sent: Tuesday, October 15, 2013 5:48 PM
> To: Chen Hanxiao; libvir-list at 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 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>
> >> 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