In the near future the qemuAssignDeviceAliases() function is
going to be called multiple times: once at the domain define
time, then in domain prepare phase. To avoid regenerating the
same aliases the second time we need to be able to tell the
function which time it is being called.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
src/qemu/qemu_alias.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++--
src/qemu/qemu_alias.h | 4 +-
src/qemu/qemu_driver.c | 2 +-
src/qemu/qemu_process.c | 2 +-
tests/qemuhotplugtest.c | 2 +-
5 files changed, 115 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
index e1431e2a2..00868c50f 100644
--- a/src/qemu/qemu_alias.c
+++ b/src/qemu/qemu_alias.c
@@ -481,25 +481,99 @@ qemuAssignDeviceShmemAlias(virDomainDefPtr def,
}
+/*
+ * qemuAssignDeviceAliases:
+ * @def: domain definition
+ * @qemuCaps: qemu capabilities
+ * @regenerate: forcibly regenerate aliases
+ *
+ * Assign aliases to domain devices. If @regenerate is false only
+ * the missing aliases are generated leaving already assigned
+ * aliases untouched. If @regenerate is true any preexisting
+ * aliases are overwritten.
+ *
+ * Returns 0 on success, -1 otherwise.
+ */
int
-qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
+qemuAssignDeviceAliases(virDomainDefPtr def,
+ virQEMUCapsPtr qemuCaps,
+ bool regenerate)
{
size_t i;
+ if (regenerate) {
+ /* If we need to regenerate the aliases, we have to free
+ * them all upfront because there are some dependencies,
+ * e.g. an interface can be a hostdev and so on.
+ * Therefore, if we'd do the freeing in the loop below we
+ * might not start from zero. */
+ for (i = 0; i < def->ndisks; i++)
+ VIR_FREE(def->disks[i]->info.alias);
+ for (i = 0; i < def->nnets; i++)
+ VIR_FREE(def->nets[i]->info.alias);
+ for (i = 0; i < def->nfss; i++)
+ VIR_FREE(def->fss[i]->info.alias);
+ for (i = 0; i < def->nsounds; i++)
+ VIR_FREE(def->sounds[i]->info.alias);
+ for (i = 0; i < def->nhostdevs; i++)
+ VIR_FREE(def->hostdevs[i]->info->alias);
+ for (i = 0; i < def->nredirdevs; i++)
+ VIR_FREE(def->redirdevs[i]->info.alias);
+ for (i = 0; i < def->nvideos; i++)
+ VIR_FREE(def->videos[i]->info.alias);
+ for (i = 0; i < def->ncontrollers; i++)
+ VIR_FREE(def->controllers[i]->info.alias);
+ for (i = 0; i < def->ninputs; i++)
+ VIR_FREE(def->inputs[i]->info.alias);
+ for (i = 0; i < def->nparallels; i++)
+ VIR_FREE(def->parallels[i]->info.alias);
+ for (i = 0; i < def->nserials; i++)
+ VIR_FREE(def->serials[i]->info.alias);
+ for (i = 0; i < def->nchannels; i++)
+ VIR_FREE(def->channels[i]->info.alias);
+ for (i = 0; i < def->nconsoles; i++)
+ VIR_FREE(def->consoles[i]->info.alias);
+ for (i = 0; i < def->nhubs; i++)
+ VIR_FREE(def->hubs[i]->info.alias);
+ for (i = 0; i < def->nshmems; i++)
+ VIR_FREE(def->shmems[i]->info.alias);
+ for (i = 0; i < def->nsmartcards; i++)
+ VIR_FREE(def->smartcards[i]->info.alias);
+ if (def->watchdog)
+ VIR_FREE(def->watchdog->info.alias);
+ if (def->memballoon)
+ VIR_FREE(def->memballoon->info.alias);
+ for (i = 0; i < def->nrngs; i++)
+ VIR_FREE(def->rngs[i]->info.alias);
+ if (def->tpm)
+ VIR_FREE(def->tpm->info.alias);
+ for (i = 0; i < def->nmems; i++)
+ VIR_FREE(def->mems[i]->info.alias);
+ }
+
+
for (i = 0; i < def->ndisks; i++) {
+ if (def->disks[i]->info.alias && !regenerate)
+ continue;
if (qemuAssignDeviceDiskAlias(def, def->disks[i], qemuCaps) < 0)
return -1;
}
for (i = 0; i < def->nnets; i++) {
+ if (def->nets[i]->info.alias && !regenerate)
+ continue;
if (qemuAssignDeviceNetAlias(def, def->nets[i], -1) < 0)
return -1;
}
for (i = 0; i < def->nfss; i++) {
+ if (def->fss[i]->info.alias && !regenerate)
+ continue;
if (qemuAssignDeviceFSAlias(def->fss[i], i) < 0)
return -1;
}
for (i = 0; i < def->nsounds; i++) {
+ if (def->sounds[i]->info.alias && !regenerate)
+ continue;
if (qemuAssignDeviceSoundAlias(def->sounds[i], i) < 0)
return -1;
}
@@ -509,71 +583,102 @@ 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 && !regenerate)
+ continue;
if (!def->hostdevs[i]->info->alias &&
qemuAssignDeviceHostdevAlias(def, &def->hostdevs[i]->info->alias, -1) < 0)
return -1;
}
for (i = 0; i < def->nredirdevs; i++) {
+ if (def->redirdevs[i]->info.alias && !regenerate)
+ continue;
if (qemuAssignDeviceRedirdevAlias(def, def->redirdevs[i], i) < 0)
return -1;
}
for (i = 0; i < def->nvideos; i++) {
+ if (def->videos[i]->info.alias && !regenerate)
+ continue;
if (qemuAssignDeviceVideoAlias(def->videos[i], i) < 0)
return -1;
}
for (i = 0; i < def->ncontrollers; i++) {
+ if (def->controllers[i]->info.alias && !regenerate)
+ continue;
if (qemuAssignDeviceControllerAlias(def, qemuCaps, def->controllers[i]) < 0)
return -1;
}
for (i = 0; i < def->ninputs; i++) {
+ if (def->inputs[i]->info.alias && !regenerate)
+ continue;
if (qemuAssignDeviceInputAlias(def->inputs[i], i) < 0)
return -1;
}
for (i = 0; i < def->nparallels; i++) {
+ if (def->parallels[i]->info.alias && !regenerate)
+ continue;
if (qemuAssignDeviceChrAlias(def, def->parallels[i], i) < 0)
return -1;
}
for (i = 0; i < def->nserials; i++) {
+ if (def->serials[i]->info.alias && !regenerate)
+ continue;
if (qemuAssignDeviceChrAlias(def, def->serials[i], i) < 0)
return -1;
}
for (i = 0; i < def->nchannels; i++) {
+ if (def->channels[i]->info.alias && !regenerate)
+ continue;
if (qemuAssignDeviceChrAlias(def, def->channels[i], i) < 0)
return -1;
}
for (i = 0; i < def->nconsoles; i++) {
+ if (def->consoles[i]->info.alias && !regenerate)
+ continue;
if (qemuAssignDeviceChrAlias(def, def->consoles[i], i) < 0)
return -1;
}
for (i = 0; i < def->nhubs; i++) {
+ if (def->hubs[i]->info.alias && !regenerate)
+ continue;
if (qemuAssignDeviceHubAlias(def->hubs[i], i) < 0)
return -1;
}
for (i = 0; i < def->nshmems; i++) {
+ if (def->shmems[i]->info.alias && !regenerate)
+ continue;
if (qemuAssignDeviceShmemAlias(def, def->shmems[i], i) < 0)
return -1;
}
for (i = 0; i < def->nsmartcards; i++) {
+ if (def->smartcards[i]->info.alias && !regenerate)
+ continue;
if (qemuAssignDeviceSmartcardAlias(def->smartcards[i], i) < 0)
return -1;
}
- if (def->watchdog) {
+ if (def->watchdog &&
+ (!def->watchdog->info.alias || regenerate)) {
if (qemuAssignDeviceWatchdogAlias(def->watchdog, 0) < 0)
return -1;
}
- if (def->memballoon) {
+ if (def->memballoon &&
+ (!def->memballoon->info.alias || regenerate)) {
if (qemuAssingDeviceMemballoonAlias(def->memballoon, 0) < 0)
return -1;
}
for (i = 0; i < def->nrngs; i++) {
+ if (def->rngs[i]->info.alias && !regenerate)
+ continue;
if (qemuAssignDeviceRNGAlias(def, def->rngs[i], i) < 0)
return -1;
}
- if (def->tpm) {
+ if (def->tpm &&
+ (!def->tpm->info.alias || regenerate)) {
if (qemuAssignDeviceTPMAlias(def->tpm, 0) < 0)
return -1;
}
for (i = 0; i < def->nmems; i++) {
+ if (def->mems[i]->info.alias && !regenerate)
+ continue;
if (qemuAssignDeviceMemoryAlias(NULL, def->mems[i], false) < 0)
return -1;
}
diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h
index 860ab6c0c..e33a05580 100644
--- a/src/qemu/qemu_alias.h
+++ b/src/qemu/qemu_alias.h
@@ -66,7 +66,9 @@ int qemuAssignDeviceShmemAlias(virDomainDefPtr def,
virDomainShmemDefPtr shmem,
int idx);
-int qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps);
+int qemuAssignDeviceAliases(virDomainDefPtr def,
+ virQEMUCapsPtr qemuCaps,
+ bool regenerate);
int qemuDomainDeviceAliasIndex(const virDomainDeviceInfo *info,
const char *prefix);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1b271792d..64ba7d454 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16145,7 +16145,7 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr conn,
def->emulator)))
goto cleanup;
- if (qemuAssignDeviceAliases(def, qemuCaps) < 0)
+ if (qemuAssignDeviceAliases(def, qemuCaps, true) < 0)
goto cleanup;
if (!(vm = virDomainObjListAdd(driver->domains, def,
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index e6cc41e13..448de679c 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5340,7 +5340,7 @@ qemuProcessPrepareDomain(virConnectPtr conn,
goto cleanup;
}
- if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0)
+ if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps, false) < 0)
goto cleanup;
VIR_DEBUG("Setting graphics devices");
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index 23a498617..0b1b00176 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -95,7 +95,7 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt,
goto cleanup;
}
- if (qemuAssignDeviceAliases((*vm)->def, priv->qemuCaps) < 0)
+ if (qemuAssignDeviceAliases((*vm)->def, priv->qemuCaps, true) < 0)
goto cleanup;
(*vm)->def->id = QEMU_HOTPLUG_TEST_DOMAIN_ID;
--
2.13.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Sep 21, 2017 at 16:47:08 +0200, Michal Privoznik wrote:
> In the near future the qemuAssignDeviceAliases() function is
> going to be called multiple times: once at the domain define
> time, then in domain prepare phase. To avoid regenerating the
> same aliases the second time we need to be able to tell the
> function which time it is being called.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> src/qemu/qemu_alias.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++--
> src/qemu/qemu_alias.h | 4 +-
> src/qemu/qemu_driver.c | 2 +-
> src/qemu/qemu_process.c | 2 +-
> tests/qemuhotplugtest.c | 2 +-
> 5 files changed, 115 insertions(+), 8 deletions(-)
>
> diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
> index e1431e2a2..00868c50f 100644
> --- a/src/qemu/qemu_alias.c
> +++ b/src/qemu/qemu_alias.c
> @@ -481,25 +481,99 @@ qemuAssignDeviceShmemAlias(virDomainDefPtr def,
> }
>
>
> +/*
> + * qemuAssignDeviceAliases:
> + * @def: domain definition
> + * @qemuCaps: qemu capabilities
> + * @regenerate: forcibly regenerate aliases
> + *
> + * Assign aliases to domain devices. If @regenerate is false only
> + * the missing aliases are generated leaving already assigned
> + * aliases untouched. If @regenerate is true any preexisting
> + * aliases are overwritten.
> + *
> + * Returns 0 on success, -1 otherwise.
> + */
> int
> -qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
> +qemuAssignDeviceAliases(virDomainDefPtr def,
> + virQEMUCapsPtr qemuCaps,
> + bool regenerate)
> {
> size_t i;
>
> + if (regenerate) {
> + /* If we need to regenerate the aliases, we have to free
> + * them all upfront because there are some dependencies,
> + * e.g. an interface can be a hostdev and so on.
> + * Therefore, if we'd do the freeing in the loop below we
> + * might not start from zero. */
> + for (i = 0; i < def->ndisks; i++)
> + VIR_FREE(def->disks[i]->info.alias);
> + for (i = 0; i < def->nnets; i++)
> + VIR_FREE(def->nets[i]->info.alias);
> + for (i = 0; i < def->nfss; i++)
> + VIR_FREE(def->fss[i]->info.alias);
> + for (i = 0; i < def->nsounds; i++)
> + VIR_FREE(def->sounds[i]->info.alias);
> + for (i = 0; i < def->nhostdevs; i++)
> + VIR_FREE(def->hostdevs[i]->info->alias);
> + for (i = 0; i < def->nredirdevs; i++)
> + VIR_FREE(def->redirdevs[i]->info.alias);
> + for (i = 0; i < def->nvideos; i++)
> + VIR_FREE(def->videos[i]->info.alias);
> + for (i = 0; i < def->ncontrollers; i++)
> + VIR_FREE(def->controllers[i]->info.alias);
> + for (i = 0; i < def->ninputs; i++)
> + VIR_FREE(def->inputs[i]->info.alias);
> + for (i = 0; i < def->nparallels; i++)
> + VIR_FREE(def->parallels[i]->info.alias);
> + for (i = 0; i < def->nserials; i++)
> + VIR_FREE(def->serials[i]->info.alias);
> + for (i = 0; i < def->nchannels; i++)
> + VIR_FREE(def->channels[i]->info.alias);
> + for (i = 0; i < def->nconsoles; i++)
> + VIR_FREE(def->consoles[i]->info.alias);
> + for (i = 0; i < def->nhubs; i++)
> + VIR_FREE(def->hubs[i]->info.alias);
> + for (i = 0; i < def->nshmems; i++)
> + VIR_FREE(def->shmems[i]->info.alias);
> + for (i = 0; i < def->nsmartcards; i++)
> + VIR_FREE(def->smartcards[i]->info.alias);
> + if (def->watchdog)
> + VIR_FREE(def->watchdog->info.alias);
> + if (def->memballoon)
> + VIR_FREE(def->memballoon->info.alias);
> + for (i = 0; i < def->nrngs; i++)
> + VIR_FREE(def->rngs[i]->info.alias);
> + if (def->tpm)
> + VIR_FREE(def->tpm->info.alias);
> + for (i = 0; i < def->nmems; i++)
> + VIR_FREE(def->mems[i]->info.alias);
> + }
> +
> +
> for (i = 0; i < def->ndisks; i++) {
> + if (def->disks[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceDiskAlias(def, def->disks[i], qemuCaps) < 0)
> return -1;
> }
> for (i = 0; i < def->nnets; i++) {
> + if (def->nets[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceNetAlias(def, def->nets[i], -1) < 0)
> return -1;
> }
>
> for (i = 0; i < def->nfss; i++) {
> + if (def->fss[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceFSAlias(def->fss[i], i) < 0)
> return -1;
> }
> for (i = 0; i < def->nsounds; i++) {
> + if (def->sounds[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceSoundAlias(def->sounds[i], i) < 0)
> return -1;
> }
> @@ -509,71 +583,102 @@ 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 && !regenerate)
> + continue;
> if (!def->hostdevs[i]->info->alias &&
> qemuAssignDeviceHostdevAlias(def, &def->hostdevs[i]->info->alias, -1) < 0)
> return -1;
> }
> for (i = 0; i < def->nredirdevs; i++) {
> + if (def->redirdevs[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceRedirdevAlias(def, def->redirdevs[i], i) < 0)
> return -1;
> }
> for (i = 0; i < def->nvideos; i++) {
> + if (def->videos[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceVideoAlias(def->videos[i], i) < 0)
> return -1;
> }
> for (i = 0; i < def->ncontrollers; i++) {
> + if (def->controllers[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceControllerAlias(def, qemuCaps, def->controllers[i]) < 0)
> return -1;
> }
> for (i = 0; i < def->ninputs; i++) {
> + if (def->inputs[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceInputAlias(def->inputs[i], i) < 0)
> return -1;
> }
> for (i = 0; i < def->nparallels; i++) {
> + if (def->parallels[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceChrAlias(def, def->parallels[i], i) < 0)
> return -1;
> }
> for (i = 0; i < def->nserials; i++) {
> + if (def->serials[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceChrAlias(def, def->serials[i], i) < 0)
> return -1;
> }
> for (i = 0; i < def->nchannels; i++) {
> + if (def->channels[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceChrAlias(def, def->channels[i], i) < 0)
> return -1;
> }
> for (i = 0; i < def->nconsoles; i++) {
> + if (def->consoles[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceChrAlias(def, def->consoles[i], i) < 0)
> return -1;
> }
> for (i = 0; i < def->nhubs; i++) {
> + if (def->hubs[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceHubAlias(def->hubs[i], i) < 0)
> return -1;
> }
> for (i = 0; i < def->nshmems; i++) {
> + if (def->shmems[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceShmemAlias(def, def->shmems[i], i) < 0)
> return -1;
> }
> for (i = 0; i < def->nsmartcards; i++) {
> + if (def->smartcards[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceSmartcardAlias(def->smartcards[i], i) < 0)
> return -1;
> }
> - if (def->watchdog) {
> + if (def->watchdog &&
> + (!def->watchdog->info.alias || regenerate)) {
> if (qemuAssignDeviceWatchdogAlias(def->watchdog, 0) < 0)
> return -1;
> }
> - if (def->memballoon) {
> + if (def->memballoon &&
> + (!def->memballoon->info.alias || regenerate)) {
> if (qemuAssingDeviceMemballoonAlias(def->memballoon, 0) < 0)
> return -1;
> }
> for (i = 0; i < def->nrngs; i++) {
> + if (def->rngs[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceRNGAlias(def, def->rngs[i], i) < 0)
> return -1;
> }
So if you want to make aliases persistent, at least this will not work
properly with device coldplug.
If you have two devices, detach the first one, then the second one moves
to index 0 but will still have alias ending with 1. Then if you cold-add
another device that will be put into index 1, and when starting the VM
it will get assigned the same alias as the one which has the old one.
This applies to all devices where the alias depends on the ordering.
Additionally this series specifically does not assign aliases for
coldplugged devices so there's a discrepancy between when defining a XML
and modifying it with the APIs which should not really be the case.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Sep 21, 2017 at 06:05:19PM +0200, Peter Krempa wrote:
> On Thu, Sep 21, 2017 at 16:47:08 +0200, Michal Privoznik wrote:
> > In the near future the qemuAssignDeviceAliases() function is
> > going to be called multiple times: once at the domain define
> > time, then in domain prepare phase. To avoid regenerating the
> > same aliases the second time we need to be able to tell the
> > function which time it is being called.
> >
> > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > ---
> > src/qemu/qemu_alias.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++--
> > src/qemu/qemu_alias.h | 4 +-
> > src/qemu/qemu_driver.c | 2 +-
> > src/qemu/qemu_process.c | 2 +-
> > tests/qemuhotplugtest.c | 2 +-
> > 5 files changed, 115 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
> > index e1431e2a2..00868c50f 100644
> > --- a/src/qemu/qemu_alias.c
> > +++ b/src/qemu/qemu_alias.c
> > @@ -481,25 +481,99 @@ qemuAssignDeviceShmemAlias(virDomainDefPtr def,
> > }
> >
> >
> > +/*
> > + * qemuAssignDeviceAliases:
> > + * @def: domain definition
> > + * @qemuCaps: qemu capabilities
> > + * @regenerate: forcibly regenerate aliases
> > + *
> > + * Assign aliases to domain devices. If @regenerate is false only
> > + * the missing aliases are generated leaving already assigned
> > + * aliases untouched. If @regenerate is true any preexisting
> > + * aliases are overwritten.
> > + *
> > + * Returns 0 on success, -1 otherwise.
> > + */
> > int
> > -qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
> > +qemuAssignDeviceAliases(virDomainDefPtr def,
> > + virQEMUCapsPtr qemuCaps,
> > + bool regenerate)
> > {
> > size_t i;
> >
> > + if (regenerate) {
> > + /* If we need to regenerate the aliases, we have to free
> > + * them all upfront because there are some dependencies,
> > + * e.g. an interface can be a hostdev and so on.
> > + * Therefore, if we'd do the freeing in the loop below we
> > + * might not start from zero. */
> > + for (i = 0; i < def->ndisks; i++)
> > + VIR_FREE(def->disks[i]->info.alias);
> > + for (i = 0; i < def->nnets; i++)
> > + VIR_FREE(def->nets[i]->info.alias);
> > + for (i = 0; i < def->nfss; i++)
> > + VIR_FREE(def->fss[i]->info.alias);
> > + for (i = 0; i < def->nsounds; i++)
> > + VIR_FREE(def->sounds[i]->info.alias);
> > + for (i = 0; i < def->nhostdevs; i++)
> > + VIR_FREE(def->hostdevs[i]->info->alias);
> > + for (i = 0; i < def->nredirdevs; i++)
> > + VIR_FREE(def->redirdevs[i]->info.alias);
> > + for (i = 0; i < def->nvideos; i++)
> > + VIR_FREE(def->videos[i]->info.alias);
> > + for (i = 0; i < def->ncontrollers; i++)
> > + VIR_FREE(def->controllers[i]->info.alias);
> > + for (i = 0; i < def->ninputs; i++)
> > + VIR_FREE(def->inputs[i]->info.alias);
> > + for (i = 0; i < def->nparallels; i++)
> > + VIR_FREE(def->parallels[i]->info.alias);
> > + for (i = 0; i < def->nserials; i++)
> > + VIR_FREE(def->serials[i]->info.alias);
> > + for (i = 0; i < def->nchannels; i++)
> > + VIR_FREE(def->channels[i]->info.alias);
> > + for (i = 0; i < def->nconsoles; i++)
> > + VIR_FREE(def->consoles[i]->info.alias);
> > + for (i = 0; i < def->nhubs; i++)
> > + VIR_FREE(def->hubs[i]->info.alias);
> > + for (i = 0; i < def->nshmems; i++)
> > + VIR_FREE(def->shmems[i]->info.alias);
> > + for (i = 0; i < def->nsmartcards; i++)
> > + VIR_FREE(def->smartcards[i]->info.alias);
> > + if (def->watchdog)
> > + VIR_FREE(def->watchdog->info.alias);
> > + if (def->memballoon)
> > + VIR_FREE(def->memballoon->info.alias);
> > + for (i = 0; i < def->nrngs; i++)
> > + VIR_FREE(def->rngs[i]->info.alias);
> > + if (def->tpm)
> > + VIR_FREE(def->tpm->info.alias);
> > + for (i = 0; i < def->nmems; i++)
> > + VIR_FREE(def->mems[i]->info.alias);
> > + }
> > +
> > +
> > for (i = 0; i < def->ndisks; i++) {
> > + if (def->disks[i]->info.alias && !regenerate)
> > + continue;
> > if (qemuAssignDeviceDiskAlias(def, def->disks[i], qemuCaps) < 0)
> > return -1;
> > }
> > for (i = 0; i < def->nnets; i++) {
> > + if (def->nets[i]->info.alias && !regenerate)
> > + continue;
> > if (qemuAssignDeviceNetAlias(def, def->nets[i], -1) < 0)
> > return -1;
> > }
> >
> > for (i = 0; i < def->nfss; i++) {
> > + if (def->fss[i]->info.alias && !regenerate)
> > + continue;
> > if (qemuAssignDeviceFSAlias(def->fss[i], i) < 0)
> > return -1;
> > }
> > for (i = 0; i < def->nsounds; i++) {
> > + if (def->sounds[i]->info.alias && !regenerate)
> > + continue;
> > if (qemuAssignDeviceSoundAlias(def->sounds[i], i) < 0)
> > return -1;
> > }
> > @@ -509,71 +583,102 @@ 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 && !regenerate)
> > + continue;
> > if (!def->hostdevs[i]->info->alias &&
> > qemuAssignDeviceHostdevAlias(def, &def->hostdevs[i]->info->alias, -1) < 0)
> > return -1;
> > }
> > for (i = 0; i < def->nredirdevs; i++) {
> > + if (def->redirdevs[i]->info.alias && !regenerate)
> > + continue;
> > if (qemuAssignDeviceRedirdevAlias(def, def->redirdevs[i], i) < 0)
> > return -1;
> > }
> > for (i = 0; i < def->nvideos; i++) {
> > + if (def->videos[i]->info.alias && !regenerate)
> > + continue;
> > if (qemuAssignDeviceVideoAlias(def->videos[i], i) < 0)
> > return -1;
> > }
> > for (i = 0; i < def->ncontrollers; i++) {
> > + if (def->controllers[i]->info.alias && !regenerate)
> > + continue;
> > if (qemuAssignDeviceControllerAlias(def, qemuCaps, def->controllers[i]) < 0)
> > return -1;
> > }
> > for (i = 0; i < def->ninputs; i++) {
> > + if (def->inputs[i]->info.alias && !regenerate)
> > + continue;
> > if (qemuAssignDeviceInputAlias(def->inputs[i], i) < 0)
> > return -1;
> > }
> > for (i = 0; i < def->nparallels; i++) {
> > + if (def->parallels[i]->info.alias && !regenerate)
> > + continue;
> > if (qemuAssignDeviceChrAlias(def, def->parallels[i], i) < 0)
> > return -1;
> > }
> > for (i = 0; i < def->nserials; i++) {
> > + if (def->serials[i]->info.alias && !regenerate)
> > + continue;
> > if (qemuAssignDeviceChrAlias(def, def->serials[i], i) < 0)
> > return -1;
> > }
> > for (i = 0; i < def->nchannels; i++) {
> > + if (def->channels[i]->info.alias && !regenerate)
> > + continue;
> > if (qemuAssignDeviceChrAlias(def, def->channels[i], i) < 0)
> > return -1;
> > }
> > for (i = 0; i < def->nconsoles; i++) {
> > + if (def->consoles[i]->info.alias && !regenerate)
> > + continue;
> > if (qemuAssignDeviceChrAlias(def, def->consoles[i], i) < 0)
> > return -1;
> > }
> > for (i = 0; i < def->nhubs; i++) {
> > + if (def->hubs[i]->info.alias && !regenerate)
> > + continue;
> > if (qemuAssignDeviceHubAlias(def->hubs[i], i) < 0)
> > return -1;
> > }
> > for (i = 0; i < def->nshmems; i++) {
> > + if (def->shmems[i]->info.alias && !regenerate)
> > + continue;
> > if (qemuAssignDeviceShmemAlias(def, def->shmems[i], i) < 0)
> > return -1;
> > }
> > for (i = 0; i < def->nsmartcards; i++) {
> > + if (def->smartcards[i]->info.alias && !regenerate)
> > + continue;
> > if (qemuAssignDeviceSmartcardAlias(def->smartcards[i], i) < 0)
> > return -1;
> > }
> > - if (def->watchdog) {
> > + if (def->watchdog &&
> > + (!def->watchdog->info.alias || regenerate)) {
> > if (qemuAssignDeviceWatchdogAlias(def->watchdog, 0) < 0)
> > return -1;
> > }
> > - if (def->memballoon) {
> > + if (def->memballoon &&
> > + (!def->memballoon->info.alias || regenerate)) {
> > if (qemuAssingDeviceMemballoonAlias(def->memballoon, 0) < 0)
> > return -1;
> > }
> > for (i = 0; i < def->nrngs; i++) {
> > + if (def->rngs[i]->info.alias && !regenerate)
> > + continue;
> > if (qemuAssignDeviceRNGAlias(def, def->rngs[i], i) < 0)
> > return -1;
> > }
>
> So if you want to make aliases persistent, at least this will not work
> properly with device coldplug.
>
> If you have two devices, detach the first one, then the second one moves
> to index 0 but will still have alias ending with 1. Then if you cold-add
> another device that will be put into index 1, and when starting the VM
> it will get assigned the same alias as the one which has the old one.
>
> This applies to all devices where the alias depends on the ordering.
Yep, we would need to maintain a hash table remembering all currently
assigned aliases, and then increment the counter until we find a free
one for that dev type.
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 09/21/2017 07:18 PM, Daniel P. Berrange wrote:
> On Thu, Sep 21, 2017 at 06:05:19PM +0200, Peter Krempa wrote:
>> On Thu, Sep 21, 2017 at 16:47:08 +0200, Michal Privoznik wrote:
>>> In the near future the qemuAssignDeviceAliases() function is
>>> going to be called multiple times: once at the domain define
>>> time, then in domain prepare phase. To avoid regenerating the
>>> same aliases the second time we need to be able to tell the
>>> function which time it is being called.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>> ---
>>> src/qemu/qemu_alias.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++--
>>> src/qemu/qemu_alias.h | 4 +-
>>> src/qemu/qemu_driver.c | 2 +-
>>> src/qemu/qemu_process.c | 2 +-
>>> tests/qemuhotplugtest.c | 2 +-
>>> 5 files changed, 115 insertions(+), 8 deletions(-)
>>>
>> So if you want to make aliases persistent, at least this will not work
>> properly with device coldplug.
Ah, good point.
>>
>> If you have two devices, detach the first one, then the second one moves
>> to index 0 but will still have alias ending with 1. Then if you cold-add
>> another device that will be put into index 1, and when starting the VM
>> it will get assigned the same alias as the one which has the old one.
>>
>> This applies to all devices where the alias depends on the ordering.
>
> Yep, we would need to maintain a hash table remembering all currently
> assigned aliases, and then increment the counter until we find a free
> one for that dev type.
Alternatively, every time we want to assign an alias for a device we
traverse its siblings and see if it's taken.
for (i = 0; ; i++) {
alias = "device$i";
for (j = 0; j < def->ndevice; j++) {
if (STREQ(def->device[j]->info.alias, alias))
continue;
break
}
if (j != def->ndevice) {
/* alias is free */
device->alias = alias;
break;
}
}
This is roughly the same as your approach except the hash table is taken
out. Which I find better because:
1) the hash table would have to live under qemuDomainObjPrivatePtr (or
under virDomainObjPtr) and I'd have to pass pointer to it all the way
down to virDomainDeviceInfoClear() -> huge change
2) we would have two copies of aliases in memory.
IOW, my approach is more time complex but less memory complex.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Sep 22, 2017 at 10:18:02 +0200, Michal Privoznik wrote:
> On 09/21/2017 07:18 PM, Daniel P. Berrange wrote:
> > On Thu, Sep 21, 2017 at 06:05:19PM +0200, Peter Krempa wrote:
> >> On Thu, Sep 21, 2017 at 16:47:08 +0200, Michal Privoznik wrote:
> >>> In the near future the qemuAssignDeviceAliases() function is
> >>> going to be called multiple times: once at the domain define
> >>> time, then in domain prepare phase. To avoid regenerating the
> >>> same aliases the second time we need to be able to tell the
> >>> function which time it is being called.
> >>>
> >>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> >>> ---
> >>> src/qemu/qemu_alias.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++--
> >>> src/qemu/qemu_alias.h | 4 +-
> >>> src/qemu/qemu_driver.c | 2 +-
> >>> src/qemu/qemu_process.c | 2 +-
> >>> tests/qemuhotplugtest.c | 2 +-
> >>> 5 files changed, 115 insertions(+), 8 deletions(-)
> >>>
>
> >> So if you want to make aliases persistent, at least this will not work
> >> properly with device coldplug.
>
> Ah, good point.
>
> >>
> >> If you have two devices, detach the first one, then the second one moves
> >> to index 0 but will still have alias ending with 1. Then if you cold-add
> >> another device that will be put into index 1, and when starting the VM
> >> it will get assigned the same alias as the one which has the old one.
> >>
> >> This applies to all devices where the alias depends on the ordering.
> >
> > Yep, we would need to maintain a hash table remembering all currently
> > assigned aliases, and then increment the counter until we find a free
> > one for that dev type.
>
> Alternatively, every time we want to assign an alias for a device we
> traverse its siblings and see if it's taken.
>
> for (i = 0; ; i++) {
> alias = "device$i";
> for (j = 0; j < def->ndevice; j++) {
> if (STREQ(def->device[j]->info.alias, alias))
> continue;
> break
> }
> if (j != def->ndevice) {
> /* alias is free */
> device->alias = alias;
> break;
> }
> }
You can also generate them of a global sequence number. Qemu does not
care that they are not consecutive. It might trigger some OCD based eye
twitching, but it's way less yucky.
> This is roughly the same as your approach except the hash table is taken
> out. Which I find better because:
>
> 1) the hash table would have to live under qemuDomainObjPrivatePtr (or
> under virDomainObjPtr) and I'd have to pass pointer to it all the way
> down to virDomainDeviceInfoClear() -> huge change
>
> 2) we would have two copies of aliases in memory.
>
> IOW, my approach is more time complex but less memory complex.
Preferably as with disks or memory modules, you can base the alias on
something which is already guaranteed to be unique (IDE unit number,
DIMM slot number) which breaks the dependancy and gives you uniqueness
for free.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Sep 22, 2017 at 10:25:46AM +0200, Peter Krempa wrote:
> On Fri, Sep 22, 2017 at 10:18:02 +0200, Michal Privoznik wrote:
> > On 09/21/2017 07:18 PM, Daniel P. Berrange wrote:
> > > On Thu, Sep 21, 2017 at 06:05:19PM +0200, Peter Krempa wrote:
> > >> On Thu, Sep 21, 2017 at 16:47:08 +0200, Michal Privoznik wrote:
> > >>> In the near future the qemuAssignDeviceAliases() function is
> > >>> going to be called multiple times: once at the domain define
> > >>> time, then in domain prepare phase. To avoid regenerating the
> > >>> same aliases the second time we need to be able to tell the
> > >>> function which time it is being called.
> > >>>
> > >>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > >>> ---
> > >>> src/qemu/qemu_alias.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++--
> > >>> src/qemu/qemu_alias.h | 4 +-
> > >>> src/qemu/qemu_driver.c | 2 +-
> > >>> src/qemu/qemu_process.c | 2 +-
> > >>> tests/qemuhotplugtest.c | 2 +-
> > >>> 5 files changed, 115 insertions(+), 8 deletions(-)
> > >>>
> >
> > >> So if you want to make aliases persistent, at least this will not work
> > >> properly with device coldplug.
> >
> > Ah, good point.
> >
> > >>
> > >> If you have two devices, detach the first one, then the second one moves
> > >> to index 0 but will still have alias ending with 1. Then if you cold-add
> > >> another device that will be put into index 1, and when starting the VM
> > >> it will get assigned the same alias as the one which has the old one.
> > >>
> > >> This applies to all devices where the alias depends on the ordering.
> > >
> > > Yep, we would need to maintain a hash table remembering all currently
> > > assigned aliases, and then increment the counter until we find a free
> > > one for that dev type.
> >
> > Alternatively, every time we want to assign an alias for a device we
> > traverse its siblings and see if it's taken.
> >
> > for (i = 0; ; i++) {
> > alias = "device$i";
> > for (j = 0; j < def->ndevice; j++) {
> > if (STREQ(def->device[j]->info.alias, alias))
> > continue;
> > break
> > }
> > if (j != def->ndevice) {
> > /* alias is free */
> > device->alias = alias;
> > break;
> > }
> > }
>
> You can also generate them of a global sequence number. Qemu does not
> care that they are not consecutive. It might trigger some OCD based eye
> twitching, but it's way less yucky.
Yeah a single global counter would be reasonable IMHO. Still has a little
cpu time to initialize it when libvirtd starts up & loads persistent
configs, but that's not unreasonable.
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Michal Privoznik <mprivozn@redhat.com> [2017-09-21, 04:47PM +0200]:
> In the near future the qemuAssignDeviceAliases() function is
> going to be called multiple times: once at the domain define
> time, then in domain prepare phase. To avoid regenerating the
> same aliases the second time we need to be able to tell the
> function which time it is being called.
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> src/qemu/qemu_alias.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++--
> src/qemu/qemu_alias.h | 4 +-
> src/qemu/qemu_driver.c | 2 +-
> src/qemu/qemu_process.c | 2 +-
> tests/qemuhotplugtest.c | 2 +-
> 5 files changed, 115 insertions(+), 8 deletions(-)
>
> diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
> index e1431e2a2..00868c50f 100644
> --- a/src/qemu/qemu_alias.c
> +++ b/src/qemu/qemu_alias.c
> @@ -481,25 +481,99 @@ qemuAssignDeviceShmemAlias(virDomainDefPtr def,
> }
>
>
> +/*
> + * qemuAssignDeviceAliases:
> + * @def: domain definition
> + * @qemuCaps: qemu capabilities
> + * @regenerate: forcibly regenerate aliases
> + *
> + * Assign aliases to domain devices. If @regenerate is false only
> + * the missing aliases are generated leaving already assigned
> + * aliases untouched. If @regenerate is true any preexisting
> + * aliases are overwritten.
> + *
> + * Returns 0 on success, -1 otherwise.
> + */
> int
> -qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
> +qemuAssignDeviceAliases(virDomainDefPtr def,
> + virQEMUCapsPtr qemuCaps,
> + bool regenerate)
> {
> size_t i;
>
> + if (regenerate) {
> + /* If we need to regenerate the aliases, we have to free
> + * them all upfront because there are some dependencies,
> + * e.g. an interface can be a hostdev and so on.
> + * Therefore, if we'd do the freeing in the loop below we
> + * might not start from zero. */
> + for (i = 0; i < def->ndisks; i++)
> + VIR_FREE(def->disks[i]->info.alias);
> + for (i = 0; i < def->nnets; i++)
> + VIR_FREE(def->nets[i]->info.alias);
> + for (i = 0; i < def->nfss; i++)
> + VIR_FREE(def->fss[i]->info.alias);
> + for (i = 0; i < def->nsounds; i++)
> + VIR_FREE(def->sounds[i]->info.alias);
> + for (i = 0; i < def->nhostdevs; i++)
> + VIR_FREE(def->hostdevs[i]->info->alias);
> + for (i = 0; i < def->nredirdevs; i++)
> + VIR_FREE(def->redirdevs[i]->info.alias);
> + for (i = 0; i < def->nvideos; i++)
> + VIR_FREE(def->videos[i]->info.alias);
> + for (i = 0; i < def->ncontrollers; i++)
> + VIR_FREE(def->controllers[i]->info.alias);
> + for (i = 0; i < def->ninputs; i++)
> + VIR_FREE(def->inputs[i]->info.alias);
> + for (i = 0; i < def->nparallels; i++)
> + VIR_FREE(def->parallels[i]->info.alias);
> + for (i = 0; i < def->nserials; i++)
> + VIR_FREE(def->serials[i]->info.alias);
> + for (i = 0; i < def->nchannels; i++)
> + VIR_FREE(def->channels[i]->info.alias);
> + for (i = 0; i < def->nconsoles; i++)
> + VIR_FREE(def->consoles[i]->info.alias);
> + for (i = 0; i < def->nhubs; i++)
> + VIR_FREE(def->hubs[i]->info.alias);
> + for (i = 0; i < def->nshmems; i++)
> + VIR_FREE(def->shmems[i]->info.alias);
> + for (i = 0; i < def->nsmartcards; i++)
> + VIR_FREE(def->smartcards[i]->info.alias);
> + if (def->watchdog)
> + VIR_FREE(def->watchdog->info.alias);
> + if (def->memballoon)
> + VIR_FREE(def->memballoon->info.alias);
> + for (i = 0; i < def->nrngs; i++)
> + VIR_FREE(def->rngs[i]->info.alias);
> + if (def->tpm)
> + VIR_FREE(def->tpm->info.alias);
> + for (i = 0; i < def->nmems; i++)
> + VIR_FREE(def->mems[i]->info.alias);
> + }
> +
> +
> for (i = 0; i < def->ndisks; i++) {
> + if (def->disks[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceDiskAlias(def, def->disks[i], qemuCaps) < 0)
> return -1;
> }
> for (i = 0; i < def->nnets; i++) {
> + if (def->nets[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceNetAlias(def, def->nets[i], -1) < 0)
> return -1;
> }
>
> for (i = 0; i < def->nfss; i++) {
> + if (def->fss[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceFSAlias(def->fss[i], i) < 0)
> return -1;
> }
> for (i = 0; i < def->nsounds; i++) {
> + if (def->sounds[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceSoundAlias(def->sounds[i], i) < 0)
> return -1;
> }
> @@ -509,71 +583,102 @@ 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 && !regenerate)
> + continue;
> if (!def->hostdevs[i]->info->alias &&
> qemuAssignDeviceHostdevAlias(def, &def->hostdevs[i]->info->alias, -1) < 0)
> return -1;
> }
> for (i = 0; i < def->nredirdevs; i++) {
> + if (def->redirdevs[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceRedirdevAlias(def, def->redirdevs[i], i) < 0)
> return -1;
> }
> for (i = 0; i < def->nvideos; i++) {
> + if (def->videos[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceVideoAlias(def->videos[i], i) < 0)
> return -1;
> }
> for (i = 0; i < def->ncontrollers; i++) {
> + if (def->controllers[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceControllerAlias(def, qemuCaps, def->controllers[i]) < 0)
> return -1;
> }
> for (i = 0; i < def->ninputs; i++) {
> + if (def->inputs[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceInputAlias(def->inputs[i], i) < 0)
> return -1;
> }
> for (i = 0; i < def->nparallels; i++) {
> + if (def->parallels[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceChrAlias(def, def->parallels[i], i) < 0)
> return -1;
> }
> for (i = 0; i < def->nserials; i++) {
> + if (def->serials[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceChrAlias(def, def->serials[i], i) < 0)
> return -1;
> }
> for (i = 0; i < def->nchannels; i++) {
> + if (def->channels[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceChrAlias(def, def->channels[i], i) < 0)
> return -1;
> }
> for (i = 0; i < def->nconsoles; i++) {
> + if (def->consoles[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceChrAlias(def, def->consoles[i], i) < 0)
> return -1;
> }
> for (i = 0; i < def->nhubs; i++) {
> + if (def->hubs[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceHubAlias(def->hubs[i], i) < 0)
> return -1;
> }
> for (i = 0; i < def->nshmems; i++) {
> + if (def->shmems[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceShmemAlias(def, def->shmems[i], i) < 0)
> return -1;
> }
> for (i = 0; i < def->nsmartcards; i++) {
> + if (def->smartcards[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceSmartcardAlias(def->smartcards[i], i) < 0)
> return -1;
> }
> - if (def->watchdog) {
> + if (def->watchdog &&
> + (!def->watchdog->info.alias || regenerate)) {
Can you make this conditional consistent to the others?
> if (qemuAssignDeviceWatchdogAlias(def->watchdog, 0) < 0)
> return -1;
> }
> - if (def->memballoon) {
> + if (def->memballoon &&
> + (!def->memballoon->info.alias || regenerate)) {
And this.
> if (qemuAssingDeviceMemballoonAlias(def->memballoon, 0) < 0)
> return -1;
> }
> for (i = 0; i < def->nrngs; i++) {
> + if (def->rngs[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceRNGAlias(def, def->rngs[i], i) < 0)
> return -1;
> }
> - if (def->tpm) {
> + if (def->tpm &&
> + (!def->tpm->info.alias || regenerate)) {
This too.
> if (qemuAssignDeviceTPMAlias(def->tpm, 0) < 0)
> return -1;
> }
> for (i = 0; i < def->nmems; i++) {
> + if (def->mems[i]->info.alias && !regenerate)
> + continue;
> if (qemuAssignDeviceMemoryAlias(NULL, def->mems[i], false) < 0)
> return -1;
> }
> diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h
> index 860ab6c0c..e33a05580 100644
> --- a/src/qemu/qemu_alias.h
> +++ b/src/qemu/qemu_alias.h
> @@ -66,7 +66,9 @@ int qemuAssignDeviceShmemAlias(virDomainDefPtr def,
> virDomainShmemDefPtr shmem,
> int idx);
>
> -int qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps);
> +int qemuAssignDeviceAliases(virDomainDefPtr def,
> + virQEMUCapsPtr qemuCaps,
> + bool regenerate);
>
> int qemuDomainDeviceAliasIndex(const virDomainDeviceInfo *info,
> const char *prefix);
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 1b271792d..64ba7d454 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -16145,7 +16145,7 @@ static virDomainPtr qemuDomainQemuAttach(virConnectPtr conn,
> def->emulator)))
> goto cleanup;
>
> - if (qemuAssignDeviceAliases(def, qemuCaps) < 0)
> + if (qemuAssignDeviceAliases(def, qemuCaps, true) < 0)
> goto cleanup;
>
> if (!(vm = virDomainObjListAdd(driver->domains, def,
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index e6cc41e13..448de679c 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -5340,7 +5340,7 @@ qemuProcessPrepareDomain(virConnectPtr conn,
> goto cleanup;
> }
>
> - if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0)
> + if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps, false) < 0)
> goto cleanup;
>
> VIR_DEBUG("Setting graphics devices");
> diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
> index 23a498617..0b1b00176 100644
> --- a/tests/qemuhotplugtest.c
> +++ b/tests/qemuhotplugtest.c
> @@ -95,7 +95,7 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt,
> goto cleanup;
> }
>
> - if (qemuAssignDeviceAliases((*vm)->def, priv->qemuCaps) < 0)
> + if (qemuAssignDeviceAliases((*vm)->def, priv->qemuCaps, true) < 0)
Instead of passing a state flag into the function, would it be possible
to manually clear the aliases before calling qemuAssignDeviceAliases and
dropping the regenerate flag? I find boolean parameters that
fundamentally change the behaviour of a function hard to follow at a
certain time.
> goto cleanup;
>
> (*vm)->def->id = QEMU_HOTPLUG_TEST_DOMAIN_ID;
> --
> 2.13.5
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
IBM Systems
Linux on z Systems & Virtualization Development
------------------------------------------------------------------------
IBM Deutschland
Schönaicher Str. 220
71032 Böblingen
Phone: +49 7031 16 1819
E-Mail: bwalk@de.ibm.com
------------------------------------------------------------------------
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.