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@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