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;