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

[Eric Blake]

On 11/08/2011 12:16 AM, Xu He Jie wrote:
> As the description of removing CDROM media from
>    http://wiki.libvirt.org/page/QEMUSwitchToLibvirt#eject_DEV

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.

>
> Add flag 'VSH_OFLAG_EMPTY_OK' to the option 'source' of attach-disk
>
> Signed-off-by: Xu He Jie<xuhj at linux.vnet.ibm.com>
> ---
>   tools/virsh.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/tools/virsh.c b/tools/virsh.c
> index 5544a41..e7eae74 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -11569,7 +11569,7 @@ 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, N_("source of disk device")},
> +    {"source",  VSH_OT_DATA, VSH_OFLAG_REQ | VSH_OFLAG_EMPTY_OK, N_("source of disk device")},

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org