[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 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. 

> 
> >              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)
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);
> > diff --git a/tools/virsh.pod b/tools/virsh.pod
> > index e12a800..f6a80a3 100644
> > --- a/tools/virsh.pod
> > +++ b/tools/virsh.pod
> > @@ -1915,7 +1915,7 @@ expected.
> >  =item B<attach-disk> I<domain> I<source> I<target>
> >  [[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]]
> >  [I<--driver driver>] [I<--subdriver subdriver>] [I<--cache cache>]
> > -[I<--type type>] [I<--mode mode>] [I<--config>] [I<--sourcetype
soucetype>]
> > +[I<--type type>] [I<--mode mode[,mode2]>] [I<--config>] [I<--sourcetype
> soucetype>]
> >  [I<--serial serial>] [I<--wwn wwn>] [I<--shareable>] [I<--rawio>]
> >  [I<--address address>] [I<--multifunction>] [I<--print-xml>]
> >
> >
> 
> also it might be worth mentioning the meaning of the option in the text
> after this block.

I'll add it in the future patch.
> 
> Peter