[libvirt] [PATCH v1 4/7] qemu: Allow regeneration of aliases

Bjoern Walk bwalk at linux.vnet.ibm.com
Thu Oct 5 08:11:55 UTC 2017


Michal Privoznik <mprivozn at 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 at 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 at 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 at 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 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 906 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20171005/a2e39e7c/attachment-0001.sig>


More information about the libvir-list mailing list