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 4:58 PM > On 10/15/13 05:54, Chen Hanxiao wrote: >> From: Chen Hanxiao <chenhanxiao(a)cn.fujitsu.com&gt; >> >> 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);