[libvirt] [PATCH v2]Pre-assign memory modules slot number and assign alias.

[Peter Krempa]

On Thu, Sep 29, 2016 at 22:52:46 +0530, Nitesh Konkar wrote:
> Find the smallest missing slot number and
> pre-assign slot numbers and assign aliases
> of memory modules. This keeps the slot number
> consistent with the alias.
> 
> Signed-off-by: Nitesh Konkar <nitkon12 at linux.vnet.ibm.com>
> ---
>  src/qemu/qemu_alias.c   | 54 +++++++++++++++++++++++++++++++++++++++----------
>  src/qemu/qemu_command.c |  5 ++++-
>  2 files changed, 47 insertions(+), 12 deletions(-)
> 
> diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
> index cc83fec..cdca69a 100644
> --- a/src/qemu/qemu_alias.c
> +++ b/src/qemu/qemu_alias.c
> @@ -331,23 +331,49 @@ qemuAssignDeviceRNGAlias(virDomainDefPtr def,
>      return 0;
>  }
>  
> +static int
> +qemuGetSmallestSlotIdx(virDomainDefPtr def)
> +{
> +    size_t i;
> +    int idx = 0;
> +    int minidx = 0;
> +    bool check[100] = {false};

This won't work. Choosing an arbitrary limit is wrong. Especially since
you know the range of values beforehand.

> +
> +    /* Find the missing slot */
> +    for (i = 0; i < def->nmems; i++) {
> +        idx = qemuDomainDeviceAliasIndex(&def->mems[i]->info, "dimm");

As I've said previously, it's not really necessary to parse the slot
number from the alias. It's necessary to assign the alias according to
the slot number. This also means that you can look at the value rather
than parsing it.

> +        check[idx] = true;
> +    }
> +
> +    for (i = 0; i < def->nmems; i++) {
> +        if (!check[i]) {
> +            minidx = i;
> +            break;
> +        }

Umm ... no. Using virBitmap would be an option here, but really this is
not necessary at all.

> +    }
> +
> +    if (i >= def->nmems)
> +       minidx = i;
> +
> +    return minidx;
> +}
>  
>  int
>  qemuAssignDeviceMemoryAlias(virDomainDefPtr def,
>                              virDomainMemoryDefPtr mem)
>  {
> -    size_t i;
> -    int maxidx = 0;
> -    int idx;
> +    int minidx;
>  
> -    for (i = 0; i < def->nmems; i++) {
> -        if ((idx = qemuDomainDeviceAliasIndex(&def->mems[i]->info, "dimm")) >= maxidx)
> -            maxidx = idx + 1;
> -    }
> +    if (mem->info.addr.dimm.base)
> +        minidx = mem->info.addr.dimm.slot;
> +    else
> +        minidx = qemuGetSmallestSlotIdx(def);
>  
> -    if (virAsprintf(&mem->info.alias, "dimm%d", maxidx) < 0)
> +    if (virAsprintf(&mem->info.alias, "dimm%d", minidx) < 0)
>          return -1;
>  
> +    mem->info.addr.dimm.slot = minidx;
> +
>      return 0;
>  }
>  
> @@ -381,7 +407,6 @@ qemuAssignDeviceShmemAlias(virDomainDefPtr def,
>      return 0;
>  }
>  
> -

Spurious whitespace change.

>  int
>  qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
>  {
> @@ -475,8 +500,15 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
>              return -1;

This function should just assign the alias. The address allocation does
not belong here at all.

>      }
>      for (i = 0; i < def->nmems; i++) {
> -        if (virAsprintf(&def->mems[i]->info.alias, "dimm%zu", i) < 0)
> -            return -1;
> +        def->mems[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM;
> +        if (def->mems[i]->info.addr.dimm.base) {
> +            if (virAsprintf(&def->mems[i]->info.alias, "dimm%d", def->mems[i]->info.addr.dimm.slot) < 0)
> +                return -1;
> +        } else {
> +            def->mems[i]->info.addr.dimm.slot = i;
> +            if (virAsprintf(&def->mems[i]->info.alias, "dimm%zu", i) < 0)

If the slots are partially assigned manually and partially not assigned
this will break eventually since you can get into a situation where the
manualy assigned slot number won't be on the correct index.

> +                return -1;
> +        }
>      }
>  
>      return 0;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index f24a98b..e236918 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3469,8 +3469,11 @@ qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem)
>                            mem->info.alias, mem->info.alias);
>  
>          if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) {
> +            if (mem->info.addr.dimm.base)
> +                virBufferAsprintf(&buf, ",addr=%llu", mem->info.addr.dimm.base);
> +
>              virBufferAsprintf(&buf, ",slot=%d", mem->info.addr.dimm.slot);
> -            virBufferAsprintf(&buf, ",addr=%llu", mem->info.addr.dimm.base);
> +
>          }
>  
>          break;

Missing test-cases for command line changes.

I'll fix this patch and repost it.

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160930/48ec885b/attachment-0001.sig>