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

On Fri, Jan 15, 2010 at 01:39:18PM +0100, Daniel Veillard wrote: > 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: We pass an explicit '16' to the virStrToLong_ui() so that should prevent it doing octal IIUC > > <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 My intent was for these attributes to always be interpreted as hex, no matter what, even if 0x is left off.