Re: [libvirt] [PATCH 02/34] Introduce a standardized data structure for device addresses

On Fri, Jan 08, 2010 at 05:22:58PM +0000, Daniel P. Berrange wrote: > All guest devices now use a common device address structure > summarized by: > > enum virDomainDeviceAddressType { > VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE, > VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI, > }; > > struct _virDomainDevicePCIAddress { > unsigned int domain; > unsigned int bus; > unsigned int slot; > unsigned int function; > }; > > struct _virDomainDeviceInfo { > int type; > union { > virDomainDevicePCIAddress pci; > } addr; > }; > > This replaces the anonymous structs in Disk/Net/Hostdev data > structures. Where available, the address is *always* printed > in the XML file, instead of being hidden in the internal state > file. > > <address type='pci' domain='0x0000' bus='0x1e' slot='0x07' function='0x0'/> > > The structure definition is based on Wolfgang Mauerer's disk > controller patch series. > > * docs/schemas/domain.rng: Define the <address> syntax and > associate it with disk/net/hostdev devices > * src/conf/domain_conf.h, src/conf/domain_conf.c, > src/libvirt_private.syms: APIs for parsing/formatting address > information. Also remove the QEMU specific 'pci_addr' attributes > * src/qemu/qemu_driver.c: Replace use of 'pci_addr' attrs with > new standardized format. > --- > docs/schemas/domain.rng | 55 +++++-- > src/conf/domain_conf.c | 420 +++++++++++++++++++++++++++++++++------------- > src/conf/domain_conf.h | 84 +++++----- > src/libvirt_private.syms | 6 + > src/qemu/qemu_driver.c | 64 ++++--- > 5 files changed, 428 insertions(+), 201 deletions(-) > [...] > +int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info, > + int type) > +{ > + if (info->type != type) > + return 0; > + > + switch (info->type) { > + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: > + return virDomainDevicePCIAddressIsValid(&info->addr.pci); > + } Hum, a switch without default and not handling all cases may generate a warning with some compiler options (I find this useful when extending the union) maybe we should explicitely return 0 with _NONE > +static int > +virDomainDevicePCIAddressParseXML(virConnectPtr conn, > + xmlNodePtr node, > + virDomainDevicePCIAddressPtr addr) > +{ > + char *domain, *slot, *bus, *function; > + int ret = -1; > + > + memset(addr, 0, sizeof(*addr)); > + > + domain = virXMLPropString(node, "domain"); > + bus = virXMLPropString(node, "bus"); > + slot = virXMLPropString(node, "slot"); > + function = virXMLPropString(node, "function"); > + > + if (domain && > + virStrToLong_ui(domain, NULL, 16, &addr->domain) < 0) { > + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", > + _("Cannot parse <address> 'domain' attribute")); > + goto cleanup; > + } Hum, there is a small mismatch between the parsing function and the validation, virStrToLong_ uses strtol like function decoding 0 leading numbers as octal, but we don't match octal in the Relax-NG associated functions:

<define name="pciDomain"> <data type="string"> <param name="pattern">(0x)?[0-9a-fA-F]{1,4}</param> </data> </define> Do we really intent to allow 0 started octal values ? I guess in octal we would need more than 4 digit to cover the address range. But it's a minor point, we could fix the RNG later