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> >>> >>> 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. '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. > >> >>> 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) If you make the argument check above bulletproof, which it isn't right now it will not bother you. > 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); Peter
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list