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

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



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 :|


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