[libvirt] [PATCHv2] qemu: add a check for slot and base when build dimm address

[John Ferlan]

On 05/27/2015 05:50 AM, Luyao Huang wrote:
> When hot-plug a memory device, we don't check if there
> is a memory device have the same address with the memory device
> we want hot-pluged. Qemu forbid use/hot-plug 2 memory device
> with same slot or the same base(qemu side this elemnt named addr).
> 
> Introduce a address check when build memory device qemu command line.
> 
> Signed-off-by: Luyao Huang <lhuang at redhat.com>
> ---
> v2:
>  move the check to qemuBuildMemoryDeviceStr() to check the dimm address
>  when start/hot-plug a memory device.
> 
>  src/qemu/qemu_command.c | 77 ++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 57 insertions(+), 20 deletions(-)
> 

Perhaps a bit easier to review if this was split into two patches the
first being purely code motion and the second being fixing the
problem...  That said, I don't think the refactor is necessary. I've
attached an alternative and some notes inline below...

John

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index d8ce511..0380a3b 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4955,6 +4955,61 @@ qemuBuildMemoryDimmBackendStr(virDomainMemoryDefPtr mem,
>  }
>  
>  
> +static int
> +qemuBuildMemoryDeviceAddr(virBuffer *buf,
> +                          virDomainDefPtr def,
> +                          virDomainMemoryDefPtr mem)

static bool
qemuCheckMemoryDimmConflict(virDomainDefPtr def,
                            virDomainMemoryDefPtr mem)

> +{
> +    ssize_t i;

size_t usually, then keep the following checks in the caller

> +
> +    if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
> +        return 0;
> +
> +    if (mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("only 'dimm' addresses are supported for the "
> +                         "pc-dimm device"));
> +        return -1;
> +    }
> +
> +    if (mem->info.addr.dimm.slot >= def->mem.memory_slots) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("memory device slot '%u' exceeds slots count '%u'"),
> +                       mem->info.addr.dimm.slot, def->mem.memory_slots);
> +        return -1;
> +    }
> +

Thru here... Since it seems the following is your bugfix correct?

> +    for (i = 0; i < def->nmems; i++) {
> +         virDomainMemoryDefPtr tmp = def->mems[i];
> +
> +         if (tmp == mem ||
> +             tmp->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM)
> +             continue;
> +
> +         if (mem->info.addr.dimm.slot == tmp->info.addr.dimm.slot) {
> +             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                            _("memory device slot '%u' already been"
> +                              " used by other memory device"),

...is already being used by another

> +                            mem->info.addr.dimm.slot);
> +             return -1;

return true;

> +         }
> +
> +         if (mem->info.addr.dimm.base != 0 && tmp->info.addr.dimm.base != 0 &&
> +             mem->info.addr.dimm.base == tmp->info.addr.dimm.base) {
> +             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                            _("memory device base '0x%llx' already been"

...is already being used by another...

> +                              " used by other memory device"),
> +                            mem->info.addr.dimm.base);
> +             return -1;

return true

> +         }
> +    }

return false;

Keep remainder in caller

> +
> +    virBufferAsprintf(buf, ",slot=%d", mem->info.addr.dimm.slot);
> +    virBufferAsprintf(buf, ",addr=%llu", mem->info.addr.dimm.base);
> +
> +    return 0;
> +}
> +
>  char *
>  qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem,
>                           virDomainDefPtr def,
> @@ -4976,29 +5031,11 @@ qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem,
>              return NULL;
>          }
>  
> -        if (mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM &&
> -            mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                           _("only 'dimm' addresses are supported for the "
> -                             "pc-dimm device"));

Your refactor adjusts this test, so if the type was something else then
the virBufferAsprintf happens before the error... it's a nit, but future
adjustments could do something unexpected...

> -            return NULL;
> -        }
> -
> -        if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM &&
> -            mem->info.addr.dimm.slot >= def->mem.memory_slots) {
> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                           _("memory device slot '%u' exceeds slots count '%u'"),
> -                           mem->info.addr.dimm.slot, def->mem.memory_slots);
> -            return NULL;
> -        }
> -

Rather than refactoring - why not change the purpose of the called
routine...

    if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM &&
        qemuCheckMemoryDimmConflict(def, mem))
        return NULL;

>          virBufferAsprintf(&buf, "pc-dimm,node=%d,memdev=mem%s,id=%s",
>                            mem->targetNode, mem->info.alias, mem->info.alias);
>  
> -        if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) {
> -            virBufferAsprintf(&buf, ",slot=%d", mem->info.addr.dimm.slot);
> -            virBufferAsprintf(&buf, ",addr=%llu", mem->info.addr.dimm.base);
> -        }
> +        if (qemuBuildMemoryDeviceAddr(&buf, def, mem) < 0)
> +            return NULL;
>  
>          break;
>  
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-qemu-Add-a-check-for-slot-and-base-dimm-address-conf.patch
Type: text/x-patch
Size: 2644 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150610/b2a68fb9/attachment-0001.bin>