> -----Original Message----- > From: Peter Krempa [mailto:pkrempa@xxxxxxxxxx] > Sent: Tuesday, October 15, 2013 5:48 PM > To: Chen Hanxiao; libvir-list@xxxxxxxxxx > Subject: Re: [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@xxxxxxxxxx] > >> Sent: Tuesday, October 15, 2013 4:58 PM > >> On 10/15/13 05:54, Chen Hanxiao wrote: > >>> From: Chen Hanxiao <chenhanxiao@xxxxxxxxxxxxxx> > >> 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list