On 11/6/2018 3:15 AM, John Ferlan wrote:
> On 10/22/18 4:01 AM, Wang Huaqiang wrote:
>> Adding element 'id' to virDomainResctrlDef tracking resource group
>> id, it reflects the attribute 'id' of of element <cachetune> in XML.
>>
>> virResctrlAlloc.id is a copy from virDomanResctrlDef.id.
>>
>> Signed-off-by: Wang Huaqiang<huaqiang.wang@intel.com>
>> ---
>> src/conf/domain_conf.c | 20 ++++++++------------
>> src/conf/domain_conf.h | 1 +
>> 2 files changed, 9 insertions(+), 12 deletions(-)
>>
> Not sure I see the need to duplicate the alloc->id value especially
> since it's "invalid" have "resctrl->alloc == NULL", right?
>
> Can this resctrl->id ever be different? Not that I can see. I see this
> is a desire to reduce an error case in Format processing, but if
> virResctrlAllocGetID returned NULL there's other problems...
>
> John
This patch should be removed and will be removed.
It is introduced in series v4, in that series the 'resctrl->alloc' is
allowed
to be NULL representing a default monitor.
Now we changed our design and resctrl->alloc is always assigned with an
virResctrlAlloc data structure, then 'resctrl->id' is not necessary. We
can keep the
resctrl ID in alloc->id.
Thanks for review.
Huaqiang
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 01f795f..1aecd8c 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -2980,6 +2980,7 @@ virDomainResctrlDefFree(virDomainResctrlDefPtr resctrl)
>> virObjectUnref(resctrl->alloc);
>> virBitmapFree(resctrl->vcpus);
>> VIR_FREE(resctrl->monitors);
>> + VIR_FREE(resctrl->id);
>> VIR_FREE(resctrl);
>> }
>>
>> @@ -19145,6 +19146,9 @@ virDomainResctrlNew(xmlNodePtr node,
>> if (VIR_ALLOC(resctrl) < 0)
>> goto cleanup;
>>
>> + if (VIR_STRDUP(resctrl->id, alloc_id) < 0)
>> + goto cleanup;
>> +
>> if (!(resctrl->vcpus = virBitmapNewCopy(*vcpus))) {
>> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> _("failed to copy 'vcpus'"));
>> @@ -27323,13 +27327,9 @@ virDomainCachetuneDefFormat(virBufferPtr buf,
>>
>> virBufferAsprintf(buf, "<cachetune vcpus='%s'", vcpus);
>>
>> - if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) {
>> - const char *alloc_id = virResctrlAllocGetID(resctrl->alloc);
>> - if (!alloc_id)
>> - goto cleanup;
>> + if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE))
>> + virBufferAsprintf(buf, " id='%s'", resctrl->id);
>>
>> - virBufferAsprintf(buf, " id='%s'", alloc_id);
>> - }
>> virBufferAddLit(buf, ">\n");
>>
>> virBufferAddBuffer(buf, &childrenBuf);
>> @@ -27386,13 +27386,9 @@ virDomainMemorytuneDefFormat(virBufferPtr buf,
>>
>> virBufferAsprintf(buf, "<memorytune vcpus='%s'", vcpus);
>>
>> - if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) {
>> - const char *alloc_id = virResctrlAllocGetID(resctrl->alloc);
>> - if (!alloc_id)
>> - goto cleanup;
>> + if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE))
>> + virBufferAsprintf(buf, " id='%s'", resctrl->id);
>>
>> - virBufferAsprintf(buf, " id='%s'", alloc_id);
>> - }
>> virBufferAddLit(buf, ">\n");
>>
>> virBufferAddBuffer(buf, &childrenBuf);
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index 60f6464..e190aa2 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -2248,6 +2248,7 @@ typedef struct _virDomainResctrlDef virDomainResctrlDef;
>> typedef virDomainResctrlDef *virDomainResctrlDefPtr;
>>
>> struct _virDomainResctrlDef {
>> + char *id;
>> virBitmapPtr vcpus;
>> virResctrlAllocPtr alloc;
>>
>>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list