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

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