Re: [libvirt] [PATCH] virsh: Add VSH_OFLAG_EMPTY_OK for attach-disk command

On 11/08/2011 12:16 AM, Xu He Jie wrote: Hmm. virsh attach-disk --type cdrom --mode readonly myguest "" hdc might look a bit nicer as: virsh attach-disk --type cdrom --mode readonly myguest --target hdc except that we marked --source as a required argument, so we have to provide something even when there is no real source. So I agree that we need your patch at a bare minimum to support this documented command line for adding a cdrom drive without a disk. Meanwhile, conf/domain_conf.c has this comment: /* People sometimes pass a bogus '' source path when they mean to omit the source element completely. eg CDROM without media. This is just a little compatability check to help those broken apps */ if (source && STREQ(source, "")) VIR_FREE(source); So while passing an empty string down through the XML eventually does the right thing, we make it sound like it is a gross hack, and that it would be nicer if virsh weren't one of "those broken apps" that exploits the xml hack. ACK to your change, but I'm reformatting it to fit 80 columns, then squashing in this additional change before pushing so that virsh will be a model citizen: diff --git i/tools/virsh.c w/tools/virsh.c index e7eae74..eed727b 100644 --- i/tools/virsh.c +++ w/tools/virsh.c @@ -11569,7 +11569,8 @@ static const vshCmdInfo info_attach_disk[] = { static const vshCmdOptDef opts_attach_disk[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, - {"source", VSH_OT_DATA, VSH_OFLAG_REQ | VSH_OFLAG_EMPTY_OK, N_("source of disk device")}, + {"source", VSH_OT_DATA, VSH_OFLAG_REQ | VSH_OFLAG_EMPTY_OK, + N_("source of disk device")}, {"target", VSH_OT_DATA, VSH_OFLAG_REQ, N_("target of disk device")}, {"driver", VSH_OT_STRING, 0, N_("driver of disk device")}, {"subdriver", VSH_OT_STRING, 0, N_("subdriver of disk device")}, @@ -11754,6 +11755,10 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptString(cmd, "source", &source) <= 0) goto cleanup; + /* Allow empty string as a placeholder that implies no source, for + * use in adding a cdrom drive with no disk. */ + if (!*source) + source = NULL; if (vshCommandOptString(cmd, "target", &target) <= 0) goto cleanup; @@ -11808,9 +11813,10 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) if (driver || subdriver || cache) virBufferAddLit(&buf, "/>\n"); - virBufferAsprintf(&buf, " <source %s='%s'/>\n", - (isFile) ? "file" : "dev", - source); + if (source) + virBufferAsprintf(&buf, " <source %s='%s'/>\n", + (isFile) ? "file" : "dev", + source); virBufferAsprintf(&buf, " <target dev='%s'/>\n", target); if (mode) virBufferAsprintf(&buf, " <%s/>\n", mode); -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org