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

[Peter Krempa]

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
> 
> Signed-off-by: Chen Hanxiao <chenhanxiao at cn.fujitsu.com>
> ---
>  tools/virsh-domain.c | 22 +++++++++++++++++++---
>  tools/virsh.pod      |  2 +-
>  2 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 856e888..ba67a69 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -563,7 +563,8 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
>      }
>  
>      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.

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

or tokenize the string properly (see vshStringToArray) and output the
elements in a loop

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

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/ed85821b/attachment-0001.sig>