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@xxxxxxxxxx
> 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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list