[libvirt] [PATCH 02/14] Retain disk PCI address across libvirtd restarts

[Daniel P. Berrange]

On Mon, Jul 20, 2009 at 03:05:31PM +0100, Mark McLoughlin wrote:
> On Mon, 2009-07-20 at 14:42 +0100, Daniel P. Berrange wrote:
> > On Mon, Jul 20, 2009 at 12:51:12PM +0100, Mark McLoughlin wrote:
> > > When we hot-plug a disk device into a qemu guest, we need to retain its
> > > PCI address so that it can be removed again later. Currently, we do
> > > retain the slot number, but not across libvirtd restarts.
> > > 
> > > Add <state devaddr="xxxx:xx:xx"/> to the disk device XML config when the
> > > VIR_DOMAIN_XML_INTERNAL_STATUS flag is used. We still don't parse the
> > > domain and bus number, but the format allows us to do that in future.
> > > 
> > > * src/domain_conf.h: replace slotnum with pci_addr
> > > 
> > 
> > 
> > > diff --git a/src/domain_conf.h b/src/domain_conf.h
> > > index 6e111fa..1766b61 100644
> > > --- a/src/domain_conf.h
> > > +++ b/src/domain_conf.h
> > > @@ -106,7 +106,7 @@ struct _virDomainDiskDef {
> > >      int cachemode;
> > >      unsigned int readonly : 1;
> > >      unsigned int shared : 1;
> > > -    int slotnum; /* pci slot number for unattach */
> > > +    char *pci_addr; /* for detach */
> > >  };
> > 
> > I think it'd be nicer to store the parsed address here as a
> > nested struct with domain, bus, slot.
> > 
> > It is not really saving us trouble by using a string, since most
> > of the places using this field end up asprintf'ing it into another
> > string, or even having to extract pieces out of it again.
> 
> It's saving us trouble because you don't have to code the equivalent of
> virDomainHostdevSubsysPciDefParseXML() and have e.g.
> 
>   <state>
>     <address domain="0" bus="0" slot="5"/>
>   </state>
> 
> I just now started to do it and then realized how much extra hassle the
> XML parsing was going to be. All for some internal data that we use in
> textual format anyway. Are you sure? :-)

True you have to parse 3 attributes instead of just 1, but IMHO it
pays off elsewhere in the patch. eg in the QEMU driver's PCI disk
attach code, this is rather wierd, unless you realize that 'pci_addr'
field here is a string of the format 'X:Y:Z', and not including a
function number.

+        const char *slot = strrchr(detach->pci_addr, ':');
+        if (!slot) {
+            qemudReportError(conn, NULL, NULL, VIR_ERR_OPERATION_FAILED,
+                             _("Invalid PCI address for device '%s' : %s"),
+                             detach->dst, detach->pci_addr);
+            goto cleanup;
+        }
+        ++slot;
+        if (virAsprintf(&cmd, "pci_del 0 %s", slot) < 0) {

If we stored it raw, then you'd just have 1 line

+        if (virAsprintf(&cmd, "pci_del 0 %s", detach->pci_addr.slot) < 0) {


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