[libvirt] [PATCH 1/6] Allow network model to contain "-"
From: Michael Ellerman <michael@ellerman.id.au> In QEMU PPC64 we have a network device called "spapr-vlan". We can specify this using the existing syntax for network devices, however libvirt currently rejects "spapr-vlan" in virDomainNetDefParseXML() because of the "-". Fix the code to accept "-". Signed-off-by: Michael Ellerman <michael@ellerman.id.au> --- src/conf/domain_conf.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 75e51a0..5de33b9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3703,8 +3703,7 @@ virDomainNetDefParseXML(virCapsPtr caps, if (model != NULL) { int i; for (i = 0 ; i < strlen(model) ; i++) { - int char_ok = c_isalnum(model[i]) || model[i] == '_'; - if (!char_ok) { + if (!c_isalnum(model[i]) && model[i] != '_' && model[i] != '-') { virDomainReportError(VIR_ERR_INVALID_ARG, "%s", _("Model name contains invalid characters")); goto error; -- 1.7.7.3
From: Michael Ellerman <michael@ellerman.id.au> On the PPC64 pseries machine type we need to use the spapr-vscsi device rather than an lsi. Signed-off-by: Michael Ellerman <michael@ellerman.id.au> --- src/qemu/qemu_command.c | 13 +++++++++---- src/qemu/qemu_command.h | 3 ++- src/qemu/qemu_hotplug.c | 2 +- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 032ead1..ca812de 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2118,7 +2118,8 @@ qemuBuildUSBControllerDevStr(virDomainControllerDefPtr def, } char * -qemuBuildControllerDevStr(virDomainControllerDefPtr def, +qemuBuildControllerDevStr(virDomainDefPtr domainDef, + virDomainControllerDefPtr def, virBitmapPtr qemuCaps, int *nusbcontroller) { @@ -2126,7 +2127,11 @@ qemuBuildControllerDevStr(virDomainControllerDefPtr def, switch (def->type) { case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: - virBufferAddLit(&buf, "lsi"); + if (STREQ(domainDef->os.arch, "ppc64") && STREQ(domainDef->os.machine, "pseries")) { + virBufferAddLit(&buf, "spapr-vscsi"); + } else { + virBufferAddLit(&buf, "lsi"); + } virBufferAsprintf(&buf, ",id=scsi%d", def->idx); break; @@ -4040,7 +4045,7 @@ qemuBuildCommandLine(virConnectPtr conn, char *devstr; virCommandAddArg(cmd, "-device"); - if (!(devstr = qemuBuildControllerDevStr(cont, qemuCaps, NULL))) + if (!(devstr = qemuBuildControllerDevStr(def, cont, qemuCaps, NULL))) goto error; virCommandAddArg(cmd, devstr); @@ -4059,7 +4064,7 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, "-device"); char *devstr; - if (!(devstr = qemuBuildControllerDevStr(def->controllers[i], qemuCaps, + if (!(devstr = qemuBuildControllerDevStr(def, def->controllers[i], qemuCaps, &usbcontroller))) goto error; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 1fe0394..3978b2b 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -95,7 +95,8 @@ char * qemuBuildDriveDevStr(virDomainDiskDefPtr disk, char * qemuBuildFSDevStr(virDomainFSDefPtr fs, virBitmapPtr qemuCaps); /* Current, best practice */ -char * qemuBuildControllerDevStr(virDomainControllerDefPtr def, +char * qemuBuildControllerDevStr(virDomainDefPtr domainDef, + virDomainControllerDefPtr def, virBitmapPtr qemuCaps, int *nusbcontroller); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 96c0070..eabfeaa 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -329,7 +329,7 @@ int qemuDomainAttachPciControllerDevice(struct qemud_driver *driver, goto cleanup; } - if (!(devstr = qemuBuildControllerDevStr(controller, priv->qemuCaps, NULL))) { + if (!(devstr = qemuBuildControllerDevStr(vm->def, controller, priv->qemuCaps, NULL))) { goto cleanup; } } -- 1.7.7.3
On 12/07/2011 11:41 PM, Michael Ellerman wrote:
From: Michael Ellerman <michael@ellerman.id.au>
On the PPC64 pseries machine type we need to use the spapr-vscsi device rather than an lsi.
Signed-off-by: Michael Ellerman <michael@ellerman.id.au> --- src/qemu/qemu_command.c | 13 +++++++++---- src/qemu/qemu_command.h | 3 ++- src/qemu/qemu_hotplug.c | 2 +- 3 files changed, 12 insertions(+), 6 deletions(-)
ACK. I've also added you to AUTHORS.
@@ -2126,7 +2127,11 @@ qemuBuildControllerDevStr(virDomainControllerDefPtr def,
switch (def->type) { case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: - virBufferAddLit(&buf, "lsi"); + if (STREQ(domainDef->os.arch, "ppc64") && STREQ(domainDef->os.machine, "pseries")) {
I wrapped this line to fit in 80 chars. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: Michael Ellerman <michaele@au1.ibm.com> For QEMU PPC64 we have a machine type ("pseries") which has a virtual bus called "spapr-vio". We need to be able to create devices on this bus, and as such need a way to specify the address for those devices. This patch adds a new address type "spapr-vio", which achieves this. The addressing is specified with a "reg" property in the address definition. The reg is optional, if it is not specified QEMU will auto-assign an address for the device. Signed-off-by: Michael Ellerman <michael@ellerman.id.au> --- src/conf/domain_conf.c | 43 ++++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 9 +++++++++ src/qemu/qemu_command.c | 5 +++++ 3 files changed, 56 insertions(+), 1 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5de33b9..665a0cd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -139,7 +139,8 @@ VIR_ENUM_IMPL(virDomainDeviceAddress, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST, "drive", "virtio-serial", "ccid", - "usb") + "usb", + "spapr-vio") VIR_ENUM_IMPL(virDomainDeviceAddressPciMulti, VIR_DOMAIN_DEVICE_ADDRESS_PCI_MULTI_LAST, @@ -1909,6 +1910,11 @@ virDomainDeviceInfoFormat(virBufferPtr buf, info->addr.usb.port); break; + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO: + if (info->addr.spaprvio.has_reg) + virBufferAsprintf(buf, " reg='%#llx'", info->addr.spaprvio.reg); + break; + default: virDomainReportError(VIR_ERR_INTERNAL_ERROR, _("unknown address type '%d'"), info->type); @@ -2168,6 +2174,34 @@ cleanup: } static int +virDomainDeviceSpaprVioAddressParseXML(xmlNodePtr node, + virDomainDeviceSpaprVioAddressPtr addr) +{ + char *reg; + int ret; + + memset(addr, 0, sizeof(*addr)); + addr->has_reg = false; + + reg = virXMLPropString(node, "reg"); + if (reg) { + if (virStrToLong_ull(reg, NULL, 16, &addr->reg) < 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse <address> 'reg' attribute")); + ret = -1; + goto cleanup; + } + + addr->has_reg = true; + } + + ret = 0; +cleanup: + VIR_FREE(reg); + return ret; +} + +static int virDomainDeviceUSBMasterParseXML(xmlNodePtr node, virDomainDeviceUSBMasterPtr master) { @@ -2280,6 +2314,11 @@ virDomainDeviceInfoParseXML(xmlNodePtr node, goto cleanup; break; + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO: + if (virDomainDeviceSpaprVioAddressParseXML(address, &info->addr.spaprvio) < 0) + goto cleanup; + break; + default: /* Should not happen */ virDomainReportError(VIR_ERR_INTERNAL_ERROR, @@ -3174,6 +3213,7 @@ virDomainControllerDefParseXML(xmlNodePtr node, } if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO && def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Controllers must use the 'pci' address type")); @@ -3563,6 +3603,7 @@ virDomainNetDefParseXML(virCapsPtr caps, /* XXX what about ISA/USB based NIC models - once we support * them we should make sure address type is correct */ if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO && def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Network interfaces must use 'pci' address type")); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index d6ed898..0ed6aba 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -69,6 +69,7 @@ enum virDomainDeviceAddressType { VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCID, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB, + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST }; @@ -121,6 +122,13 @@ struct _virDomainDeviceUSBAddress { char *port; }; +typedef struct _virDomainDeviceSpaprVioAddress virDomainDeviceSpaprVioAddress; +typedef virDomainDeviceSpaprVioAddress *virDomainDeviceSpaprVioAddressPtr; +struct _virDomainDeviceSpaprVioAddress { + unsigned long long reg; + bool has_reg; +}; + enum virDomainControllerMaster { VIR_DOMAIN_CONTROLLER_MASTER_NONE, VIR_DOMAIN_CONTROLLER_MASTER_USB, @@ -145,6 +153,7 @@ struct _virDomainDeviceInfo { virDomainDeviceVirtioSerialAddress vioserial; virDomainDeviceCcidAddress ccid; virDomainDeviceUSBAddress usb; + virDomainDeviceSpaprVioAddress spaprvio; } addr; int mastertype; union { diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index eaef0e1..17cc972 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1253,6 +1253,8 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs) def->controllers[i]->idx == 0) continue; + if (def->controllers[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) + continue; if (def->controllers[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) continue; if (qemuDomainPCIAddressSetNextAddr(addrs, &def->controllers[i]->info) < 0) @@ -1403,6 +1405,9 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, virBufferAsprintf(buf, ",bus="); qemuUsbId(buf, info->addr.usb.bus); virBufferAsprintf(buf, ".0,port=%s", info->addr.usb.port); + } else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) { + if (info->addr.spaprvio.has_reg) + virBufferAsprintf(buf, ",reg=%#llx", info->addr.spaprvio.reg); } return 0; -- 1.7.7.3
On 12/07/2011 11:41 PM, Michael Ellerman wrote:
From: Michael Ellerman <michaele@au1.ibm.com>
This is a different address than you used for patch 2 and 3 (and yet a third address compared to the email where you sent this patch). We can cope with that, but it means picking a favorite address for AUTHORS, then listing alternate addresses in .mailmap. You may want to change authorship of this patch when re-submitting (git commit --amend --author=address), or at least give instructions on which address(es) you want to be known by for your libvirt.git contributions.
For QEMU PPC64 we have a machine type ("pseries") which has a virtual bus called "spapr-vio". We need to be able to create devices on this bus, and as such need a way to specify the address for those devices.
This patch adds a new address type "spapr-vio", which achieves this.
The addressing is specified with a "reg" property in the address definition. The reg is optional, if it is not specified QEMU will auto-assign an address for the device.
Signed-off-by: Michael Ellerman <michael@ellerman.id.au> --- src/conf/domain_conf.c | 43 ++++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 9 +++++++++ src/qemu/qemu_command.c | 5 +++++ 3 files changed, 56 insertions(+), 1 deletions(-)
I can't apply this patch without documentation. Below, I'll include a first cut at the rng changes that I think match your code, as well as discuss what needs to be done in docs/formatdomain.html.in, before you send a v2 of the remainder of your series. A test case would also be nice, correlating the new XML to the new ppc64-specific command line, but that may entail writing separate patches to improve the testsuite to allow targetting a controlled ppc64 target type. The testsuite already hard-codes a fake x86_64 target, so that the suite can run tests even if on machines that lack an x86_64 qemu emulator, so I think there's precedence on how to do it. I've pushed 1-3, and we'll wait for v2 for 4-6.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5de33b9..665a0cd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -139,7 +139,8 @@ VIR_ENUM_IMPL(virDomainDeviceAddress, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST, "drive", "virtio-serial", "ccid", - "usb") + "usb", + "spapr-vio")
Can spapr-vio ever be used as a controller type, or does that not make sense? I'm guessing that since an <address type='spapr-vio' reg='0x1000'/> only has 'reg' as the differentiating address, and reg is 64-bits, that it is a flat addressing space, so you don't need a controller (contrast with <address type='pci' bus='0x0'/> corresponding to <controller type='pci' index='0'/>). But that makes documenting things a bit harder - previously, we have documented <address> in a haphazard manner, under the particular devices that must live on a particular bus, and pointed back to the corresponding controller. But if there is no corresponding controller, and since we now have an instance of which addressing to use depending on which architecture (for example, a console lives on <address type='pci'/> for x86_64 and <address type='spapr-vio'/> for ppc64), I think I have to do a pre-req patch that reorganizes the existing <address> documentation, to make it easier to insert in your new mode.
VIR_ENUM_IMPL(virDomainDeviceAddressPciMulti, VIR_DOMAIN_DEVICE_ADDRESS_PCI_MULTI_LAST, @@ -1909,6 +1910,11 @@ virDomainDeviceInfoFormat(virBufferPtr buf, info->addr.usb.port); break;
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO: + if (info->addr.spaprvio.has_reg) + virBufferAsprintf(buf, " reg='%#llx'", info->addr.spaprvio.reg); + break;
Is the intent that reg will always be in hex? "%#llx" results in 0 or in 0x1000; would it be better to use "0x%llx" to get 0x0 vs. 0x1000 so that we always have a leading 0x?
static int +virDomainDeviceSpaprVioAddressParseXML(xmlNodePtr node, + virDomainDeviceSpaprVioAddressPtr addr) +{ + char *reg; + int ret; + + memset(addr, 0, sizeof(*addr)); + addr->has_reg = false; + + reg = virXMLPropString(node, "reg"); + if (reg) { + if (virStrToLong_ull(reg, NULL, 16, &addr->reg) < 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse <address> 'reg' attribute")); + ret = -1; + goto cleanup; + } + + addr->has_reg = true;
Looking at patch 6/6, I see you start default assignments at 0x1000, 0x2000, or 0x30000000 depending on which device type is getting assigned, and increment attempts by 0x1000 until you have no collision. Must assignments be on a 0x1000 boundary? If so, we can tighten up the pattern I propose in the rng to require it, and the parser should be fixed to require it as well. Can an assignment of reg='0' ever be valid? If not, then we can use non-zero info->addr.spaprvio.reg as evidence of default assignment, rather than needing info->addr.spaprvio.has_reg. As promised, I think this RNG matches your code, but it would need further tweaks if we declare that a valid address must always be a non-zero multiple of 0x1000. diff --git i/docs/schemas/domaincommon.rng w/docs/schemas/domaincommon.rng index 27ec1da..dc85329 100644 --- i/docs/schemas/domaincommon.rng +++ w/docs/schemas/domaincommon.rng @@ -2263,6 +2263,11 @@ </attribute> </optional> </define> + <define name='spaprvioaddress'> + <attribute name='reg'> + <ref name='spaprvioAddr'/> + </attribute> + </define> <!-- Devices attached to a domain. Sub-elements such as <alias> are not documented here, as they @@ -2577,6 +2582,12 @@ </attribute> <ref name="usbportaddress"/> </group> + <group> + <attribute name='type'> + <value>spapr-vio</value> + </attribute> + <ref name='spaprvioaddress'/> + </group> </choice> </element> </define> @@ -2826,6 +2837,11 @@ <param name="pattern">(0x)?[0-7]</param> </data> </define> + <define name='spaprvioAddr'> + <data type='string'> + <param name='pattern'>(0x)?[0-9a-fA-F]{1,16}</param> + </data> + </define> <define name="driveController"> <data type="string"> <param name="pattern">[0-9]{1,2}</param> -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, 2011-12-09 at 14:49 -0700, Eric Blake wrote:
On 12/07/2011 11:41 PM, Michael Ellerman wrote:
From: Michael Ellerman <michaele@au1.ibm.com>
This is a different address than you used for patch 2 and 3 (and yet a third address compared to the email where you sent this patch). We can cope with that, but it means picking a favorite address for AUTHORS, then listing alternate addresses in .mailmap. You may want to change authorship of this patch when re-submitting (git commit --amend --author=address), or at least give instructions on which address(es) you want to be known by for your libvirt.git contributions.
Sorry that's a bit sloppy of me. They are all my addresses (obviously), I use michael@ellerman.id.au as my canonical address. I'm not sure why the mails are coming from michael@ozlabs.org, they didn't use to and I haven't had time to debug it yet.
I can't apply this patch without documentation. Below, I'll include a first cut at the rng changes that I think match your code, as well as discuss what needs to be done in docs/formatdomain.html.in, before you send a v2 of the remainder of your series.
Ah sorry. We actually have some patches internally to do that but I didn't write them so I didn't send them. They look similar to your changes below so I'll sort that out for v2.
A test case would also be nice, correlating the new XML to the new ppc64-specific command line, but that may entail writing separate patches to improve the testsuite to allow targetting a controlled ppc64 target type. The testsuite already hard-codes a fake x86_64 target, so that the suite can run tests even if on machines that lack an x86_64 qemu emulator, so I think there's precedence on how to do it.
OK I'll have a look at it and see if I can come up with something.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5de33b9..665a0cd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -139,7 +139,8 @@ VIR_ENUM_IMPL(virDomainDeviceAddress, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST, "drive", "virtio-serial", "ccid", - "usb") + "usb", + "spapr-vio")
Can spapr-vio ever be used as a controller type, or does that not make sense? I'm guessing that since an <address type='spapr-vio' reg='0x1000'/> only has 'reg' as the differentiating address, and reg is 64-bits, that it is a flat addressing space, so you don't need a controller (contrast with <address type='pci' bus='0x0'/> corresponding to <controller type='pci' index='0'/>).
No it can't be used as a controller. There's just a single instance of the bus with a flat 64-bit address space.
But that makes documenting things a bit harder - previously, we have documented <address> in a haphazard manner, under the particular devices that must live on a particular bus, and pointed back to the corresponding controller. But if there is no corresponding controller, and since we now have an instance of which addressing to use depending on which architecture (for example, a console lives on <address type='pci'/> for x86_64 and <address type='spapr-vio'/> for ppc64), I think I have to do a pre-req patch that reorganizes the existing <address> documentation, to make it easier to insert in your new mode.
OK, I haven't looked at the help doco yet, I'll have a look, and let me know what you want done there.
VIR_ENUM_IMPL(virDomainDeviceAddressPciMulti, VIR_DOMAIN_DEVICE_ADDRESS_PCI_MULTI_LAST, @@ -1909,6 +1910,11 @@ virDomainDeviceInfoFormat(virBufferPtr buf, info->addr.usb.port); break;
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO: + if (info->addr.spaprvio.has_reg) + virBufferAsprintf(buf, " reg='%#llx'", info->addr.spaprvio.reg); + break;
Is the intent that reg will always be in hex? "%#llx" results in 0 or in 0x1000; would it be better to use "0x%llx" to get 0x0 vs. 0x1000 so that we always have a leading 0x?
It is usually specified in hex, but that's just a stylistic convention. I guess it's better to switch to "0x%llx" though just for 100% consistency.
static int +virDomainDeviceSpaprVioAddressParseXML(xmlNodePtr node, + virDomainDeviceSpaprVioAddressPtr addr) +{ + char *reg; + int ret; + + memset(addr, 0, sizeof(*addr)); + addr->has_reg = false; + + reg = virXMLPropString(node, "reg"); + if (reg) { + if (virStrToLong_ull(reg, NULL, 16, &addr->reg) < 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse <address> 'reg' attribute")); + ret = -1; + goto cleanup; + } + + addr->has_reg = true;
Looking at patch 6/6, I see you start default assignments at 0x1000, 0x2000, or 0x30000000 depending on which device type is getting assigned, and increment attempts by 0x1000 until you have no collision. Must assignments be on a 0x1000 boundary?
No they can take any value. 4K is just "neater".
Can an assignment of reg='0' ever be valid? If not, then we can use non-zero info->addr.spaprvio.reg as evidence of default assignment, rather than needing info->addr.spaprvio.has_reg.
No 0 is a valid address. Which is a pity because has_reg is a bit ugly. But I think it's better than picking 0 or some other value as a magic "unassigned" value - that might come back to bite us.
As promised, I think this RNG matches your code, but it would need further tweaks if we declare that a valid address must always be a non-zero multiple of 0x1000.
Thanks, I'll incorporate that into v2. cheers
From: Michael Ellerman <michael@ellerman.id.au> For the PPC64 pseries machine type we need to add address information for the spapr-vty device. Signed-off-by: Michael Ellerman <michael@ellerman.id.au> --- src/qemu/qemu_command.c | 10 +++++++--- src/qemu/qemu_command.h | 1 + 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ca812de..eaef0e1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4586,7 +4586,8 @@ qemuBuildCommandLine(virConnectPtr conn, VIR_FREE(devstr); virCommandAddArg(cmd, "-device"); - if (!(devstr = qemuBuildChrDeviceStr(serial, def->os.arch, + if (!(devstr = qemuBuildChrDeviceStr(serial, qemuCaps, + def->os.arch, def->os.machine))) goto error; virCommandAddArg(cmd, devstr); @@ -5482,15 +5483,18 @@ qemuBuildCommandLine(virConnectPtr conn, */ char * qemuBuildChrDeviceStr(virDomainChrDefPtr serial, + virBitmapPtr qemuCaps, char *os_arch, char *machine) { virBuffer cmd = VIR_BUFFER_INITIALIZER; - if (STREQ(os_arch, "ppc64") && STREQ(machine, "pseries")) + if (STREQ(os_arch, "ppc64") && STREQ(machine, "pseries")) { virBufferAsprintf(&cmd, "spapr-vty,chardev=char%s", serial->info.alias); - else + if (qemuBuildDeviceAddressStr(&cmd, &serial->info, qemuCaps) < 0) + goto error; + } else virBufferAsprintf(&cmd, "isa-serial,chardev=char%s,id=%s", serial->info.alias, serial->info.alias); diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 3978b2b..c76f83a 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -56,6 +56,7 @@ virCommandPtr qemuBuildCommandLine(virConnectPtr conn, /* Generate string for arch-specific '-device' parameter */ char * qemuBuildChrDeviceStr (virDomainChrDefPtr serial, + virBitmapPtr qemuCaps, char *os_arch, char *machine); -- 1.7.7.3
On 12/07/2011 11:41 PM, Michael Ellerman wrote:
From: Michael Ellerman <michael@ellerman.id.au>
For the PPC64 pseries machine type we need to add address information for the spapr-vty device.
Signed-off-by: Michael Ellerman <michael@ellerman.id.au> --- src/qemu/qemu_command.c | 10 +++++++--- src/qemu/qemu_command.h | 1 + 2 files changed, 8 insertions(+), 3 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: Michael Ellerman <michael@ellerman.id.au> Currently qemuDomainAssignPCIAddresses() is called to assign addresses to PCI devices. We need to do something similar for devices with spapr-vio addresses. So create one place where address assignment will be done, that is qemuDomainAssignAddresses(). Signed-off-by: Michael Ellerman <michael@ellerman.id.au> --- src/qemu/qemu_command.c | 4 ++++ src/qemu/qemu_command.h | 2 ++ src/qemu/qemu_driver.c | 12 ++++++------ 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 17cc972..32ee8d8 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -811,6 +811,10 @@ cleanup: return ret; } +int qemuDomainAssignAddresses(virDomainDefPtr def) +{ + return qemuDomainAssignPCIAddresses(def); +} static void qemuDomainPCIAddressSetFreeEntry(void *payload, diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index c76f83a..de61cf3 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -174,6 +174,8 @@ virDomainDefPtr qemuParseCommandLinePid(virCapsPtr caps, virDomainChrSourceDefPtr *monConfig, bool *monJSON); +int qemuDomainAssignAddresses(virDomainDefPtr def); + int qemuDomainAssignPCIAddresses(virDomainDefPtr def); qemuDomainPCIAddressSetPtr qemuDomainPCIAddressSetCreate(virDomainDefPtr def); int qemuDomainPCIAddressReserveFunction(qemuDomainPCIAddressSetPtr addrs, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1e5ed9a..10a289e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1317,7 +1317,7 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, if (qemudCanonicalizeMachine(driver, def) < 0) goto cleanup; - if (qemuDomainAssignPCIAddresses(def) < 0) + if (qemuDomainAssignAddresses(def) < 0) goto cleanup; if (!(vm = virDomainAssignDef(driver->caps, @@ -4903,7 +4903,7 @@ static virDomainPtr qemudDomainDefine(virConnectPtr conn, const char *xml) { if (qemudCanonicalizeMachine(driver, def) < 0) goto cleanup; - if (qemuDomainAssignPCIAddresses(def) < 0) + if (qemuDomainAssignAddresses(def) < 0) goto cleanup; if (!(vm = virDomainAssignDef(driver->caps, @@ -5372,7 +5372,7 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) if (virDomainDefAddImplicitControllers(vmdef) < 0) return -1; - if (qemuDomainAssignPCIAddresses(vmdef) < 0) + if (qemuDomainAssignAddresses(vmdef) < 0) return -1; break; @@ -5390,7 +5390,7 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, return -1; } dev->data.net = NULL; - if (qemuDomainAssignPCIAddresses(vmdef) < 0) + if (qemuDomainAssignAddresses(vmdef) < 0) return -1; break; @@ -5526,7 +5526,7 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef, vmdef->nets[pos] = net; dev->data.net = NULL; - if (qemuDomainAssignPCIAddresses(vmdef) < 0) + if (qemuDomainAssignAddresses(vmdef) < 0) return -1; break; @@ -10794,7 +10794,7 @@ static virDomainPtr qemuDomainAttach(virConnectPtr conn, if (qemudCanonicalizeMachine(driver, def) < 0) goto cleanup; - if (qemuDomainAssignPCIAddresses(def) < 0) + if (qemuDomainAssignAddresses(def) < 0) goto cleanup; if (!(vm = virDomainAssignDef(driver->caps, -- 1.7.7.3
On 12/07/2011 11:41 PM, Michael Ellerman wrote:
From: Michael Ellerman <michael@ellerman.id.au>
Currently qemuDomainAssignPCIAddresses() is called to assign addresses to PCI devices.
We need to do something similar for devices with spapr-vio addresses. So create one place where address assignment will be done, that is qemuDomainAssignAddresses().
Signed-off-by: Michael Ellerman <michael@ellerman.id.au> --- src/qemu/qemu_command.c | 4 ++++ src/qemu/qemu_command.h | 2 ++ src/qemu/qemu_driver.c | 12 ++++++------ 3 files changed, 12 insertions(+), 6 deletions(-)
ACK, and in fact, this one is independent and small enough that I went ahead and pushed it (leaving just 4 and 6 in your series needing a v2). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: Michael Ellerman <michael@ellerman.id.au> Add logic to assign addresses for devices with spapr-vio addresses. We also do validation of addresses specified by the user, ie. ensuring that there are not duplicate addresses on the bus. Signed-off-by: Michael Ellerman <michael@ellerman.id.au> --- src/qemu/qemu_command.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 87 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 32ee8d8..62a1258 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -684,6 +684,87 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virBitmapPtr qemuCaps) return -1; } +static int +qemuSpaprVIOFindByReg(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, void *opaque) +{ + virDomainDeviceInfoPtr target = (virDomainDeviceInfoPtr)opaque; + + if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) + return 0; + + /* Match a dev that has a reg, is not us, and has a matching reg */ + if (info->addr.spaprvio.has_reg && info != target && + info->addr.spaprvio.reg == target->addr.spaprvio.reg) + /* Has to be < 0 so virDomainDeviceInfoIterate() will exit */ + return -1; + + return 0; +} + +static int +qemuAssignSpaprVIOAddress(virDomainDefPtr def, virDomainDeviceInfoPtr info, + unsigned long long default_reg) +{ + bool user_reg; + int rc; + + if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) + return 0; + + /* Check if the user has assigned the reg already, if so use it */ + user_reg = info->addr.spaprvio.has_reg; + if (!user_reg) { + info->addr.spaprvio.reg = default_reg; + info->addr.spaprvio.has_reg = true; + } + + rc = virDomainDeviceInfoIterate(def, qemuSpaprVIOFindByReg, info); + while (rc != 0) { + if (user_reg) { + qemuReportError(VIR_ERR_XML_ERROR, + _("spapr-vio address %#llx already in use"), + info->addr.spaprvio.reg); + return -EEXIST; + } + + /* We assigned the reg, so try a new value */ + info->addr.spaprvio.reg += 0x1000; + rc = virDomainDeviceInfoIterate(def, qemuSpaprVIOFindByReg, info); + } + + return 0; +} + +static int qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def) +{ + int i, rc; + + for (i = 0 ; i < def->nnets; i++) { + rc = qemuAssignSpaprVIOAddress(def, &def->nets[i]->info, + 0x1000ul); + if (rc) + return rc; + } + + for (i = 0 ; i < def->ncontrollers; i++) { + rc = qemuAssignSpaprVIOAddress(def, &def->controllers[i]->info, + 0x2000ul); + if (rc) + return rc; + } + + for (i = 0 ; i < def->nserials; i++) { + rc = qemuAssignSpaprVIOAddress(def, &def->serials[i]->info, + 0x30000000ul); + if (rc) + return rc; + } + + /* No other devices are currently supported on spapr-vio */ + + return 0; +} #define QEMU_PCI_ADDRESS_LAST_SLOT 31 #define QEMU_PCI_ADDRESS_LAST_FUNCTION 8 @@ -813,6 +894,12 @@ cleanup: int qemuDomainAssignAddresses(virDomainDefPtr def) { + int rc; + + rc = qemuDomainAssignSpaprVIOAddresses(def); + if (rc) + return rc; + return qemuDomainAssignPCIAddresses(def); } -- 1.7.7.3
On 12/07/2011 11:41 PM, Michael Ellerman wrote:
From: Michael Ellerman <michael@ellerman.id.au>
Add logic to assign addresses for devices with spapr-vio addresses.
We also do validation of addresses specified by the user, ie. ensuring that there are not duplicate addresses on the bus.
Signed-off-by: Michael Ellerman <michael@ellerman.id.au> --- src/qemu/qemu_command.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 87 insertions(+), 0 deletions(-)
Depends on patch 4, but I'll review anyway...
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 32ee8d8..62a1258 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -684,6 +684,87 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virBitmapPtr qemuCaps) return -1; }
+static int +qemuSpaprVIOFindByReg(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, void *opaque) +{ + virDomainDeviceInfoPtr target = (virDomainDeviceInfoPtr)opaque;
The cast isn't strictly necessary in C.
+ + if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) + return 0; + + /* Match a dev that has a reg, is not us, and has a matching reg */ + if (info->addr.spaprvio.has_reg && info != target &&
As I asked in 4, can a valid reg ever be 0, or must it be non-zero? If the latter, then you don't need has_reg.
+ info->addr.spaprvio.reg == target->addr.spaprvio.reg) + /* Has to be < 0 so virDomainDeviceInfoIterate() will exit */ + return -1; + + return 0; +} + +static int +qemuAssignSpaprVIOAddress(virDomainDefPtr def, virDomainDeviceInfoPtr info, + unsigned long long default_reg) +{ + bool user_reg; + int rc; + + if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) + return 0; + + /* Check if the user has assigned the reg already, if so use it */ + user_reg = info->addr.spaprvio.has_reg; + if (!user_reg) { + info->addr.spaprvio.reg = default_reg; + info->addr.spaprvio.has_reg = true; + } + + rc = virDomainDeviceInfoIterate(def, qemuSpaprVIOFindByReg, info); + while (rc != 0) { + if (user_reg) { + qemuReportError(VIR_ERR_XML_ERROR, + _("spapr-vio address %#llx already in use"), + info->addr.spaprvio.reg); + return -EEXIST; + } + + /* We assigned the reg, so try a new value */ + info->addr.spaprvio.reg += 0x1000; + rc = virDomainDeviceInfoIterate(def, qemuSpaprVIOFindByReg, info); + }
Looks reasonable at either honoring the user's choice, or at iterating by 0x1000 until a free slot is found. Does that match the qemu algorithm?
+ + return 0; +} + +static int qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def) +{ + int i, rc; + + for (i = 0 ; i < def->nnets; i++) { + rc = qemuAssignSpaprVIOAddress(def, &def->nets[i]->info, + 0x1000ul); + if (rc) + return rc; + } + + for (i = 0 ; i < def->ncontrollers; i++) { + rc = qemuAssignSpaprVIOAddress(def, &def->controllers[i]->info, + 0x2000ul); + if (rc) + return rc; + } + + for (i = 0 ; i < def->nserials; i++) { + rc = qemuAssignSpaprVIOAddress(def, &def->serials[i]->info, + 0x30000000ul);
Again, I assume these three defaults match qemu. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, 2011-12-09 at 15:14 -0700, Eric Blake wrote:
On 12/07/2011 11:41 PM, Michael Ellerman wrote:
From: Michael Ellerman <michael@ellerman.id.au>
Add logic to assign addresses for devices with spapr-vio addresses.
We also do validation of addresses specified by the user, ie. ensuring that there are not duplicate addresses on the bus.
Signed-off-by: Michael Ellerman <michael@ellerman.id.au> --- src/qemu/qemu_command.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 87 insertions(+), 0 deletions(-)
Depends on patch 4, but I'll review anyway...
Thanks.
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 32ee8d8..62a1258 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -684,6 +684,87 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virBitmapPtr qemuCaps) return -1; }
+static int +qemuSpaprVIOFindByReg(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, void *opaque) +{ + virDomainDeviceInfoPtr target = (virDomainDeviceInfoPtr)opaque;
The cast isn't strictly necessary in C.
Will fix.
+ if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) + return 0; + + /* Match a dev that has a reg, is not us, and has a matching reg */ + if (info->addr.spaprvio.has_reg && info != target &&
As I asked in 4, can a valid reg ever be 0, or must it be non-zero? If the latter, then you don't need has_reg.
Zero is valid, so as I said I think has_reg is the best solution.
+ /* Check if the user has assigned the reg already, if so use it */ + user_reg = info->addr.spaprvio.has_reg; + if (!user_reg) { + info->addr.spaprvio.reg = default_reg; + info->addr.spaprvio.has_reg = true; + } + + rc = virDomainDeviceInfoIterate(def, qemuSpaprVIOFindByReg, info); + while (rc != 0) { + if (user_reg) { + qemuReportError(VIR_ERR_XML_ERROR, + _("spapr-vio address %#llx already in use"), + info->addr.spaprvio.reg); + return -EEXIST; + } + + /* We assigned the reg, so try a new value */ + info->addr.spaprvio.reg += 0x1000; + rc = virDomainDeviceInfoIterate(def, qemuSpaprVIOFindByReg, info); + }
Looks reasonable at either honoring the user's choice, or at iterating by 0x1000 until a free slot is found. Does that match the qemu algorithm?
Actually we're planning on not doing anything in QEMU. So this will be the only code that does magical address assignment. Users who want to use bare QEMU and specify their own devices will just have to assign reg values themselves.
+static int qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def) +{ + int i, rc; + + for (i = 0 ; i < def->nnets; i++) { + rc = qemuAssignSpaprVIOAddress(def, &def->nets[i]->info, + 0x1000ul); + if (rc) + return rc; + } + + for (i = 0 ; i < def->ncontrollers; i++) { + rc = qemuAssignSpaprVIOAddress(def, &def->controllers[i]->info, + 0x2000ul); + if (rc) + return rc; + } + + for (i = 0 ; i < def->nserials; i++) { + rc = qemuAssignSpaprVIOAddress(def, &def->serials[i]->info, + 0x30000000ul);
Again, I assume these three defaults match qemu.
Yeah they do. I'll add a comment to that effect. cheers
On 12/07/2011 11:41 PM, Michael Ellerman wrote:
From: Michael Ellerman <michael@ellerman.id.au>
In QEMU PPC64 we have a network device called "spapr-vlan". We can specify this using the existing syntax for network devices, however libvirt currently rejects "spapr-vlan" in virDomainNetDefParseXML() because of the "-". Fix the code to accept "-".
Thanks for contributing! I ran out of time to fully review this today, but at least this one looks promising.
Signed-off-by: Michael Ellerman <michael@ellerman.id.au> --- src/conf/domain_conf.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 75e51a0..5de33b9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3703,8 +3703,7 @@ virDomainNetDefParseXML(virCapsPtr caps, if (model != NULL) { int i; for (i = 0 ; i < strlen(model) ; i++) { - int char_ok = c_isalnum(model[i]) || model[i] == '_'; - if (!char_ok) { + if (!c_isalnum(model[i]) && model[i] != '_' && model[i] != '-') {
I'm not sure if we need to tweak our RNG grammar to also allow this in the XML validation. I'll check that out tomorrow, when I get around to applying this one and reviewing the rest of the series. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, 2011-12-08 at 17:38 -0700, Eric Blake wrote:
On 12/07/2011 11:41 PM, Michael Ellerman wrote:
From: Michael Ellerman <michael@ellerman.id.au>
In QEMU PPC64 we have a network device called "spapr-vlan". We can specify this using the existing syntax for network devices, however libvirt currently rejects "spapr-vlan" in virDomainNetDefParseXML() because of the "-". Fix the code to accept "-".
Thanks for contributing! I ran out of time to fully review this today, but at least this one looks promising.
No worries.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 75e51a0..5de33b9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3703,8 +3703,7 @@ virDomainNetDefParseXML(virCapsPtr caps, if (model != NULL) { int i; for (i = 0 ; i < strlen(model) ; i++) { - int char_ok = c_isalnum(model[i]) || model[i] == '_'; - if (!char_ok) { + if (!c_isalnum(model[i]) && model[i] != '_' && model[i] != '-') {
I'm not sure if we need to tweak our RNG grammar to also allow this in the XML validation. I'll check that out tomorrow, when I get around to applying this one and reviewing the rest of the series.
OK. It looks to me like we don't, but I don't know much about RNG so I'll defer to you. cheers
On 12/08/2011 05:38 PM, Eric Blake wrote:
int i; for (i = 0 ; i < strlen(model) ; i++) {
Hmm - an O(n) function call on an O(n) loop, for a quadratic action (of course, it's in the noise, since the user's model name is likely short). But we can do better with a more efficient search for bogus bytes (strspn is O(n), if implemented well in libc).
- int char_ok = c_isalnum(model[i]) || model[i] == '_'; - if (!char_ok) { + if (!c_isalnum(model[i]) && model[i] != '_' && model[i] != '-') {
I'm not sure if we need to tweak our RNG grammar to also allow this in the XML validation. I'll check that out tomorrow, when I get around to applying this one and reviewing the rest of the series.
It turns out the XML didn't do any validation at all. Here's what I came up with - tightening the RNG and relaxing the domain_conf code, so that they now match. Since the concept is the same as yours, I went ahead and pushed it, but I claimed authorship on this one, since I practically rewrote it. diff --git i/docs/schemas/domaincommon.rng w/docs/schemas/domaincommon.rng index b8fbcf9..27ec1da 100644 --- i/docs/schemas/domaincommon.rng +++ w/docs/schemas/domaincommon.rng @@ -1276,7 +1276,11 @@ </optional> <optional> <element name="model"> - <attribute name="type"/> + <attribute name="type"> + <data type="string"> + <param name='pattern'>[a-zA-Z0-9\-_]+</param> + </data> + </attribute> <empty/> </element> </optional> diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c index 5de33b9..f3ec777 100644 --- i/src/conf/domain_conf.c +++ w/src/conf/domain_conf.c @@ -3385,6 +3385,9 @@ error: return ret; } +#define NET_MODEL_CHARS \ + "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ091234567890_-" + /* Parse the XML definition for a network interface * @param node XML nodeset to parse for net definition * @return 0 on success, -1 on failure @@ -3699,15 +3702,13 @@ virDomainNetDefParseXML(virCapsPtr caps, * reasonable, not that it is a supported NIC type. FWIW kvm * supports these types as of April 2008: * i82551 i82557b i82559er ne2k_pci pcnet rtl8139 e1000 virtio + * QEMU PPC64 supports spapr-vlan */ if (model != NULL) { - int i; - for (i = 0 ; i < strlen(model) ; i++) { - if (!c_isalnum(model[i]) && model[i] != '_' && model[i] != '-') { - virDomainReportError(VIR_ERR_INVALID_ARG, "%s", - _("Model name contains invalid characters")); - goto error; - } + if (strspn(model, NET_MODEL_CHARS) < strlen(model)) { + virDomainReportError(VIR_ERR_INVALID_ARG, "%s", + _("Model name contains invalid characters")); + goto error; } def->model = model; model = NULL; -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, 2011-12-09 at 12:34 -0700, Eric Blake wrote:
On 12/08/2011 05:38 PM, Eric Blake wrote:
int i; for (i = 0 ; i < strlen(model) ; i++) {
Hmm - an O(n) function call on an O(n) loop, for a quadratic action (of course, it's in the noise, since the user's model name is likely short). But we can do better with a more efficient search for bogus bytes (strspn is O(n), if implemented well in libc).
- int char_ok = c_isalnum(model[i]) || model[i] == '_'; - if (!char_ok) { + if (!c_isalnum(model[i]) && model[i] != '_' && model[i] != '-') {
I'm not sure if we need to tweak our RNG grammar to also allow this in the XML validation. I'll check that out tomorrow, when I get around to applying this one and reviewing the rest of the series.
It turns out the XML didn't do any validation at all. Here's what I came up with - tightening the RNG and relaxing the domain_conf code, so that they now match. Since the concept is the same as yours, I went ahead and pushed it, but I claimed authorship on this one, since I practically rewrote it.
Yep fair enough. Your new version is much nicer, looks good. cheers
participants (3)
-
Eric Blake -
Michael Ellerman -
Michael Ellerman