On Tue, Aug 05, 2008 at 04:10:59PM +0100, Daniel P. Berrange wrote:
> 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
Ok, I've found the original xenDaemonAttachDevice() had this hack to make
them consistent:
if (!memcmp(sexpr, "(device ", 8)) {
conf = sexpr + 8;
*(conf + strlen(conf) -1) = 0; /* suppress final ) */
}
else
conf = sexpr;
Which is just gross.
So, ACK to your original patch - it brings us back into line with expected
SEXPR for XenD we had in the past
Daniel
--
|: 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