[libvirt] [PATCH]: Fix "virsh attach-disk" and "virsh attach-interface"

[Daniel P. Berrange]

On Tue, Aug 05, 2008 at 12:02:47PM +0200, Chris Lalancette wrote:
> With the recent refactoring of the domain code, plus the changes with the Xend
> code, a couple of bugs were introduced into the attach-disk and attach-interface
> functionality.  This patch fixes 3 bugs:
> 
> 1)  In xenDaemonAttachDevice(), there is a switch statement to determine which
> of the xenDaemonFormatSxpr{Disk,Net} functions to call.  Unfortunately, the case
> statements are all missing the corresponding "break", so we always fall-through
> to the default error case.  This patch just adds the appropriate break statements.

ACK

> 2)  (minor) In xenDaemonDomainDefineXML (that's a mouthful!), there is a stray
> "fprintf".  This is now converted to a proper virXendError().

ACK

> 3)  xenDaemonFormatSxpr{Disk,Net} were adding an extra (device to the front of
> the sexpr expressions that xend did not expect (this is Xend on RHEL 5.2).
> Because of this, the attaches would fail.  The patch fixes this by removing the
> (device from the front, which makes attach-disk and attach-interface work again.

This isn't entirely correct. The previous code was in fact inconsistent in this
area. Taking libvirt 0.4.4 as an example

  - xenDaemonAttachDevice  calls into
  - virParseXMLDevice  returns the SEXPR by calling into
  - virDomainParseXMLDiskDesc or virDomainParseXMLIfDesc

The DiskDesc method returns an SEXPR with a '(device ' prefix. THe IfDesc method
returns a SEXPR without a '(device ' prefix. I'm pretty sure I've used the disk
hotplug in RHEL5, which implies it does accept a (device prefix. I've never tried
network hotplug, so can't say whether that worked or not. In any case I think this
needs some more investigation of behaviour on Xen 3.0.3, vs  3.1.0 and 3.2.0 before
we change the SEXPR again

Regards,
Daniel

> Index: src/xend_internal.c
> ===================================================================
> RCS file: /data/cvs/libvirt/src/xend_internal.c,v
> retrieving revision 1.207
> diff -u -r1.207 xend_internal.c
> --- a/src/xend_internal.c	4 Aug 2008 06:33:25 -0000	1.207
> +++ b/src/xend_internal.c	5 Aug 2008 09:59:57 -0000
> @@ -3900,6 +3900,7 @@
>                                      STREQ(def->os.type, "hvm") ? 1 : 0,
>                                      priv->xendConfigVersion) < 0)
>              goto cleanup;
> +        break;
>  
>      case VIR_DOMAIN_DEVICE_NET:
>          if (xenDaemonFormatSxprNet(domain->conn,
> @@ -3908,6 +3909,7 @@
>                                     STREQ(def->os.type, "hvm") ? 1 : 0,
>                                     priv->xendConfigVersion) < 0)
>              goto cleanup;
> +        break;
>  
>      default:
>          virXendError(domain->conn, VIR_ERR_NO_SUPPORT, "%s",
> @@ -4292,7 +4294,8 @@
>      ret = xend_op(conn, "", "op", "new", "config", sexpr, NULL);
>      VIR_FREE(sexpr);
>      if (ret != 0) {
> -        fprintf(stderr, _("Failed to create inactive domain %s\n"), name);
> +        virXendError(conn, VIR_ERR_XEN_CALL,
> +                     _("Failed to create inactive domain %s\n"), name);
>          goto error;
>      }
>  
> @@ -5029,7 +5032,6 @@
>          xendConfigVersion == 1)
>          return 0;
>  
> -    virBufferAddLit(buf, "(device ");
>      /* Normally disks are in a (device (vbd ...)) block
>       * but blktap disks ended up in a differently named
>       * (device (tap ....)) block.... */
> @@ -5083,7 +5085,7 @@
>      else
>          virBufferAddLit(buf, "(mode 'w')");
>  
> -    virBufferAddLit(buf, "))");
> +    virBufferAddLit(buf, ")");
>  
>      return 0;
>  }
> @@ -5117,7 +5119,7 @@
>          return -1;
>      }
>  
> -    virBufferAddLit(buf, "(device (vif ");
> +    virBufferAddLit(buf, "(vif ");
>  
>      virBufferVSprintf(buf,
>                        "(mac '%02x:%02x:%02x:%02x:%02x:%02x')",
> @@ -5177,7 +5179,7 @@
>      if ((hvm) && (xendConfigVersion < 4))
>          virBufferAddLit(buf, "(type ioemu)");
>  
> -    virBufferAddLit(buf, "))");
> +    virBufferAddLit(buf, ")");
>  
>      return 0;
>  }

> --
> Libvir-list mailing list
> Libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list


-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|