[libvirt] [PATCH v1 00/14] Never ending story of user supplied aliases
As discussed earlier [1], we should allow users to set device aliases at the define time. While the discussed approach calls for generating missing aliases too, I'm saving that for another patch set. There are couple of reasons for that: a) I don't think it's really necessary (if users are interested in a device they can just set the alias). b) This patch set is already big enough as is. c) Generating aliases is going to have its own problems. Therefore, for now I'm only proposing parsing user supplied aliases. The idea is that it's not enabled by default for all drivers. Any driver that want to/can provide this feature has to set VIR_DOMAIN_DEF_FEATURE_USER_ALIAS. Note that we have some drivers that don't have notion of device aliases. But the code is generic enough so that it should be easy to use in the drivers that do know what aliases are. 1: https://www.redhat.com/archives/libvir-list/2017-October/msg00201.html Michal Privoznik (14): conf: Fix virDomainDeviceGetInfo const correctness virDomainObjGetOneDefState: Fix error message qemuAssignDeviceAliases: Use qemuAssignDeviceRNGAlias for assigning RNG aliases qemu: Move device alias assignment to separate functions qemu: Be tolerant to preexisting aliases conf: Pass xmlopt down to virDomainDeviceInfoParseXML conf: Parse user supplied aliases conf: Validate user supplied aliases virDomainDeviceInfoCheckABIStability: Check for alias too qemuxml2argvdata: Drop device aliases qemuhotplugtest: Load active XML conf: Format alias even for inactive XMLs docs: Document user aliases qemu: Parse alias from inactive XMLs docs/formatdomain.html.in | 23 ++ src/conf/domain_conf.c | 353 ++++++++++++++++----- src/conf/domain_conf.h | 8 +- src/libvirt_private.syms | 1 + src/qemu/qemu_alias.c | 139 +++++++- src/qemu/qemu_domain.c | 3 +- src/qemu/qemu_driver.c | 3 + src/qemu/qemu_hotplug.c | 6 +- tests/qemuhotplugtest.c | 3 +- .../qemuxml2argv-disk-cdrom-network-ftp.xml | 1 - .../qemuxml2argv-disk-cdrom-network-ftps.xml | 1 - .../qemuxml2argv-disk-cdrom-network-http.xml | 1 - .../qemuxml2argv-disk-cdrom-network-https.xml | 1 - .../qemuxml2argv-disk-cdrom-network-tftp.xml | 1 - .../qemuxml2argv-usb-redir-filter.xml | 1 - 15 files changed, 444 insertions(+), 101 deletions(-) -- 2.13.6
This function is not changing passed domain definition. Therefore, mark the argument as 'const'. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 2 +- src/conf/domain_conf.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 25d48f977..31684df54 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3501,7 +3501,7 @@ int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info, } virDomainDeviceInfoPtr -virDomainDeviceGetInfo(virDomainDeviceDefPtr device) +virDomainDeviceGetInfo(const virDomainDeviceDef *device) { switch ((virDomainDeviceType) device->type) { case VIR_DOMAIN_DEVICE_DISK: diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a42efcfa6..1e007346f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2756,7 +2756,7 @@ virDomainDeviceDefPtr virDomainDeviceDefCopy(virDomainDeviceDefPtr src, virDomainXMLOptionPtr xmlopt); int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info, int type); -virDomainDeviceInfoPtr virDomainDeviceGetInfo(virDomainDeviceDefPtr device); +virDomainDeviceInfoPtr virDomainDeviceGetInfo(const virDomainDeviceDef *device); void virDomainTPMDefFree(virDomainTPMDefPtr def); typedef int (*virDomainDeviceInfoCallback)(virDomainDefPtr def, -- 2.13.6
On Thu, Oct 19, 2017 at 10:10:56AM +0200, Michal Privoznik wrote:
This function is not changing passed domain definition. Therefore, mark the argument as 'const'.
Is there any other reason for this then just your new code using const? I don't see any pointer that's passed through your later code that would be const nowadays, so why would we play the const-correctness game here when it doesn't make much sense in C when the pointer is to a struct since the const doesn't propagate? If s/const virDomainDeviceDef/virDomainDeviceDef/ on your later patch makes this one unnecessary, then NACK to this. If there is a reason, however, feel free to keep this in.
On 10/19/2017 11:55 AM, Martin Kletzander wrote:
On Thu, Oct 19, 2017 at 10:10:56AM +0200, Michal Privoznik wrote:
This function is not changing passed domain definition. Therefore, mark the argument as 'const'.
Is there any other reason for this then just your new code using const? I don't see any pointer that's passed through your later code that would be const nowadays, so why would we play the const-correctness game here when it doesn't make much sense in C when the pointer is to a struct since the const doesn't propagate?
If s/const virDomainDeviceDef/virDomainDeviceDef/ on your later patch makes this one unnecessary, then NACK to this. If there is a reason, however, feel free to keep this in.
Well, later in 08/14 I am indeed calling this from function which takes const virDomainDeviceDef * and I can change it to virDomainDeviceDefPtr. And while I think this patch still makes things better (solves const correctness), I don't want it to be show stopper. Thus I'm dropping it. Michal
It looks like the error message was copied from virsh, because that's where we have @ctl. Nevertheless, it's @flags which is invalid, not @ctl. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 31684df54..b3feddc39 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3418,12 +3418,13 @@ virDomainObjGetOneDefState(virDomainObjPtr vm, unsigned int flags, bool *live) { - if (flags & VIR_DOMAIN_AFFECT_LIVE && flags & VIR_DOMAIN_AFFECT_CONFIG) { - virReportInvalidArg(ctl, "%s", - _("Flags 'VIR_DOMAIN_AFFECT_LIVE' and " - "'VIR_DOMAIN_AFFECT_CONFIG' are mutually " - "exclusive")); - return NULL; + if (flags & VIR_DOMAIN_AFFECT_LIVE && + flags & VIR_DOMAIN_AFFECT_CONFIG) { + virReportInvalidArg(flags, "%s", + _("Flags 'VIR_DOMAIN_AFFECT_LIVE' and " + "'VIR_DOMAIN_AFFECT_CONFIG' are mutually " + "exclusive")); + return NULL; } if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) -- 2.13.6
We have a special function for assigning aliases to RNG devices. Use that instead of plain virAsprintf(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_alias.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 72df1083f..eca1dc64c 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -501,7 +501,7 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) return -1; } for (i = 0; i < def->nrngs; i++) { - if (virAsprintf(&def->rngs[i]->info.alias, "rng%zu", i) < 0) + if (qemuAssignDeviceRNGAlias(def, def->rngs[i]) < 0) return -1; } if (def->tpm) { -- 2.13.6
Let's move all the virAsprintf()-s into separate functions for better structure of the code. Later, when somebody wants to generate a device alias, all they need is to expose the function. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_alias.c | 80 +++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 72 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index eca1dc64c..33bd4c10d 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -286,6 +286,70 @@ qemuAssignDeviceNetAlias(virDomainDefPtr def, } +static int +qemuAssignDeviceFSAlias(virDomainFSDefPtr fss, + int idx) +{ + return virAsprintf(&fss->info.alias, "fs%d", idx); +} + + +static int +qemuAssignDeviceSoundAlias(virDomainSoundDefPtr sound, + int idx) +{ + return virAsprintf(&sound->info.alias, "sound%d", idx); +} + + +static int +qemuAssignDeviceVideoAlias(virDomainVideoDefPtr video, + int idx) +{ + return virAsprintf(&video->info.alias, "video%d", idx); +} + + +static int +qemuAssignDeviceInputAlias(virDomainInputDefPtr input, + int idx) +{ + return virAsprintf(&input->info.alias, "input%d", idx); +} + + +static int +qemuAssignDeviceHubAlias(virDomainHubDefPtr hub, + int idx) +{ + return virAsprintf(&hub->info.alias, "hub%d", idx); +} + + +static int +qemuAssignDeviceSmartcardAlias(virDomainSmartcardDefPtr smartcard, + int idx) +{ + return virAsprintf(&smartcard->info.alias, "smartcard%d", idx); +} + + +static int +qemuAssingDeviceMemballoonAlias(virDomainMemballoonDefPtr memballoon, + int idx) +{ + return virAsprintf(&memballoon->info.alias, "balloon%d", idx); +} + + +static int +qemuAssignDeviceTPMAlias(virDomainTPMDefPtr tpm, + int idx) +{ + return virAsprintf(&tpm->info.alias, "tpm%d", idx); +} + + int qemuAssignDeviceRedirdevAlias(virDomainDefPtr def, virDomainRedirdevDefPtr redirdev, @@ -431,11 +495,11 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) } for (i = 0; i < def->nfss; i++) { - if (virAsprintf(&def->fss[i]->info.alias, "fs%zu", i) < 0) + if (qemuAssignDeviceFSAlias(def->fss[i], i) < 0) return -1; } for (i = 0; i < def->nsounds; i++) { - if (virAsprintf(&def->sounds[i]->info.alias, "sound%zu", i) < 0) + if (qemuAssignDeviceSoundAlias(def->sounds[i], i) < 0) return -1; } for (i = 0; i < def->nhostdevs; i++) { @@ -453,7 +517,7 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) return -1; } for (i = 0; i < def->nvideos; i++) { - if (virAsprintf(&def->videos[i]->info.alias, "video%zu", i) < 0) + if (qemuAssignDeviceVideoAlias(def->videos[i], i) < 0) return -1; } for (i = 0; i < def->ncontrollers; i++) { @@ -461,7 +525,7 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) return -1; } for (i = 0; i < def->ninputs; i++) { - if (virAsprintf(&def->inputs[i]->info.alias, "input%zu", i) < 0) + if (qemuAssignDeviceInputAlias(def->inputs[i], i) < 0) return -1; } for (i = 0; i < def->nparallels; i++) { @@ -481,7 +545,7 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) return -1; } for (i = 0; i < def->nhubs; i++) { - if (virAsprintf(&def->hubs[i]->info.alias, "hub%zu", i) < 0) + if (qemuAssignDeviceHubAlias(def->hubs[i], i) < 0) return -1; } for (i = 0; i < def->nshmems; i++) { @@ -489,7 +553,7 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) return -1; } for (i = 0; i < def->nsmartcards; i++) { - if (virAsprintf(&def->smartcards[i]->info.alias, "smartcard%zu", i) < 0) + if (qemuAssignDeviceSmartcardAlias(def->smartcards[i], i) < 0) return -1; } if (def->watchdog) { @@ -497,7 +561,7 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) return -1; } if (def->memballoon) { - if (virAsprintf(&def->memballoon->info.alias, "balloon%d", 0) < 0) + if (qemuAssingDeviceMemballoonAlias(def->memballoon, 0) < 0) return -1; } for (i = 0; i < def->nrngs; i++) { @@ -505,7 +569,7 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) return -1; } if (def->tpm) { - if (virAsprintf(&def->tpm->info.alias, "tpm%d", 0) < 0) + if (qemuAssignDeviceTPMAlias(def->tpm, 0) < 0) return -1; } for (i = 0; i < def->nmems; i++) { -- 2.13.6
On Thu, Oct 19, 2017 at 10:10:59AM +0200, Michal Privoznik wrote:
Let's move all the virAsprintf()-s into separate functions for better structure of the code. Later, when somebody wants to generate a device alias, all they need is to expose the function.
I'm just waiting for these functions to totally fail when we introduce a hotplug of such device. There could be a universal wrapper for these, but that's not the scope of this patchset. ACK
In the future, some aliases might be already parsed therefore we should avoid overwriting them. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_alias.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++-- src/qemu/qemu_hotplug.c | 6 ++---- 2 files changed, 57 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 33bd4c10d..7a3983afa 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -93,6 +93,9 @@ qemuAssignDeviceChrAlias(virDomainDefPtr def, { const char *prefix = NULL; + if (chr->info.alias) + return 0; + switch ((virDomainChrDeviceType) chr->deviceType) { case VIR_DOMAIN_CHR_DEVICE_TYPE_PARALLEL: prefix = "parallel"; @@ -128,6 +131,9 @@ qemuAssignDeviceControllerAlias(virDomainDefPtr domainDef, { const char *prefix = virDomainControllerTypeToString(controller->type); + if (controller->info.alias) + return 0; + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { if (!virQEMUCapsHasPCIMultiBus(qemuCaps, domainDef)) { /* qemus that don't support multiple PCI buses have @@ -182,6 +188,9 @@ qemuAssignDeviceDiskAlias(virDomainDefPtr def, const char *prefix = virDomainDiskBusTypeToString(disk->bus); int controllerModel = -1; + if (disk->info.alias) + return 0; + if (disk->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) { controllerModel = @@ -223,6 +232,9 @@ qemuAssignDeviceHostdevAlias(virDomainDefPtr def, char **alias, int idx) { + if (*alias) + return 0; + if (idx == -1) { size_t i; @@ -259,6 +271,9 @@ qemuAssignDeviceNetAlias(virDomainDefPtr def, int idx) { + if (net->info.alias) + return 0; + /* <interface type='hostdev'> uses "hostdevN" as the alias * We must use "-1" as the index because the caller doesn't know * that we're now looking for a unique hostdevN rather than netN @@ -290,6 +305,9 @@ static int qemuAssignDeviceFSAlias(virDomainFSDefPtr fss, int idx) { + if (fss->info.alias) + return 0; + return virAsprintf(&fss->info.alias, "fs%d", idx); } @@ -298,6 +316,9 @@ static int qemuAssignDeviceSoundAlias(virDomainSoundDefPtr sound, int idx) { + if (sound->info.alias) + return 0; + return virAsprintf(&sound->info.alias, "sound%d", idx); } @@ -306,6 +327,9 @@ static int qemuAssignDeviceVideoAlias(virDomainVideoDefPtr video, int idx) { + if (video->info.alias) + return 0; + return virAsprintf(&video->info.alias, "video%d", idx); } @@ -314,6 +338,9 @@ static int qemuAssignDeviceInputAlias(virDomainInputDefPtr input, int idx) { + if (input->info.alias) + return 0; + return virAsprintf(&input->info.alias, "input%d", idx); } @@ -322,6 +349,9 @@ static int qemuAssignDeviceHubAlias(virDomainHubDefPtr hub, int idx) { + if (hub->info.alias) + return 0; + return virAsprintf(&hub->info.alias, "hub%d", idx); } @@ -330,6 +360,9 @@ static int qemuAssignDeviceSmartcardAlias(virDomainSmartcardDefPtr smartcard, int idx) { + if (smartcard->info.alias) + return 0; + return virAsprintf(&smartcard->info.alias, "smartcard%d", idx); } @@ -338,6 +371,9 @@ static int qemuAssingDeviceMemballoonAlias(virDomainMemballoonDefPtr memballoon, int idx) { + if (memballoon->info.alias) + return 0; + return virAsprintf(&memballoon->info.alias, "balloon%d", idx); } @@ -346,6 +382,9 @@ static int qemuAssignDeviceTPMAlias(virDomainTPMDefPtr tpm, int idx) { + if (tpm->info.alias) + return 0; + return virAsprintf(&tpm->info.alias, "tpm%d", idx); } @@ -355,6 +394,9 @@ qemuAssignDeviceRedirdevAlias(virDomainDefPtr def, virDomainRedirdevDefPtr redirdev, int idx) { + if (redirdev->info.alias) + return 0; + if (idx == -1) { size_t i; idx = 0; @@ -384,6 +426,9 @@ qemuAssignDeviceRNGAlias(virDomainDefPtr def, int maxidx = 0; int idx; + if (rng->info.alias) + return 0; + for (i = 0; i < def->nrngs; i++) { if ((idx = qemuDomainDeviceAliasIndex(&def->rngs[i]->info, "rng")) >= maxidx) maxidx = idx + 1; @@ -418,6 +463,9 @@ qemuAssignDeviceMemoryAlias(virDomainDefPtr def, int idx; const char *prefix; + if (mem->info.alias) + return 0; + if (mem->model == VIR_DOMAIN_MEMORY_MODEL_DIMM) prefix = "dimm"; else @@ -444,6 +492,9 @@ qemuAssignDeviceShmemAlias(virDomainDefPtr def, virDomainShmemDefPtr shmem, int idx) { + if (shmem->info.alias) + return 0; + if (idx == -1) { size_t i; idx = 0; @@ -474,6 +525,9 @@ qemuAssignDeviceWatchdogAlias(virDomainWatchdogDefPtr watchdog) { /* Currently, there's just one watchdog per domain */ + if (watchdog->info.alias) + return 0; + if (VIR_STRDUP(watchdog->info.alias, "watchdog0") < 0) return -1; return 0; @@ -508,8 +562,7 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) * linked to a NetDef, they will share an info and the alias * will already be set, so don't try to set it again. */ - if (!def->hostdevs[i]->info->alias && - qemuAssignDeviceHostdevAlias(def, &def->hostdevs[i]->info->alias, -1) < 0) + if (qemuAssignDeviceHostdevAlias(def, &def->hostdevs[i]->info->alias, -1) < 0) return -1; } for (i = 0; i < def->nredirdevs; i++) { diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 0288986d8..d556c9be0 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4980,10 +4980,8 @@ qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver, { int ret = -1; - if (!detach->info->alias) { - if (qemuAssignDeviceHostdevAlias(vm->def, &detach->info->alias, -1) < 0) - return -1; - } + if (qemuAssignDeviceHostdevAlias(vm->def, &detach->info->alias, -1) < 0) + return -1; switch (detach->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: -- 2.13.6
This function is going to make decisions based on the features set per each driver. For that we need the virDomainXMLOption object. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 165 ++++++++++++++++++++++++++++++------------------- 1 file changed, 102 insertions(+), 63 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b3feddc39..f4de4e288 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6402,7 +6402,8 @@ virDomainDeviceAddressParseXML(xmlNodePtr address, * @param node XML nodeset to parse for device address definition */ static int -virDomainDeviceInfoParseXML(xmlNodePtr node, +virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED, + xmlNodePtr node, virHashTablePtr bootHash, virDomainDeviceInfoPtr info, unsigned int flags) @@ -9177,7 +9178,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } def->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; } else { - if (virDomainDeviceInfoParseXML(node, bootHash, &def->info, + if (virDomainDeviceInfoParseXML(xmlopt, node, bootHash, &def->info, flags | VIR_DOMAIN_DEF_PARSE_ALLOW_BOOT) < 0) goto error; } @@ -9514,7 +9515,8 @@ virDomainControllerModelTypeToString(virDomainControllerDefPtr def, * @param node XML nodeset to parse for controller definition */ static virDomainControllerDefPtr -virDomainControllerDefParseXML(xmlNodePtr node, +virDomainControllerDefParseXML(virDomainXMLOptionPtr xmlopt, + xmlNodePtr node, xmlXPathContextPtr ctxt, unsigned int flags) { @@ -9663,7 +9665,8 @@ virDomainControllerDefParseXML(xmlNodePtr node, if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && def->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE) { VIR_DEBUG("Ignoring device address for none model usb controller"); - } else if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) { + } else if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, + &def->info, flags) < 0) { goto error; } @@ -9890,7 +9893,8 @@ virDomainNetGenerateMAC(virDomainXMLOptionPtr xmlopt, * @param node XML nodeset to parse for disk definition */ static virDomainFSDefPtr -virDomainFSDefParseXML(xmlNodePtr node, +virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt, + xmlNodePtr node, xmlXPathContextPtr ctxt, unsigned int flags) { @@ -10047,7 +10051,7 @@ virDomainFSDefParseXML(xmlNodePtr node, def->dst = target; target = NULL; - if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) + if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, &def->info, flags) < 0) goto error; cleanup: @@ -10568,7 +10572,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, } def->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; } else { - if (virDomainDeviceInfoParseXML(node, bootHash, &def->info, + if (virDomainDeviceInfoParseXML(xmlopt, node, bootHash, &def->info, flags | VIR_DOMAIN_DEF_PARSE_ALLOW_BOOT | VIR_DOMAIN_DEF_PARSE_ALLOW_ROM) < 0) goto error; @@ -11835,7 +11839,8 @@ virDomainChrDefParseXML(virDomainXMLOptionPtr xmlopt, if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL && def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD) { VIR_DEBUG("Ignoring device address for gustfwd channel"); - } else if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) { + } else if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, + &def->info, flags) < 0) { goto error; } @@ -11972,7 +11977,7 @@ virDomainSmartcardDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } - if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) + if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, &def->info, flags) < 0) goto error; if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCID) { @@ -12005,7 +12010,8 @@ virDomainSmartcardDefParseXML(virDomainXMLOptionPtr xmlopt, * */ static virDomainTPMDefPtr -virDomainTPMDefParseXML(xmlNodePtr node, +virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt, + xmlNodePtr node, xmlXPathContextPtr ctxt, unsigned int flags) { @@ -12074,7 +12080,7 @@ virDomainTPMDefParseXML(xmlNodePtr node, goto error; } - if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) + if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, &def->info, flags) < 0) goto error; cleanup: @@ -12093,7 +12099,8 @@ virDomainTPMDefParseXML(xmlNodePtr node, } static virDomainPanicDefPtr -virDomainPanicDefParseXML(xmlNodePtr node, +virDomainPanicDefParseXML(virDomainXMLOptionPtr xmlopt, + xmlNodePtr node, unsigned int flags) { virDomainPanicDefPtr panic; @@ -12102,7 +12109,8 @@ virDomainPanicDefParseXML(xmlNodePtr node, if (VIR_ALLOC(panic) < 0) return NULL; - if (virDomainDeviceInfoParseXML(node, NULL, &panic->info, flags) < 0) + if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, + &panic->info, flags) < 0) goto error; model = virXMLPropString(node, "model"); @@ -12125,7 +12133,8 @@ virDomainPanicDefParseXML(xmlNodePtr node, /* Parse the XML definition for an input device */ static virDomainInputDefPtr -virDomainInputDefParseXML(const virDomainDef *dom, +virDomainInputDefParseXML(virDomainXMLOptionPtr xmlopt, + const virDomainDef *dom, xmlNodePtr node, xmlXPathContextPtr ctxt, unsigned int flags) @@ -12236,7 +12245,7 @@ virDomainInputDefParseXML(const virDomainDef *dom, } } - if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) + if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, &def->info, flags) < 0) goto error; if (def->bus == VIR_DOMAIN_INPUT_BUS_USB && @@ -12276,7 +12285,9 @@ virDomainInputDefParseXML(const virDomainDef *dom, /* Parse the XML definition for a hub device */ static virDomainHubDefPtr -virDomainHubDefParseXML(xmlNodePtr node, unsigned int flags) +virDomainHubDefParseXML(virDomainXMLOptionPtr xmlopt, + xmlNodePtr node, + unsigned int flags) { virDomainHubDefPtr def; char *type = NULL; @@ -12298,7 +12309,7 @@ virDomainHubDefParseXML(xmlNodePtr node, unsigned int flags) goto error; } - if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) + if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, &def->info, flags) < 0) goto error; cleanup: @@ -13375,7 +13386,8 @@ virDomainSoundCodecDefParseXML(xmlNodePtr node) static virDomainSoundDefPtr -virDomainSoundDefParseXML(xmlNodePtr node, +virDomainSoundDefParseXML(virDomainXMLOptionPtr xmlopt, + xmlNodePtr node, xmlXPathContextPtr ctxt, unsigned int flags) { @@ -13427,7 +13439,7 @@ virDomainSoundDefParseXML(xmlNodePtr node, } } - if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) + if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, &def->info, flags) < 0) goto error; cleanup: @@ -13444,7 +13456,8 @@ virDomainSoundDefParseXML(xmlNodePtr node, static virDomainWatchdogDefPtr -virDomainWatchdogDefParseXML(xmlNodePtr node, +virDomainWatchdogDefParseXML(virDomainXMLOptionPtr xmlopt, + xmlNodePtr node, unsigned int flags) { @@ -13480,7 +13493,7 @@ virDomainWatchdogDefParseXML(xmlNodePtr node, } } - if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) + if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, &def->info, flags) < 0) goto error; cleanup: @@ -13592,7 +13605,7 @@ virDomainRNGDefParseXML(virDomainXMLOptionPtr xmlopt, break; } - if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) + if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, &def->info, flags) < 0) goto error; if (virDomainVirtioOptionsParseXML(virXPathNode("./driver", ctxt), @@ -13615,7 +13628,8 @@ virDomainRNGDefParseXML(virDomainXMLOptionPtr xmlopt, static virDomainMemballoonDefPtr -virDomainMemballoonDefParseXML(xmlNodePtr node, +virDomainMemballoonDefParseXML(virDomainXMLOptionPtr xmlopt, + xmlNodePtr node, xmlXPathContextPtr ctxt, unsigned int flags) { @@ -13661,7 +13675,8 @@ virDomainMemballoonDefParseXML(xmlNodePtr node, if (def->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE) VIR_DEBUG("Ignoring device address for none model Memballoon"); - else if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) + else if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, + &def->info, flags) < 0) goto error; if (virDomainVirtioOptionsParseXML(virXPathNode("./driver", ctxt), @@ -13682,7 +13697,8 @@ virDomainMemballoonDefParseXML(xmlNodePtr node, } static virDomainNVRAMDefPtr -virDomainNVRAMDefParseXML(xmlNodePtr node, +virDomainNVRAMDefParseXML(virDomainXMLOptionPtr xmlopt, + xmlNodePtr node, unsigned int flags) { virDomainNVRAMDefPtr def; @@ -13690,7 +13706,7 @@ virDomainNVRAMDefParseXML(xmlNodePtr node, if (VIR_ALLOC(def) < 0) return NULL; - if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) + if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, &def->info, flags) < 0) goto error; return def; @@ -13701,7 +13717,8 @@ virDomainNVRAMDefParseXML(xmlNodePtr node, } static virDomainShmemDefPtr -virDomainShmemDefParseXML(xmlNodePtr node, +virDomainShmemDefParseXML(virDomainXMLOptionPtr xmlopt, + xmlNodePtr node, xmlXPathContextPtr ctxt, unsigned int flags) { @@ -13785,7 +13802,7 @@ virDomainShmemDefParseXML(xmlNodePtr node, goto cleanup; } - if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) + if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, &def->info, flags) < 0) goto cleanup; @@ -14206,7 +14223,8 @@ virDomainVideoDriverDefParseXML(xmlNodePtr node) } static virDomainVideoDefPtr -virDomainVideoDefParseXML(xmlNodePtr node, +virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt, + xmlNodePtr node, xmlXPathContextPtr ctxt, const virDomainDef *dom, unsigned int flags) @@ -14320,7 +14338,7 @@ virDomainVideoDefParseXML(xmlNodePtr node, } } - if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) + if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, &def->info, flags) < 0) goto error; def->driver = virDomainVideoDriverDefParseXML(node); @@ -14388,7 +14406,7 @@ virDomainHostdevDefParseXML(virDomainXMLOptionPtr xmlopt, } if (def->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { - if (virDomainDeviceInfoParseXML(node, bootHash, def->info, + if (virDomainDeviceInfoParseXML(xmlopt, node, bootHash, def->info, flags | VIR_DOMAIN_DEF_PARSE_ALLOW_BOOT | VIR_DOMAIN_DEF_PARSE_ALLOW_ROM) < 0) goto error; @@ -14475,7 +14493,7 @@ virDomainRedirdevDefParseXML(virDomainXMLOptionPtr xmlopt, if (def->source->type == VIR_DOMAIN_CHR_TYPE_SPICEVMC) def->source->data.spicevmc = VIR_DOMAIN_CHR_SPICEVMC_USBREDIR; - if (virDomainDeviceInfoParseXML(node, bootHash, &def->info, + if (virDomainDeviceInfoParseXML(xmlopt, node, bootHash, &def->info, flags | VIR_DOMAIN_DEF_PARSE_ALLOW_BOOT) < 0) goto error; @@ -14887,7 +14905,8 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node, static virDomainMemoryDefPtr -virDomainMemoryDefParseXML(xmlNodePtr memdevNode, +virDomainMemoryDefParseXML(virDomainXMLOptionPtr xmlopt, + xmlNodePtr memdevNode, xmlXPathContextPtr ctxt, unsigned int flags) { @@ -14941,7 +14960,8 @@ virDomainMemoryDefParseXML(xmlNodePtr memdevNode, if (virDomainMemoryTargetDefParseXML(node, ctxt, def) < 0) goto error; - if (virDomainDeviceInfoParseXML(memdevNode, NULL, &def->info, flags) < 0) + if (virDomainDeviceInfoParseXML(xmlopt, memdevNode, + NULL, &def->info, flags) < 0) goto error; ctxt->node = save; @@ -15081,7 +15101,7 @@ virDomainDeviceDefParse(const char *xmlStr, goto error; break; case VIR_DOMAIN_DEVICE_FS: - if (!(dev->data.fs = virDomainFSDefParseXML(node, ctxt, flags))) + if (!(dev->data.fs = virDomainFSDefParseXML(xmlopt, node, ctxt, flags))) goto error; break; case VIR_DOMAIN_DEVICE_NET: @@ -15091,20 +15111,23 @@ virDomainDeviceDefParse(const char *xmlStr, goto error; break; case VIR_DOMAIN_DEVICE_INPUT: - if (!(dev->data.input = virDomainInputDefParseXML(def, node, + if (!(dev->data.input = virDomainInputDefParseXML(xmlopt, def, node, ctxt, flags))) goto error; break; case VIR_DOMAIN_DEVICE_SOUND: - if (!(dev->data.sound = virDomainSoundDefParseXML(node, ctxt, flags))) + if (!(dev->data.sound = virDomainSoundDefParseXML(xmlopt, node, + ctxt, flags))) goto error; break; case VIR_DOMAIN_DEVICE_WATCHDOG: - if (!(dev->data.watchdog = virDomainWatchdogDefParseXML(node, flags))) + if (!(dev->data.watchdog = virDomainWatchdogDefParseXML(xmlopt, + node, flags))) goto error; break; case VIR_DOMAIN_DEVICE_VIDEO: - if (!(dev->data.video = virDomainVideoDefParseXML(node, ctxt, def, flags))) + if (!(dev->data.video = virDomainVideoDefParseXML(xmlopt, node, + ctxt, def, flags))) goto error; break; case VIR_DOMAIN_DEVICE_HOSTDEV: @@ -15114,8 +15137,8 @@ virDomainDeviceDefParse(const char *xmlStr, goto error; break; case VIR_DOMAIN_DEVICE_CONTROLLER: - if (!(dev->data.controller = virDomainControllerDefParseXML(node, ctxt, - flags))) + if (!(dev->data.controller = virDomainControllerDefParseXML(xmlopt, node, + ctxt, flags))) goto error; break; case VIR_DOMAIN_DEVICE_GRAPHICS: @@ -15123,7 +15146,7 @@ virDomainDeviceDefParse(const char *xmlStr, goto error; break; case VIR_DOMAIN_DEVICE_HUB: - if (!(dev->data.hub = virDomainHubDefParseXML(node, flags))) + if (!(dev->data.hub = virDomainHubDefParseXML(xmlopt, node, flags))) goto error; break; case VIR_DOMAIN_DEVICE_REDIRDEV: @@ -15151,29 +15174,32 @@ virDomainDeviceDefParse(const char *xmlStr, goto error; break; case VIR_DOMAIN_DEVICE_MEMBALLOON: - if (!(dev->data.memballoon = virDomainMemballoonDefParseXML(node, + if (!(dev->data.memballoon = virDomainMemballoonDefParseXML(xmlopt, + node, ctxt, flags))) goto error; break; case VIR_DOMAIN_DEVICE_NVRAM: - if (!(dev->data.nvram = virDomainNVRAMDefParseXML(node, flags))) + if (!(dev->data.nvram = virDomainNVRAMDefParseXML(xmlopt, node, flags))) goto error; break; case VIR_DOMAIN_DEVICE_SHMEM: - if (!(dev->data.shmem = virDomainShmemDefParseXML(node, ctxt, flags))) + if (!(dev->data.shmem = virDomainShmemDefParseXML(xmlopt, node, + ctxt, flags))) goto error; break; case VIR_DOMAIN_DEVICE_TPM: - if (!(dev->data.tpm = virDomainTPMDefParseXML(node, ctxt, flags))) + if (!(dev->data.tpm = virDomainTPMDefParseXML(xmlopt, node, ctxt, flags))) goto error; break; case VIR_DOMAIN_DEVICE_PANIC: - if (!(dev->data.panic = virDomainPanicDefParseXML(node, flags))) + if (!(dev->data.panic = virDomainPanicDefParseXML(xmlopt, node, flags))) goto error; break; case VIR_DOMAIN_DEVICE_MEMORY: - if (!(dev->data.memory = virDomainMemoryDefParseXML(node, ctxt, flags))) + if (!(dev->data.memory = virDomainMemoryDefParseXML(xmlopt, node, + ctxt, flags))) goto error; break; case VIR_DOMAIN_DEVICE_IOMMU: @@ -18750,7 +18776,8 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; for (i = 0; i < n; i++) { - virDomainControllerDefPtr controller = virDomainControllerDefParseXML(nodes[i], + virDomainControllerDefPtr controller = virDomainControllerDefParseXML(xmlopt, + nodes[i], ctxt, flags); @@ -18816,7 +18843,9 @@ virDomainDefParseXML(xmlDocPtr xml, if (n && VIR_ALLOC_N(def->fss, n) < 0) goto error; for (i = 0; i < n; i++) { - virDomainFSDefPtr fs = virDomainFSDefParseXML(nodes[i], ctxt, + virDomainFSDefPtr fs = virDomainFSDefParseXML(xmlopt, + nodes[i], + ctxt, flags); if (!fs) goto error; @@ -18980,7 +19009,8 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; for (i = 0; i < n; i++) { - virDomainInputDefPtr input = virDomainInputDefParseXML(def, + virDomainInputDefPtr input = virDomainInputDefParseXML(xmlopt, + def, nodes[i], ctxt, flags); @@ -19022,7 +19052,8 @@ virDomainDefParseXML(xmlDocPtr xml, if (n && VIR_ALLOC_N(def->sounds, n) < 0) goto error; for (i = 0; i < n; i++) { - virDomainSoundDefPtr sound = virDomainSoundDefParseXML(nodes[i], + virDomainSoundDefPtr sound = virDomainSoundDefParseXML(xmlopt, + nodes[i], ctxt, flags); if (!sound) @@ -19041,7 +19072,8 @@ virDomainDefParseXML(xmlDocPtr xml, virDomainVideoDefPtr video; ssize_t insertAt = -1; - if (!(video = virDomainVideoDefParseXML(nodes[i], ctxt, def, flags))) + if (!(video = virDomainVideoDefParseXML(xmlopt, nodes[i], + ctxt, def, flags))) goto error; if (video->primary) { @@ -19110,8 +19142,9 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } if (n > 0) { - virDomainWatchdogDefPtr watchdog = - virDomainWatchdogDefParseXML(nodes[0], flags); + virDomainWatchdogDefPtr watchdog; + + watchdog = virDomainWatchdogDefParseXML(xmlopt, nodes[0], flags); if (!watchdog) goto error; @@ -19129,8 +19162,9 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } if (n > 0) { - virDomainMemballoonDefPtr memballoon = - virDomainMemballoonDefParseXML(nodes[0], ctxt, flags); + virDomainMemballoonDefPtr memballoon; + + memballoon = virDomainMemballoonDefParseXML(xmlopt, nodes[0], ctxt, flags); if (!memballoon) goto error; @@ -19164,7 +19198,7 @@ virDomainDefParseXML(xmlDocPtr xml, } if (n > 0) { - if (!(def->tpm = virDomainTPMDefParseXML(nodes[0], ctxt, flags))) + if (!(def->tpm = virDomainTPMDefParseXML(xmlopt, nodes[0], ctxt, flags))) goto error; } VIR_FREE(nodes); @@ -19178,7 +19212,7 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; } else if (n == 1) { virDomainNVRAMDefPtr nvram = - virDomainNVRAMDefParseXML(nodes[0], flags); + virDomainNVRAMDefParseXML(xmlopt, nodes[0], flags); if (!nvram) goto error; def->nvram = nvram; @@ -19191,7 +19225,9 @@ virDomainDefParseXML(xmlDocPtr xml, if (n && VIR_ALLOC_N(def->hubs, n) < 0) goto error; for (i = 0; i < n; i++) { - virDomainHubDefPtr hub = virDomainHubDefParseXML(nodes[i], flags); + virDomainHubDefPtr hub; + + hub = virDomainHubDefParseXML(xmlopt, nodes[i], flags); if (!hub) goto error; @@ -19247,7 +19283,9 @@ virDomainDefParseXML(xmlDocPtr xml, if (n && VIR_ALLOC_N(def->panics, n) < 0) goto error; for (i = 0; i < n; i++) { - virDomainPanicDefPtr panic = virDomainPanicDefParseXML(nodes[i], flags); + virDomainPanicDefPtr panic; + + panic = virDomainPanicDefParseXML(xmlopt, nodes[i], flags); if (!panic) goto error; @@ -19265,7 +19303,7 @@ virDomainDefParseXML(xmlDocPtr xml, for (i = 0; i < n; i++) { virDomainShmemDefPtr shmem; ctxt->node = nodes[i]; - shmem = virDomainShmemDefParseXML(nodes[i], ctxt, flags); + shmem = virDomainShmemDefParseXML(xmlopt, nodes[i], ctxt, flags); if (!shmem) goto error; @@ -19281,7 +19319,8 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; for (i = 0; i < n; i++) { - virDomainMemoryDefPtr mem = virDomainMemoryDefParseXML(nodes[i], + virDomainMemoryDefPtr mem = virDomainMemoryDefParseXML(xmlopt, + nodes[i], ctxt, flags); if (!mem) -- 2.13.6
If driver that is calling the parse supports user supplied aliases, they can be parsed even for inactive XMLs. However, to avoid any clashes with aliases that libvirt generates, the user ones have to have "ua-" prefix. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 13 +++++++++++-- src/conf/domain_conf.h | 1 + 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f4de4e288..f1386c116 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6398,6 +6398,10 @@ virDomainDeviceAddressParseXML(xmlNodePtr address, } +#define USER_ALIAS_PREFIX "ua-" +#define USER_ALIAS_CHARS \ + "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-" + /* Parse the XML definition for a device address * @param node XML nodeset to parse for device address definition */ @@ -6424,7 +6428,6 @@ virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED, while (cur != NULL) { if (cur->type == XML_ELEMENT_NODE) { if (alias == NULL && - !(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && virXMLNodeNameEqual(cur, "alias")) { alias = cur; } else if (address == NULL && @@ -6446,8 +6449,14 @@ virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED, cur = cur->next; } - if (alias) + if (alias) { info->alias = virXMLPropString(alias, "name"); + if (flags & VIR_DOMAIN_DEF_PARSE_INACTIVE && + (!STRPREFIX(info->alias, USER_ALIAS_PREFIX) || + !(xmlopt->config.features & VIR_DOMAIN_DEF_FEATURE_USER_ALIAS) || + strspn(info->alias, USER_ALIAS_CHARS) < strlen(info->alias))) + VIR_FREE(info->alias); + } if (master) { info->mastertype = VIR_DOMAIN_CONTROLLER_MASTER_USB; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1e007346f..5ff85057f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2508,6 +2508,7 @@ typedef enum { VIR_DOMAIN_DEF_FEATURE_OFFLINE_VCPUPIN = (1 << 2), VIR_DOMAIN_DEF_FEATURE_NAME_SLASH = (1 << 3), VIR_DOMAIN_DEF_FEATURE_INDIVIDUAL_VCPUS = (1 << 4), + VIR_DOMAIN_DEF_FEATURE_USER_ALIAS = (1 << 5), } virDomainDefFeatures; -- 2.13.6
On Thu, Oct 19, 2017 at 10:11:02AM +0200, Michal Privoznik wrote:
If driver that is calling the parse supports user supplied aliases, they can be parsed even for inactive XMLs. However, to avoid any clashes with aliases that libvirt generates, the user ones have to have "ua-" prefix.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 13 +++++++++++-- src/conf/domain_conf.h | 1 + 2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f4de4e288..f1386c116 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6398,6 +6398,10 @@ virDomainDeviceAddressParseXML(xmlNodePtr address, }
+#define USER_ALIAS_PREFIX "ua-" +#define USER_ALIAS_CHARS \ + "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-" + /* Parse the XML definition for a device address * @param node XML nodeset to parse for device address definition */ @@ -6424,7 +6428,6 @@ virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED, while (cur != NULL) { if (cur->type == XML_ELEMENT_NODE) { if (alias == NULL && - !(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && virXMLNodeNameEqual(cur, "alias")) { alias = cur; } else if (address == NULL && @@ -6446,8 +6449,14 @@ virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED, cur = cur->next; }
- if (alias) + if (alias) { info->alias = virXMLPropString(alias, "name"); + if (flags & VIR_DOMAIN_DEF_PARSE_INACTIVE && + (!STRPREFIX(info->alias, USER_ALIAS_PREFIX) || + !(xmlopt->config.features & VIR_DOMAIN_DEF_FEATURE_USER_ALIAS) || + strspn(info->alias, USER_ALIAS_CHARS) < strlen(info->alias))) + VIR_FREE(info->alias);
Assigning the pointer just so you can clear it after some hard to read condition seems just plain wrong. Only assign it if the condition is false.
+ }
if (master) { info->mastertype = VIR_DOMAIN_CONTROLLER_MASTER_USB; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1e007346f..5ff85057f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2508,6 +2508,7 @@ typedef enum { VIR_DOMAIN_DEF_FEATURE_OFFLINE_VCPUPIN = (1 << 2), VIR_DOMAIN_DEF_FEATURE_NAME_SLASH = (1 << 3), VIR_DOMAIN_DEF_FEATURE_INDIVIDUAL_VCPUS = (1 << 4), + VIR_DOMAIN_DEF_FEATURE_USER_ALIAS = (1 << 5),
Can you elaborate on why is this not the default and whydo we need to have a flag for it? If it's just so that individual drivers can decide this, adding this to the commit message is enough and ACK with this and the above fixed. Also ACK to previous patches.
} virDomainDefFeatures;
-- 2.13.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 10/19/2017 11:55 AM, Martin Kletzander wrote:
On Thu, Oct 19, 2017 at 10:11:02AM +0200, Michal Privoznik wrote:
If driver that is calling the parse supports user supplied aliases, they can be parsed even for inactive XMLs. However, to avoid any clashes with aliases that libvirt generates, the user ones have to have "ua-" prefix.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 13 +++++++++++-- src/conf/domain_conf.h | 1 + 2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f4de4e288..f1386c116 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6398,6 +6398,10 @@ virDomainDeviceAddressParseXML(xmlNodePtr address, }
+#define USER_ALIAS_PREFIX "ua-" +#define USER_ALIAS_CHARS \ + "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-" + /* Parse the XML definition for a device address * @param node XML nodeset to parse for device address definition */ @@ -6424,7 +6428,6 @@ virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED, while (cur != NULL) { if (cur->type == XML_ELEMENT_NODE) { if (alias == NULL && - !(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && virXMLNodeNameEqual(cur, "alias")) { alias = cur; } else if (address == NULL && @@ -6446,8 +6449,14 @@ virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED, cur = cur->next; }
- if (alias) + if (alias) { info->alias = virXMLPropString(alias, "name"); + if (flags & VIR_DOMAIN_DEF_PARSE_INACTIVE && + (!STRPREFIX(info->alias, USER_ALIAS_PREFIX) || + !(xmlopt->config.features & VIR_DOMAIN_DEF_FEATURE_USER_ALIAS) || + strspn(info->alias, USER_ALIAS_CHARS) < strlen(info->alias))) + VIR_FREE(info->alias);
Assigning the pointer just so you can clear it after some hard to read condition seems just plain wrong. Only assign it if the condition is false.
+ }
if (master) { info->mastertype = VIR_DOMAIN_CONTROLLER_MASTER_USB; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1e007346f..5ff85057f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2508,6 +2508,7 @@ typedef enum { VIR_DOMAIN_DEF_FEATURE_OFFLINE_VCPUPIN = (1 << 2), VIR_DOMAIN_DEF_FEATURE_NAME_SLASH = (1 << 3), VIR_DOMAIN_DEF_FEATURE_INDIVIDUAL_VCPUS = (1 << 4), + VIR_DOMAIN_DEF_FEATURE_USER_ALIAS = (1 << 5),
Can you elaborate on why is this not the default and whydo we need to have a flag for it? If it's just so that individual drivers can decide this, adding this to the commit message is enough and ACK with this and the above fixed.
I've described the reasons in cover letter. In short, rather than enabling this for all drivers, we can take the safe approach and enable it just for the drivers we know are safe. For now. Also, not every driver we have has notion of aliases. But if it turns out that we have enabled it for all the drivers, this flag can be dropped. Until then, I'd like to be rather safe than sorry. Due to Pavel's findings (Thanks! Nice catch), I will send v2 (enhanced with news.xml entry and tests). Meanwhile, I'm pushing 02-06 Michal
They have to be unique within the domain. As usual, backwards compatibility takes its price. In this particular situation we have a device that is represented twice in a domain and so is its alias. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 148 ++++++++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 5 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 3 + 4 files changed, 155 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index f1386c116..0cf67dff1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5458,6 +5458,145 @@ virDomainDeviceDefValidateInternal(const virDomainDeviceDef *dev, } +struct virDomainDefValidateAliasesData { + virHashTablePtr aliases; +}; + + +static int +virDomainDeviceDefValidateAliasesIterator(virDomainDefPtr def, + virDomainDeviceDefPtr dev, + virDomainDeviceInfoPtr info, + void *opaque) +{ + struct virDomainDefValidateAliasesData *data = opaque; + const char *alias = info->alias; + + if (!alias) + return 0; + + /* Some crazy backcompat for consoles. */ + if (def->nserials && def->nconsoles && + def->consoles[0]->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && + def->consoles[0]->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL && + dev->type == VIR_DOMAIN_DEVICE_CHR && + virDomainChrEquals(def->serials[0], dev->data.chr)) + return 0; + + if (virHashLookup(data->aliases, alias)) { + virReportError(VIR_ERR_XML_ERROR, + _("non unique alias detected: %s"), + alias); + return -1; + } + + if (virHashAddEntry(data->aliases, alias, (void *) 1) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to construct table of device aliases")); + return -1; + } + + return 0; +} + + +/** + * virDomainDefValidateAliases: + * + * Check for uniqueness of device aliases. If @aliases is not + * NULL return hash table of all the aliases in it. + * + * Returns 0 on success, + * -1 otherwise (with error reported). + */ +static int +virDomainDefValidateAliases(virDomainXMLOptionPtr xmlopt ATTRIBUTE_UNUSED, + const virDomainDef *def, + virHashTablePtr *aliases, + unsigned int parseFlags) +{ + struct virDomainDefValidateAliasesData data; + int ret = -1; + + /* validate configuration only in certain places */ + if (parseFlags & VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE) + return 0; + + /* We are not storing copies of aliases. Don't free them. */ + if (!(data.aliases = virHashCreate(10, NULL))) + goto cleanup; + + if (virDomainDeviceInfoIterateInternal((virDomainDefPtr) def, + virDomainDeviceDefValidateAliasesIterator, + true, &data) < 0) + goto cleanup; + + if (aliases) { + *aliases = data.aliases; + data.aliases = NULL; + } + + ret = 0; + cleanup: + virHashFree(data.aliases); + return ret; +} + + +static int +virDomainDeviceValidateAliasImpl(virDomainXMLOptionPtr xmlopt, + const virDomainDef *def, + const virDomainDeviceDef *dev) +{ + virHashTablePtr aliases = NULL; + virDomainDeviceInfoPtr info = virDomainDeviceGetInfo(dev); + int ret = -1; + + if (!info || !info->alias) + return 0; + + if (virDomainDefValidateAliases(xmlopt, def, &aliases, 0) < 0) + goto cleanup; + + if (virHashLookup(aliases, info->alias)) { + virReportError(VIR_ERR_XML_ERROR, + _("non unique alias detected: %s"), + info->alias); + goto cleanup; + } + + ret = 0; + cleanup: + + virHashFree(aliases); + return ret; +} + + +int +virDomainDeviceValidateAliasForHotplug(virDomainXMLOptionPtr xmlopt, + virDomainObjPtr vm, + const virDomainDeviceDef *dev, + unsigned int flags) +{ + virDomainDefPtr persDef = NULL; + virDomainDefPtr liveDef = NULL; + + if (virDomainObjGetDefs(vm, flags, &liveDef, &persDef) < 0) + return -1; + + if (persDef && + virDomainDeviceValidateAliasImpl(xmlopt, vm->def, dev) < 0) + return -1; + + if (liveDef && + virDomainDeviceValidateAliasImpl(xmlopt, vm->newDef, dev) < 0) + return -1; + + return 0; +} + + static int virDomainDeviceDefValidate(const virDomainDeviceDef *dev, const virDomainDef *def, @@ -5615,7 +5754,9 @@ virDomainDefCheckDuplicateDriveAddresses(const virDomainDef *def) static int -virDomainDefValidateInternal(const virDomainDef *def) +virDomainDefValidateInternal(virDomainXMLOptionPtr xmlopt, + const virDomainDef *def, + unsigned int parseFlags) { if (virDomainDefCheckDuplicateDiskInfo(def) < 0) return -1; @@ -5626,6 +5767,9 @@ virDomainDefValidateInternal(const virDomainDef *def) if (virDomainDefGetVcpusTopology(def, NULL) < 0) return -1; + if (virDomainDefValidateAliases(xmlopt, def, NULL, parseFlags) < 0) + return -1; + if (def->iommu && def->iommu->intremap == VIR_TRISTATE_SWITCH_ON && (def->features[VIR_DOMAIN_FEATURE_IOAPIC] != VIR_TRISTATE_SWITCH_ON || @@ -5690,7 +5834,7 @@ virDomainDefValidate(virDomainDefPtr def, true, &data) < 0) return -1; - if (virDomainDefValidateInternal(def) < 0) + if (virDomainDefValidateInternal(xmlopt, def, parseFlags) < 0) return -1; return 0; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 5ff85057f..2b90e98cb 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2659,6 +2659,11 @@ int virDomainDefPostParse(virDomainDefPtr def, virDomainXMLOptionPtr xmlopt, void *parseOpaque); +int virDomainDeviceValidateAliasForHotplug(virDomainXMLOptionPtr xmlopt, + virDomainObjPtr vm, + const virDomainDeviceDef *dev, + unsigned int flags); + int virDomainDefValidate(virDomainDefPtr def, virCapsPtr caps, unsigned int parseFlags, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3b5df28e5..8449e9685 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -294,6 +294,7 @@ virDomainDeviceFindControllerModel; virDomainDeviceGetInfo; virDomainDeviceInfoIterate; virDomainDeviceTypeToString; +virDomainDeviceValidateAliasForHotplug; virDomainDiskBusTypeToString; virDomainDiskByAddress; virDomainDiskByName; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fb4d72236..dcd2c2109 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8375,6 +8375,9 @@ qemuDomainAttachDeviceLiveAndConfig(virConnectPtr conn, if (dev == NULL) goto cleanup; + if (virDomainDeviceValidateAliasForHotplug(driver->xmlopt, vm, dev, flags) < 0) + goto cleanup; + if (flags & VIR_DOMAIN_AFFECT_CONFIG && flags & VIR_DOMAIN_AFFECT_LIVE) { /* If we are affecting both CONFIG and LIVE -- 2.13.6
Since we'll be passing user's input onto qemu command line, we have to make sure aliases don't change during migration and all the other places where ABI is checked. Aliases are part of ABI now. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0cf67dff1..cb80939af 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19870,6 +19870,13 @@ virDomainDeviceInfoCheckABIStability(virDomainDeviceInfoPtr src, return false; } + if (STRNEQ_NULLABLE(src->alias, dst->alias)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target device alias %s does not match source %s"), + NULLSTR(src->alias), NULLSTR(dst->alias)); + return false; + } + switch ((virDomainDeviceAddressType) src->type) { case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: if (src->addr.pci.domain != dst->addr.pci.domain || -- 2.13.6
On Thu, Oct 19, 2017 at 10:11:04AM +0200, Michal Privoznik wrote:
Since we'll be passing user's input onto qemu command line, we have to make sure aliases don't change during migration and all the other places where ABI is checked. Aliases are part of ABI now.
I don't see how change of an alias would have an impact on the guest OS since it is not visible at all. Is QEMU using the IDs to refer to objects in the migrated data stream? AFAIK (actually not me, I just asked random stranger about it) it does not (except the memory modules that Peter told us about in one of previous series). I see no reason why we would disable the option to rename a device when doing migration. We are reconstructing the command-line anyway. Did migration fail for you when you tried this?
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0cf67dff1..cb80939af 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19870,6 +19870,13 @@ virDomainDeviceInfoCheckABIStability(virDomainDeviceInfoPtr src, return false; }
+ if (STRNEQ_NULLABLE(src->alias, dst->alias)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target device alias %s does not match source %s"), + NULLSTR(src->alias), NULLSTR(dst->alias)); + return false; + } + switch ((virDomainDeviceAddressType) src->type) { case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: if (src->addr.pci.domain != dst->addr.pci.domain || -- 2.13.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Oct 19, 2017 at 02:16:41PM +0200, Martin Kletzander wrote:
On Thu, Oct 19, 2017 at 10:11:04AM +0200, Michal Privoznik wrote:
Since we'll be passing user's input onto qemu command line, we have to make sure aliases don't change during migration and all the other places where ABI is checked. Aliases are part of ABI now.
I don't see how change of an alias would have an impact on the guest OS since it is not visible at all. Is QEMU using the IDs to refer to objects in the migrated data stream? AFAIK (actually not me, I just asked random stranger about it) it does not (except the memory modules that Peter told us about in one of previous series). I see no reason why we would disable the option to rename a device when doing migration. We are reconstructing the command-line anyway. Did migration fail for you when you tried this?
QEMU never includes the device ID in the migration stream, except for that one screw-up wrt memory modules that we know about. So technically the change below could be loosened to only apply to the memory module alias. Since the whole point of aliases is to have a long term stable identifier for devices though, I figure it is probably beneficial to enforce no-renames for all devices even if QEMU is safe without it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0cf67dff1..cb80939af 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19870,6 +19870,13 @@ virDomainDeviceInfoCheckABIStability(virDomainDeviceInfoPtr src, return false; }
+ if (STRNEQ_NULLABLE(src->alias, dst->alias)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target device alias %s does not match source %s"), + NULLSTR(src->alias), NULLSTR(dst->alias)); + return false; + } + switch ((virDomainDeviceAddressType) src->type) { case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: if (src->addr.pci.domain != dst->addr.pci.domain ||
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Oct 19, 2017 at 01:27:11PM +0100, Daniel P. Berrange wrote:
On Thu, Oct 19, 2017 at 02:16:41PM +0200, Martin Kletzander wrote:
On Thu, Oct 19, 2017 at 10:11:04AM +0200, Michal Privoznik wrote:
Since we'll be passing user's input onto qemu command line, we have to make sure aliases don't change during migration and all the other places where ABI is checked. Aliases are part of ABI now.
I don't see how change of an alias would have an impact on the guest OS since it is not visible at all. Is QEMU using the IDs to refer to objects in the migrated data stream? AFAIK (actually not me, I just asked random stranger about it) it does not (except the memory modules that Peter told us about in one of previous series). I see no reason why we would disable the option to rename a device when doing migration. We are reconstructing the command-line anyway. Did migration fail for you when you tried this?
QEMU never includes the device ID in the migration stream, except for that one screw-up wrt memory modules that we know about.
So technically the change below could be loosened to only apply to the memory module alias. Since the whole point of aliases is to have a long term stable identifier for devices though, I figure it is probably beneficial to enforce no-renames for all devices even if QEMU is safe without it.
Well, depends how you define long-term. Since it is the user/mgmt app that defines these aliases, I don't see why they could not change them.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0cf67dff1..cb80939af 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19870,6 +19870,13 @@ virDomainDeviceInfoCheckABIStability(virDomainDeviceInfoPtr src, return false; }
+ if (STRNEQ_NULLABLE(src->alias, dst->alias)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target device alias %s does not match source %s"), + NULLSTR(src->alias), NULLSTR(dst->alias)); + return false; + } + switch ((virDomainDeviceAddressType) src->type) { case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI: if (src->addr.pci.domain != dst->addr.pci.domain ||
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
The qemuxml2argvtest expects the domain XMLs to be inactive ones. Therefore we should pass inactive XMLs. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-ftp.xml | 1 - tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-ftps.xml | 1 - tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-http.xml | 1 - tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-https.xml | 1 - tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-tftp.xml | 1 - tests/qemuxml2argvdata/qemuxml2argv-usb-redir-filter.xml | 1 - 6 files changed, 6 deletions(-) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-ftp.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-ftp.xml index df41e5832..b4e331160 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-ftp.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-ftp.xml @@ -26,7 +26,6 @@ </source> <target dev='hdc' bus='ide'/> <readonly/> - <alias name='ide0-1-0'/> <address type='drive' controller='0' bus='1' target='0' unit='0'/> </disk> <controller type='usb' index='0'/> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-ftps.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-ftps.xml index 57049a266..4e6ca1bad 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-ftps.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-ftps.xml @@ -26,7 +26,6 @@ </source> <target dev='hdc' bus='ide'/> <readonly/> - <alias name='ide0-1-0'/> <address type='drive' controller='0' bus='1' target='0' unit='0'/> </disk> <controller type='usb' index='0'/> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-http.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-http.xml index 8a1286cb1..0eed65672 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-http.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-http.xml @@ -26,7 +26,6 @@ </source> <target dev='hdc' bus='ide'/> <readonly/> - <alias name='ide0-1-0'/> <address type='drive' controller='0' bus='1' target='0' unit='0'/> </disk> <controller type='usb' index='0'/> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-https.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-https.xml index 41b5a89b6..cd92fe44a 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-https.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-https.xml @@ -26,7 +26,6 @@ </source> <target dev='hdc' bus='ide'/> <readonly/> - <alias name='ide0-1-0'/> <address type='drive' controller='0' bus='1' target='0' unit='0'/> </disk> <controller type='usb' index='0'/> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-tftp.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-tftp.xml index e2cb4a006..1c3b185b0 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-tftp.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-cdrom-network-tftp.xml @@ -26,7 +26,6 @@ </source> <target dev='hdc' bus='ide'/> <readonly/> - <alias name='ide0-1-0'/> <address type='drive' controller='0' bus='1' target='0' unit='0'/> </disk> <controller type='usb' index='0'/> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-usb-redir-filter.xml b/tests/qemuxml2argvdata/qemuxml2argv-usb-redir-filter.xml index 32fb890a1..0bc15f0df 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-usb-redir-filter.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-redir-filter.xml @@ -33,7 +33,6 @@ <address type='usb' bus='0' port='4'/> </redirdev> <redirdev bus='usb' type='spicevmc'> - <alias name='redir1'/> <address type='usb' bus='0' port='5'/> </redirdev> <redirfilter> -- 2.13.6
The point of this test is to load live XML and test hotplug. But even though the XMLs we are parsing are live, the parsing is done with VIR_DOMAIN_DEF_PARSE_INACTIVE flag. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemuhotplugtest.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index df28a7fbd..8d77c0056 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -62,6 +62,7 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt, { int ret = -1; qemuDomainObjPrivatePtr priv = NULL; + const unsigned int parseFlags = 0; if (!(*vm = virDomainObjNew(xmlopt))) goto cleanup; @@ -87,7 +88,7 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt, driver.caps, driver.xmlopt, NULL, - VIR_DOMAIN_DEF_PARSE_INACTIVE))) + parseFlags))) goto cleanup; if (qemuDomainAssignAddresses((*vm)->def, priv->qemuCaps, -- 2.13.6
We need to format alias even for inactive XMLs since that's the way how users are going to identify their devices. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index cb80939af..7efbf55b0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5916,10 +5916,9 @@ virDomainDeviceInfoFormat(virBufferPtr buf, virBufferAddLit(buf, "/>\n"); } - if (info->alias && - !(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) { + + if (info->alias) virBufferAsprintf(buf, "<alias name='%s'/>\n", info->alias); - } if (info->mastertype == VIR_DOMAIN_CONTROLLER_MASTER_USB) { virBufferAsprintf(buf, "<master startport='%d'/>\n", -- 2.13.6
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.html.in | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index b7e145e0f..57dd46dc5 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2249,6 +2249,29 @@ </dd> </dl> + <p> + To help users identifying devices they care about, every + device can have direct child <code>alias</code> element + which then has <code>name</code> attribute where users can + store identifier for the device. The identifier has to have + "ua-" prefix and must be unique within the domain. Additionally, the + identifier must consist only of the following characters: + <code>[a-zA-Z0-9_-]</code>. + <span class="since">Since 3.9.0</span> + </p> + +<pre> +<devices> + <disk type='file'> + <alias name='ua-myDisk'/> + </disk> + <interface type='network' trustGuestRxFilters='yes'> + <alias name='ua-myNIC'/> + </interface> + ... +</devices> +</pre> + <h4><a id="elementsDisks">Hard drives, floppy disks, CDROMs</a></h4> <p> -- 2.13.6
https://bugzilla.redhat.com/show_bug.cgi?id=1434451 This way users can uniquely identify devices at define time. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 05e8b96aa..0ddbea3b7 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4095,7 +4095,8 @@ virDomainDefParserConfig virQEMUDriverDomainDefParserConfig = { .features = VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG | VIR_DOMAIN_DEF_FEATURE_OFFLINE_VCPUPIN | - VIR_DOMAIN_DEF_FEATURE_INDIVIDUAL_VCPUS, + VIR_DOMAIN_DEF_FEATURE_INDIVIDUAL_VCPUS | + VIR_DOMAIN_DEF_FEATURE_USER_ALIAS, }; -- 2.13.6
On Thu, Oct 19, 2017 at 10:10:55AM +0200, Michal Privoznik wrote:
As discussed earlier [1], we should allow users to set device aliases at the define time. While the discussed approach calls for generating missing aliases too, I'm saving that for another patch set. There are couple of reasons for that:
a) I don't think it's really necessary (if users are interested in a device they can just set the alias).
b) This patch set is already big enough as is.
c) Generating aliases is going to have its own problems.
Therefore, for now I'm only proposing parsing user supplied aliases. The idea is that it's not enabled by default for all drivers. Any driver that want to/can provide this feature has to set VIR_DOMAIN_DEF_FEATURE_USER_ALIAS. Note that we have some drivers that don't have notion of device aliases. But the code is generic enough so that it should be easy to use in the drivers that do know what aliases are.
This patch series missed some of the important parts of code that relies on our generated aliases: 1. qemuGetNextChrDevIndex() ... this will fail while attaching new chardev device without an alias if there is one already provided by user and doesn't match the one that we generate. 2. qemuAssignDeviceRedirdevAlias() ... same as 1) 3. qemuAssignDeviceShmemAlias() ... same as 1) 4. qemuDomainNetVLAN() ... similar to the previous ones, hot-plugging network interface with user alias will fail because the alias is used to figure out VLAN of the interface. Pavel
On 10/19/2017 03:33 PM, Pavel Hrdina wrote:
On Thu, Oct 19, 2017 at 10:10:55AM +0200, Michal Privoznik wrote:
As discussed earlier [1], we should allow users to set device aliases at the define time. While the discussed approach calls for generating missing aliases too, I'm saving that for another patch set. There are couple of reasons for that:
a) I don't think it's really necessary (if users are interested in a device they can just set the alias).
b) This patch set is already big enough as is.
c) Generating aliases is going to have its own problems.
Therefore, for now I'm only proposing parsing user supplied aliases. The idea is that it's not enabled by default for all drivers. Any driver that want to/can provide this feature has to set VIR_DOMAIN_DEF_FEATURE_USER_ALIAS. Note that we have some drivers that don't have notion of device aliases. But the code is generic enough so that it should be easy to use in the drivers that do know what aliases are.
This patch series missed some of the important parts of code that relies on our generated aliases:
1. qemuGetNextChrDevIndex() ... this will fail while attaching new chardev device without an alias if there is one already provided by user and doesn't match the one that we generate.
2. qemuAssignDeviceRedirdevAlias() ... same as 1)
3. qemuAssignDeviceShmemAlias() ... same as 1)
Okay, these are easy to resolve.
4. qemuDomainNetVLAN() ... similar to the previous ones, hot-plugging network interface with user alias will fail because the alias is used to figure out VLAN of the interface.
This one. Well, this function is called only for ancient QEMUs (those which lack QMP, and even not for all of them). So do we care? Sure, I can cripple my code and allow user aliases only if running sufficiently new QEMU. Or just shrug and walk away. Michal
On Fri, Oct 20, 2017 at 01:20:20PM +0200, Michal Privoznik wrote:
On 10/19/2017 03:33 PM, Pavel Hrdina wrote:
On Thu, Oct 19, 2017 at 10:10:55AM +0200, Michal Privoznik wrote:
As discussed earlier [1], we should allow users to set device aliases at the define time. While the discussed approach calls for generating missing aliases too, I'm saving that for another patch set. There are couple of reasons for that:
a) I don't think it's really necessary (if users are interested in a device they can just set the alias).
b) This patch set is already big enough as is.
c) Generating aliases is going to have its own problems.
Therefore, for now I'm only proposing parsing user supplied aliases. The idea is that it's not enabled by default for all drivers. Any driver that want to/can provide this feature has to set VIR_DOMAIN_DEF_FEATURE_USER_ALIAS. Note that we have some drivers that don't have notion of device aliases. But the code is generic enough so that it should be easy to use in the drivers that do know what aliases are.
This patch series missed some of the important parts of code that relies on our generated aliases:
1. qemuGetNextChrDevIndex() ... this will fail while attaching new chardev device without an alias if there is one already provided by user and doesn't match the one that we generate.
2. qemuAssignDeviceRedirdevAlias() ... same as 1)
3. qemuAssignDeviceShmemAlias() ... same as 1)
Okay, these are easy to resolve.
4. qemuDomainNetVLAN() ... similar to the previous ones, hot-plugging network interface with user alias will fail because the alias is used to figure out VLAN of the interface.
This one. Well, this function is called only for ancient QEMUs (those which lack QMP, and even not for all of them). So do we care? Sure, I can cripple my code and allow user aliases only if running sufficiently new QEMU. Or just shrug and walk away.
In that case just ignore it, it will print an error message and since this will affect only hot-plug of network devices with user alias specified we can claim that this operation is unsupported. Pavel
On 10/20/2017 01:38 PM, Pavel Hrdina wrote:
On Fri, Oct 20, 2017 at 01:20:20PM +0200, Michal Privoznik wrote:
On 10/19/2017 03:33 PM, Pavel Hrdina wrote:
On Thu, Oct 19, 2017 at 10:10:55AM +0200, Michal Privoznik wrote:
As discussed earlier [1], we should allow users to set device aliases at the define time. While the discussed approach calls for generating missing aliases too, I'm saving that for another patch set. There are couple of reasons for that:
a) I don't think it's really necessary (if users are interested in a device they can just set the alias).
b) This patch set is already big enough as is.
c) Generating aliases is going to have its own problems.
Therefore, for now I'm only proposing parsing user supplied aliases. The idea is that it's not enabled by default for all drivers. Any driver that want to/can provide this feature has to set VIR_DOMAIN_DEF_FEATURE_USER_ALIAS. Note that we have some drivers that don't have notion of device aliases. But the code is generic enough so that it should be easy to use in the drivers that do know what aliases are.
This patch series missed some of the important parts of code that relies on our generated aliases:
1. qemuGetNextChrDevIndex() ... this will fail while attaching new chardev device without an alias if there is one already provided by user and doesn't match the one that we generate.
2. qemuAssignDeviceRedirdevAlias() ... same as 1)
3. qemuAssignDeviceShmemAlias() ... same as 1)
Okay, these are easy to resolve.
4. qemuDomainNetVLAN() ... similar to the previous ones, hot-plugging network interface with user alias will fail because the alias is used to figure out VLAN of the interface.
This one. Well, this function is called only for ancient QEMUs (those which lack QMP, and even not for all of them). So do we care? Sure, I can cripple my code and allow user aliases only if running sufficiently new QEMU. Or just shrug and walk away.
In that case just ignore it, it will print an error message and since this will affect only hot-plug of network devices with user alias specified we can claim that this operation is unsupported.
Yeah. Just don't forget the ancient QEMU part. So the whole story is that if you're running QEMU that's lacking -netdev, and you want to hotplug an interface with user supplied alias, we error out. Yeah, I'm okay with that. We have to draw a line somewhere and say: yeah, it's probably bad behaviour, be we don't care. Patches are welcome. Michal
participants (4)
-
Daniel P. Berrange -
Martin Kletzander -
Michal Privoznik -
Pavel Hrdina