[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv7 10/18] conf: Remove virDomainResctrlAppend and introduce virDomainResctrlNew





On 2018年11月06日 01:26, John Ferlan wrote:

On 10/22/18 4:01 AM, Wang Huaqiang wrote:
Introduced virDomainResctrlNew to do the most part of virDomainResctrlAppend
and move the operation of appending resctrl to @def->resctrls out of
function.

Rather than rely on virDomainResctrlAppend to perform the allocation, move
the onus to the caller and make use of virBitmapNewCopy for @vcpus and
virObjectRef for @alloc, thus removing the need to set each to NULL after the
call.

Signed-off-by: Wang Huaqiang <huaqiang wang intel com>
---
  src/conf/domain_conf.c | 60 +++++++++++++++++++++++++++++---------------------
  1 file changed, 35 insertions(+), 25 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e8e0adc..39bd396 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -18920,26 +18920,22 @@ virDomainCachetuneDefParseCache(xmlXPathContextPtr ctxt,
  }
-static int
-virDomainResctrlAppend(virDomainDefPtr def,
-                       xmlNodePtr node,
-                       virResctrlAllocPtr alloc,
-                       virBitmapPtr vcpus,
-                       unsigned int flags)
+static virDomainResctrlDefPtr
+virDomainResctrlNew(xmlNodePtr node,
+                    virResctrlAllocPtr *alloc,
+                    virBitmapPtr *vcpus,
Because we're not "stealing" @*alloc and/or @*vcpus, they do not need to
be passed by reference. I can change these.  There's some minor merge
impact in later patches too, but no big deal.

Agree. Please help make change.



+                    unsigned int flags)
  {
      char *vcpus_str = NULL;
      char *alloc_id = NULL;
-    virDomainResctrlDefPtr tmp_resctrl = NULL;
-    int ret = -1;
-
-    if (VIR_ALLOC(tmp_resctrl) < 0)
-        goto cleanup;
+    virDomainResctrlDefPtr resctrl = NULL;
+    virDomainResctrlDefPtr ret = NULL;
/* We need to format it back because we need to be consistent in the naming
       * even when users specify some "sub-optimal" string there. */
-    vcpus_str = virBitmapFormat(vcpus);
+    vcpus_str = virBitmapFormat(*vcpus);
      if (!vcpus_str)
-        goto cleanup;
+        return NULL;
if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE))
          alloc_id = virXMLPropString(node, "id");
@@ -18954,18 +18950,23 @@ virDomainResctrlAppend(virDomainDefPtr def,
              goto cleanup;
      }
     /* NB: Callers assume new @alloc, need to fill in ID now */

Not that it would prevent someone in the future from passing an @alloc
w/ ->id already filled in and overwriting, but at least for now it's not
the case.

Yes, it might happen.
If @alloc->id is specified through XML and is not following the naming convention
         virAsprintf(&alloc_id, "vcpus_%s", vcpus_str)

If you think it is necessary we might need to through a warning for this case.


With the changes (that I can make),

Reviewed-by: John Ferlan <jferlan redhat com>

John

Thanks for review.
Huaqiang


[...]


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]