[libvirt] [PATCH] virsh: A bit smarter attach-disk

Michal Privoznik mprivozn at redhat.com
Thu Mar 15 09:12:55 UTC 2012


On 15.03.2012 10:13, Osier Yang wrote:
> Detects the file type of source path if no "--sourcetype" and
> "driver" is specified, instead of always set the disk type as
> "block".
> 
> And previous "virCommandOptString" ensures "source" is not NULL,
> remove the conditional checking.
> ---
>  tools/virsh.c |   14 +++++++++-----
>  1 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/virsh.c b/tools/virsh.c
> index d45a4c9..3b845ac 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -14428,6 +14428,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
>      const char *stype = NULL;
>      virBuffer buf = VIR_BUFFER_INITIALIZER;
>      char *xml;
> +    struct stat st;
>  
>      if (!vshConnectionUsability(ctl, ctl->conn))
>          goto cleanup;
> @@ -14458,8 +14459,12 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
>      }
>  
>      if (!stype) {
> -        if (driver && (STREQ(driver, "file") || STREQ(driver, "tap")))
> +        if (driver && (STREQ(driver, "file") || STREQ(driver, "tap"))) {
>              isFile = true;
> +        } else {
> +            if (!stat(source, &st))
> +                isFile = S_ISREG(st.st_mode) ? true : false;

I think S_ISREG() would be sufficient here. But that's just cosmetic.

> +        }
>      } else if (STREQ(stype, "file")) {
>          isFile = true;
>      } else if (STRNEQ(stype, "block")) {
> @@ -14497,10 +14502,9 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
>      if (driver || subdriver || cache)
>          virBufferAddLit(&buf, "/>\n");
>  
> -    if (source)
> -        virBufferAsprintf(&buf, "  <source %s='%s'/>\n",
> -                          (isFile) ? "file" : "dev",
> -                          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);

However this looks bad. As written in commend just below
virCommandOptString(, source):

    /* Allow empty string as a placeholder that implies no source, for
     * use in adding a cdrom drive with no disk.  */
    if (!*source)
        source = NULL;

That means:
   virsh attach-disk <domain> "" dummy
make source NULL. Therefore you want to check source != NULL in the
first chunk too (okay, not as strict as here, since passing NULL to
stat() makes it fail, but it's clean coding style what matters too).

Michal




More information about the libvir-list mailing list