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

[lhuang]

On 06/11/2015 03:12 AM, John Ferlan wrote:
>
> 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...

Yes, i should split these changes in two patches, however i think you 
are right
i will remove the unnecessary refactor in next version, so no need split ;)

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

Okay

>
>> +
>> +    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?

Yes, this is bugfix part

>
>> +    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...

Thanks for pointing out this.

>
>> +                              " 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...

Right, i forgot this :)

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

Yes, the refactor is unnecessary. I just thought i should move all the 
check for pc-dimm
address in another function to check and build the address and make the 
code more clean
(because after introduce my bugfix part in qemuBuildMemoryDeviceStr() 
function the code
looks bloated and ugly).

Thanks for your review and i will write a version these days.

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

Luyao