Re: [libvirt] [PATCH] Don't validate disk type in virsh attach-disk

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Cole Robinson wrote:
> virsh attempts to validate the requested disk type, rather than just let
> the underlying driver do it. This was erroneously denying floppy device
> attaches. Patch attached.

Hi Cole,
Your patch would do the job.
Before:

  $ virsh -c test:///default attach-disk 1 s t --type floppy --mode readonly
  error: No support floppy in command 'attach-disk'

with the patch:

  $ virsh -c test:///default attach-disk 1 s t --type floppy --mode readonly
  error: this function is not supported by the hypervisor: virDomainAttachDevice

But consider what happens with e.g., "--type bogus".
Before, it'd mention the invalid type, "bogus".
With the patch, it doesn't (at least not using the test driver).
So maybe it'd be better to keep the up-front type check.

Of course, if we do keep the test in cmdAttachDisk,
the format string should be changed to this:

  vshError(ctl, FALSE, _("No %s support in command 'attach-disk'"), type);

> commit 3481061f6ed960269d9b29a4a1380367d557cf37
> Author: Cole Robinson <crobinso@xxxxxxxxxx>
> Date:   Fri Feb 27 10:47:49 2009 -0500
>
>     Let virt driver validate disk type for us in virsh attach-disk.
>
> diff --git a/src/virsh.c b/src/virsh.c
> index 298dde0..5c9c49f 100644
> --- a/src/virsh.c
> +++ b/src/virsh.c
> @@ -5022,13 +5022,6 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
>      type = vshCommandOptString(cmd, "type", NULL);
>      mode = vshCommandOptString(cmd, "mode", NULL);
>
> -    if (type) {
> -        if (STRNEQ(type, "cdrom") && STRNEQ(type, "disk")) {
> -            vshError(ctl, FALSE, _("No support %s in command 'attach-disk'"), type);
> -            goto cleanup;
> -        }
> -    }
> -
>      if (driver) {
>          if (STREQ(driver, "file") || STREQ(driver, "tap")) {
>              isFile = 1;
>
> --
> Libvir-list mailing list
> Libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list

--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]