From: Bing Niu <bing.niu@intel.com>

Renaming some functions and packing some CAT logic into functions

Signed-off-by: Bing Niu <bing.niu@intel.com>
---
 src/conf/domain_conf.c   |  2 +-
 src/libvirt_private.syms |  2 +-
 src/util/virresctrl.c    | 93 +++++++++++++++++++++++++++++++-----------------
 src/util/virresctrl.h    | 16 ++++-----
 4 files changed, 70 insertions(+), 43 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 522e0c2..62c412e 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -26882,7 +26882,7 @@ virDomainCachetuneDefFormat(virBufferPtr buf,
     int ret = -1;
 
     virBufferSetChildIndent(&childrenBuf, buf);
-    virResctrlAllocForeachSize(cachetune->alloc,
+    virResctrlAllocForeachCache(cachetune->alloc,
                                virDomainCachetuneDefFormatHelper,
                                &childrenBuf);
 
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index ea24f28..fc17059 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2627,7 +2627,7 @@ virCacheTypeToString;
 virResctrlAllocAddPID;
 virResctrlAllocCreate;
 virResctrlAllocDeterminePath;
-virResctrlAllocForeachSize;
+virResctrlAllocForeachCache;
 virResctrlAllocFormat;
 virResctrlAllocGetID;
 virResctrlAllocGetUnused;
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index e492a63..e62061f 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -316,11 +316,9 @@ virResctrlUnlock(int fd)
 }
 
 
-/* virResctrlInfo-related definitions */
 static int
-virResctrlGetInfo(virResctrlInfoPtr resctrl)
+virResctrlGetCacheInfo(virResctrlInfoPtr resctrl, DIR *dirp)
 {
-    DIR *dirp = NULL;
     char *endptr = NULL;
     char *tmp_str = NULL;
     int ret = -1;
@@ -332,12 +330,6 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl)
     virResctrlInfoPerLevelPtr i_level = NULL;
     virResctrlInfoPerTypePtr i_type = NULL;
 
-    rv = virDirOpenIfExists(&dirp, SYSFS_RESCTRL_PATH "/info");
-    if (rv <= 0) {
-        ret = rv;
-        goto cleanup;
-    }
-
     while ((rv = virDirRead(dirp, &ent, SYSFS_RESCTRL_PATH "/info")) > 0) {
         VIR_DEBUG("Parsing info type '%s'", ent->d_name);
         if (ent->d_name[0] != 'L')
@@ -443,12 +435,33 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl)
 
     ret = 0;
  cleanup:
-    VIR_DIR_CLOSE(dirp);
     VIR_FREE(i_type);
     return ret;
 }
 
 
+/* virResctrlInfo-related definitions */
+static int
+virResctrlGetInfo(virResctrlInfoPtr resctrl)
+{
+    DIR *dirp = NULL;
+    int ret = -1;
+
+    ret = virDirOpenIfExists(&dirp, SYSFS_RESCTRL_PATH "/info");
+    if (ret <= 0)
+        goto cleanup;
+
+    ret = virResctrlGetCacheInfo(resctrl, dirp);
+    if (ret < 0)
+        goto cleanup;
+
+    ret = 0;
+ cleanup:
+    VIR_DIR_CLOSE(dirp);
+    return ret;
+}
+
+
 virResctrlInfoPtr
 virResctrlInfoNew(void)
 {
@@ -773,8 +786,8 @@ virResctrlAllocSetSize(virResctrlAllocPtr alloc,
 
 
 int
-virResctrlAllocForeachSize(virResctrlAllocPtr alloc,
-                           virResctrlAllocForeachSizeCallback cb,
+virResctrlAllocForeachCache(virResctrlAllocPtr alloc,
+                           virResctrlAllocForeachCacheCallback cb,
                            void *opaque)
 {
     int ret = 0;
@@ -835,17 +848,14 @@ virResctrlAllocGetID(virResctrlAllocPtr alloc)
 }
 
 
-char *
-virResctrlAllocFormat(virResctrlAllocPtr alloc)
+static int
+virResctrlAllocFormatCache(virResctrlAllocPtr alloc, virBufferPtr buf)
 {
-    virBuffer buf = VIR_BUFFER_INITIALIZER;
+    int ret = -1;
     unsigned int level = 0;
     unsigned int type = 0;
     unsigned int cache = 0;
 
-    if (!alloc)
-        return NULL;
-
     for (level = 0; level < alloc->nlevels; level++) {
         virResctrlAllocPerLevelPtr a_level = alloc->levels[level];
 
@@ -858,7 +868,7 @@ virResctrlAllocFormat(virResctrlAllocPtr alloc)
             if (!a_type)
                 continue;
 
-            virBufferAsprintf(&buf, "L%u%s:", level, virResctrlTypeToString(type));
+            virBufferAsprintf(buf, "L%u%s:", level, virResctrlTypeToString(type));
 
             for (cache = 0; cache < a_type->nmasks; cache++) {
                 virBitmapPtr mask = a_type->masks[cache];
@@ -868,20 +878,37 @@ virResctrlAllocFormat(virResctrlAllocPtr alloc)
                     continue;
 
                 mask_str = virBitmapToString(mask, false, true);
-                if (!mask_str) {
-                    virBufferFreeAndReset(&buf);
-                    return NULL;
-                }
+                if (!mask_str)
+                    return ret;
 
-                virBufferAsprintf(&buf, "%u=%s;", cache, mask_str);
+                virBufferAsprintf(buf, "%u=%s;", cache, mask_str);
                 VIR_FREE(mask_str);
             }
 
-            virBufferTrim(&buf, ";", 1);
-            virBufferAddChar(&buf, '\n');
+            virBufferTrim(buf, ";", 1);
+            virBufferAddChar(buf, '\n');
         }
     }
 
+    ret = 0;
+    virBufferCheckError(buf);
+    return ret;
+}
+
+
+char *
+virResctrlAllocFormat(virResctrlAllocPtr alloc)
+{
+    virBuffer buf = VIR_BUFFER_INITIALIZER;
+
+    if (!alloc)
+        return NULL;
+
+    if (virResctrlAllocFormatCache(alloc, &buf) < 0) {
+        virBufferFreeAndReset(&buf);
+        return NULL;
+    }
+
     virBufferCheckError(&buf);
     return virBufferContentAndReset(&buf);
 }
@@ -939,9 +966,9 @@ virResctrlAllocParseProcessCache(virResctrlInfoPtr resctrl,
 
 
 static int
-virResctrlAllocParseProcessLine(virResctrlInfoPtr resctrl,
-                                virResctrlAllocPtr alloc,
-                                char *line)
+virResctrlAllocParseCacheLine(virResctrlInfoPtr resctrl,
+                              virResctrlAllocPtr alloc,
+                              char *line)
 {
     char **caches = NULL;
     char *tmp = NULL;
@@ -1009,7 +1036,7 @@ virResctrlAllocParse(virResctrlInfoPtr resctrl,
 
     lines = virStringSplitCount(schemata, "\n", 0, &nlines);
     for (i = 0; i < nlines; i++) {
-        if (virResctrlAllocParseProcessLine(resctrl, alloc, lines[i]) < 0)
+        if (virResctrlAllocParseCacheLine(resctrl, alloc, lines[i]) < 0)
             goto cleanup;
     }
 
@@ -1401,8 +1428,8 @@ virResctrlAllocCopyMasks(virResctrlAllocPtr dst,
  * transforming `sizes` into `masks`.
  */
 static int
-virResctrlAllocMasksAssign(virResctrlInfoPtr resctrl,
-                           virResctrlAllocPtr alloc)
+virResctrlAllocAssign(virResctrlInfoPtr resctrl,
+                      virResctrlAllocPtr alloc)
 {
     int ret = -1;
     unsigned int level = 0;
@@ -1526,7 +1553,7 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl,
     if (lockfd < 0)
         goto cleanup;
 
-    if (virResctrlAllocMasksAssign(resctrl, alloc) < 0)
+    if (virResctrlAllocAssign(resctrl, alloc) < 0)
         goto cleanup;
 
     alloc_str = virResctrlAllocFormat(alloc);
diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
index 9052a2b..26c5f3b 100644
--- a/src/util/virresctrl.h
+++ b/src/util/virresctrl.h
@@ -67,11 +67,11 @@ virResctrlInfoGetCache(virResctrlInfoPtr resctrl,
 typedef struct _virResctrlAlloc virResctrlAlloc;
 typedef virResctrlAlloc *virResctrlAllocPtr;
 
-typedef int virResctrlAllocForeachSizeCallback(unsigned int level,
-                                               virCacheType type,
-                                               unsigned int cache,
-                                               unsigned long long size,
-                                               void *opaque);
+typedef int virResctrlAllocForeachCacheCallback(unsigned int level,
+                                                virCacheType type,
+                                                unsigned int cache,
+                                                unsigned long long size,
+                                                void *opaque);
 
 virResctrlAllocPtr
 virResctrlAllocNew(void);
@@ -87,9 +87,9 @@ virResctrlAllocSetSize(virResctrlAllocPtr alloc,
                        unsigned long long size);
 
 int
-virResctrlAllocForeachSize(virResctrlAllocPtr alloc,
-                           virResctrlAllocForeachSizeCallback cb,
-                           void *opaque);
+virResctrlAllocForeachCache(virResctrlAllocPtr alloc,
+                            virResctrlAllocForeachCacheCallback cb,
+                            void *opaque);
 
 int
 virResctrlAllocSetID(virResctrlAllocPtr alloc,
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 06/15/2018 05:29 AM, bing.niu@intel.com wrote:
> From: Bing Niu <bing.niu@intel.com>
> 
> Renaming some functions and packing some CAT logic into functions

Try to do one "thing" per patch - the "and" gives it away...

Thus one patch could rename various functions and other one(s) do the
"refactor" (not packing) of functions (one per refactor).

While it seems a bit odd - it helps keep reviews/changes quick and easy.
It's also very useful when doing bisects to have smaller sets of change
in order to more easily ascertain what broke something else.
> 
> Signed-off-by: Bing Niu <bing.niu@intel.com>
> ---
>  src/conf/domain_conf.c   |  2 +-
>  src/libvirt_private.syms |  2 +-
>  src/util/virresctrl.c    | 93 +++++++++++++++++++++++++++++++-----------------
>  src/util/virresctrl.h    | 16 ++++-----
>  4 files changed, 70 insertions(+), 43 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 522e0c2..62c412e 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -26882,7 +26882,7 @@ virDomainCachetuneDefFormat(virBufferPtr buf,
>      int ret = -1;
>  
>      virBufferSetChildIndent(&childrenBuf, buf);
> -    virResctrlAllocForeachSize(cachetune->alloc,
> +    virResctrlAllocForeachCache(cachetune->alloc,
>                                 virDomainCachetuneDefFormatHelper,
>                                 &childrenBuf);

You added a character to the name and thus will need to adjust the args
to have one more space for proper alignment (e.g. 2nd/3rd args need
another space to align under "cachetune".

>  
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index ea24f28..fc17059 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2627,7 +2627,7 @@ virCacheTypeToString;
>  virResctrlAllocAddPID;
>  virResctrlAllocCreate;
>  virResctrlAllocDeterminePath;
> -virResctrlAllocForeachSize;
> +virResctrlAllocForeachCache;
>  virResctrlAllocFormat;
>  virResctrlAllocGetID;
>  virResctrlAllocGetUnused;
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index e492a63..e62061f 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -316,11 +316,9 @@ virResctrlUnlock(int fd)
>  }
>  
>  
> -/* virResctrlInfo-related definitions */

You could have kept this here instead of keeping it with the new code.

>  static int
> -virResctrlGetInfo(virResctrlInfoPtr resctrl)
> +virResctrlGetCacheInfo(virResctrlInfoPtr resctrl, DIR *dirp)
>  {
> -    DIR *dirp = NULL;
>      char *endptr = NULL;
>      char *tmp_str = NULL;
>      int ret = -1;
> @@ -332,12 +330,6 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl)
>      virResctrlInfoPerLevelPtr i_level = NULL;
>      virResctrlInfoPerTypePtr i_type = NULL;
>  
> -    rv = virDirOpenIfExists(&dirp, SYSFS_RESCTRL_PATH "/info");
> -    if (rv <= 0) {
> -        ret = rv;
> -        goto cleanup;
> -    }
> -
>      while ((rv = virDirRead(dirp, &ent, SYSFS_RESCTRL_PATH "/info")) > 0) {
>          VIR_DEBUG("Parsing info type '%s'", ent->d_name);
>          if (ent->d_name[0] != 'L')
> @@ -443,12 +435,33 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl)
>  
>      ret = 0;
>   cleanup:
> -    VIR_DIR_CLOSE(dirp);
>      VIR_FREE(i_type);
>      return ret;
>  }
>  
>  
> +/* virResctrlInfo-related definitions */
> +static int
> +virResctrlGetInfo(virResctrlInfoPtr resctrl)
> +{
> +    DIR *dirp = NULL;
> +    int ret = -1;
> +
> +    ret = virDirOpenIfExists(&dirp, SYSFS_RESCTRL_PATH "/info");
> +    if (ret <= 0)
> +        goto cleanup;
> +
> +    ret = virResctrlGetCacheInfo(resctrl, dirp);
> +    if (ret < 0)
> +        goto cleanup;
> +
> +    ret = 0;
> + cleanup:
> +    VIR_DIR_CLOSE(dirp);
> +    return ret;
> +}
> +
> +

The refactor of virResctrlGetInfo should get its own patch...

>  virResctrlInfoPtr
>  virResctrlInfoNew(void)
>  {
> @@ -773,8 +786,8 @@ virResctrlAllocSetSize(virResctrlAllocPtr alloc,
>  
>  
>  int
> -virResctrlAllocForeachSize(virResctrlAllocPtr alloc,
> -                           virResctrlAllocForeachSizeCallback cb,
> +virResctrlAllocForeachCache(virResctrlAllocPtr alloc,
> +                           virResctrlAllocForeachCacheCallback cb,
>                             void *opaque)

Again here we need to add space so that the 2nd/3rd args align under
virResctrlAllocPtr.  This is part of the rename patch.

>  {
>      int ret = 0;
> @@ -835,17 +848,14 @@ virResctrlAllocGetID(virResctrlAllocPtr alloc)
>  }
>  
>  
> -char *
> -virResctrlAllocFormat(virResctrlAllocPtr alloc)
> +static int
> +virResctrlAllocFormatCache(virResctrlAllocPtr alloc, virBufferPtr buf)

And here we have a 3rd patch possibility to refactor
virResctrlAllocFormat.  Also one line per argument e.g.:

static int
virResctrlAllocFormatCache(virResctrlAllocPtr alloc,
                           virBufferPtr buf)

>  {
> -    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    int ret = -1;

I think this'll be unnecessary as there's only two places to return
either -1 or 0.

>      unsigned int level = 0;
>      unsigned int type = 0;
>      unsigned int cache = 0;
>  
> -    if (!alloc)
> -        return NULL;
> -
>      for (level = 0; level < alloc->nlevels; level++) {
>          virResctrlAllocPerLevelPtr a_level = alloc->levels[level];
>  
> @@ -858,7 +868,7 @@ virResctrlAllocFormat(virResctrlAllocPtr alloc)
>              if (!a_type)
>                  continue;
>  
> -            virBufferAsprintf(&buf, "L%u%s:", level, virResctrlTypeToString(type));
> +            virBufferAsprintf(buf, "L%u%s:", level, virResctrlTypeToString(type));
>  
>              for (cache = 0; cache < a_type->nmasks; cache++) {
>                  virBitmapPtr mask = a_type->masks[cache];
> @@ -868,20 +878,37 @@ virResctrlAllocFormat(virResctrlAllocPtr alloc)
>                      continue;
>  
>                  mask_str = virBitmapToString(mask, false, true);
> -                if (!mask_str) {
> -                    virBufferFreeAndReset(&buf);
> -                    return NULL;
> -                }
> +                if (!mask_str)
> +                    return ret;

Just return -1

>  
> -                virBufferAsprintf(&buf, "%u=%s;", cache, mask_str);
> +                virBufferAsprintf(buf, "%u=%s;", cache, mask_str);
>                  VIR_FREE(mask_str);
>              }
>  
> -            virBufferTrim(&buf, ";", 1);
> -            virBufferAddChar(&buf, '\n');
> +            virBufferTrim(buf, ";", 1);
> +            virBufferAddChar(buf, '\n');
>          }
>      }
>  
> +    ret = 0;
> +    virBufferCheckError(buf);

^^^^^^ [1]

> +    return ret;

Just return 0

> +}
> +
> +
> +char *
> +virResctrlAllocFormat(virResctrlAllocPtr alloc)
> +{
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> +    if (!alloc)
> +        return NULL;
> +
> +    if (virResctrlAllocFormatCache(alloc, &buf) < 0) {
> +        virBufferFreeAndReset(&buf);
> +        return NULL;
> +    }
> +
>      virBufferCheckError(&buf);

^^^^^
[1] This is duplicated from the called function - I think you keep it
here and remove it from the called function, but I reserve the right to
change my mind.

It's also 'of note' (and existing) that virBufferCheckError (or actually
virBufferCheckErrorInternal) is not a void function. Because you (and
other places in libvirt) don't check return status, I get Coverity
warnings in my Coverity checker environment. There is of course a bite
sized task related to this:

https://wiki.libvirt.org/page/BiteSizedTasks#Clean_up_virBufferCheckError.28.29_callers

but it probably needs to be expanded to combine virBufferCheckError,
virBufferContentAndReset, and virBufferFreeAndReset. Which probably
means it goes beyond a bite sized task.

>      return virBufferContentAndReset(&buf);
>  }

The refactor of virResctrlAllocFormat would be a separate patch.

> @@ -939,9 +966,9 @@ virResctrlAllocParseProcessCache(virResctrlInfoPtr resctrl,
>  
>  
>  static int
> -virResctrlAllocParseProcessLine(virResctrlInfoPtr resctrl,
> -                                virResctrlAllocPtr alloc,
> -                                char *line)
> +virResctrlAllocParseCacheLine(virResctrlInfoPtr resctrl,
> +                              virResctrlAllocPtr alloc,
> +                              char *line)

Here's one for the rename pile...

>  {
>      char **caches = NULL;
>      char *tmp = NULL;
> @@ -1009,7 +1036,7 @@ virResctrlAllocParse(virResctrlInfoPtr resctrl,
>  
>      lines = virStringSplitCount(schemata, "\n", 0, &nlines);
>      for (i = 0; i < nlines; i++) {
> -        if (virResctrlAllocParseProcessLine(resctrl, alloc, lines[i]) < 0)
> +        if (virResctrlAllocParseCacheLine(resctrl, alloc, lines[i]) < 0)
>              goto cleanup;
>      }
>  
> @@ -1401,8 +1428,8 @@ virResctrlAllocCopyMasks(virResctrlAllocPtr dst,
>   * transforming `sizes` into `masks`.
>   */
>  static int
> -virResctrlAllocMasksAssign(virResctrlInfoPtr resctrl,
> -                           virResctrlAllocPtr alloc)
> +virResctrlAllocAssign(virResctrlInfoPtr resctrl,
> +                      virResctrlAllocPtr alloc)

Another in the rename pile...  Although this one becomes more
interesting in the next patch.

John

>  {
>      int ret = -1;
>      unsigned int level = 0;
> @@ -1526,7 +1553,7 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl,
>      if (lockfd < 0)
>          goto cleanup;
>  
> -    if (virResctrlAllocMasksAssign(resctrl, alloc) < 0)
> +    if (virResctrlAllocAssign(resctrl, alloc) < 0)
>          goto cleanup;
>  
>      alloc_str = virResctrlAllocFormat(alloc);

[...]

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 2018年06月30日 06:36, John Ferlan wrote:
> 
> 
> On 06/15/2018 05:29 AM, bing.niu@intel.com wrote:
>> From: Bing Niu <bing.niu@intel.com>
>>
>> Renaming some functions and packing some CAT logic into functions
> 
> Try to do one "thing" per patch - the "and" gives it away...
> 
> Thus one patch could rename various functions and other one(s) do the
> "refactor" (not packing) of functions (one per refactor).
> 
> While it seems a bit odd - it helps keep reviews/changes quick and easy.
> It's also very useful when doing bisects to have smaller sets of change
> in order to more easily ascertain what broke something else.OK. I will put renaming part and refactor into individual patches
>>
>> Signed-off-by: Bing Niu <bing.niu@intel.com>
>> ---
>>   src/conf/domain_conf.c   |  2 +-
>>   src/libvirt_private.syms |  2 +-
>>   src/util/virresctrl.c    | 93 +++++++++++++++++++++++++++++++-----------------
>>   src/util/virresctrl.h    | 16 ++++-----
>>   4 files changed, 70 insertions(+), 43 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 522e0c2..62c412e 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -26882,7 +26882,7 @@ virDomainCachetuneDefFormat(virBufferPtr buf,
>>       int ret = -1;
>>   
>>       virBufferSetChildIndent(&childrenBuf, buf);
>> -    virResctrlAllocForeachSize(cachetune->alloc,
>> +    virResctrlAllocForeachCache(cachetune->alloc,
>>                                  virDomainCachetuneDefFormatHelper,
>>                                  &childrenBuf);
> 
> You added a character to the name and thus will need to adjust the args
> to have one more space for proper alignment (e.g. 2nd/3rd args need
> another space to align under "cachetune".
Yes. I ran syntax-check, unfortunately it didn't throw a error to me. :(
> 
>>   
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index ea24f28..fc17059 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -2627,7 +2627,7 @@ virCacheTypeToString;
>>   virResctrlAllocAddPID;
>>   virResctrlAllocCreate;
>>   virResctrlAllocDeterminePath;
>> -virResctrlAllocForeachSize;
>> +virResctrlAllocForeachCache;
>>   virResctrlAllocFormat;
>>   virResctrlAllocGetID;
>>   virResctrlAllocGetUnused;
>> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
>> index e492a63..e62061f 100644
>> --- a/src/util/virresctrl.c
>> +++ b/src/util/virresctrl.c
>> @@ -316,11 +316,9 @@ virResctrlUnlock(int fd)
>>   }
>>   
>>   
>> -/* virResctrlInfo-related definitions */
> 
> You could have kept this here instead of keeping it with the new code.
That is due to I refactor virResctrlGetInfo and add a 
virResctrlGetCacheInfo before virResctrlGetInfo. Actually I didn't touch 
this line, however the git format diff as that. :(
> 
>>   static int
>> -virResctrlGetInfo(virResctrlInfoPtr resctrl)
>> +virResctrlGetCacheInfo(virResctrlInfoPtr resctrl, DIR *dirp)
>>   {
>> -    DIR *dirp = NULL;
>>       char *endptr = NULL;
>>       char *tmp_str = NULL;
>>       int ret = -1;
>> @@ -332,12 +330,6 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl)
>>       virResctrlInfoPerLevelPtr i_level = NULL;
>>       virResctrlInfoPerTypePtr i_type = NULL;
>>   
>> -    rv = virDirOpenIfExists(&dirp, SYSFS_RESCTRL_PATH "/info");
>> -    if (rv <= 0) {
>> -        ret = rv;
>> -        goto cleanup;
>> -    }
>> -
>>       while ((rv = virDirRead(dirp, &ent, SYSFS_RESCTRL_PATH "/info")) > 0) {
>>           VIR_DEBUG("Parsing info type '%s'", ent->d_name);
>>           if (ent->d_name[0] != 'L')
>> @@ -443,12 +435,33 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl)
>>   
>>       ret = 0;
>>    cleanup:
>> -    VIR_DIR_CLOSE(dirp);
>>       VIR_FREE(i_type);
>>       return ret;
>>   }
>>   
>>   
>> +/* virResctrlInfo-related definitions */
>> +static int
>> +virResctrlGetInfo(virResctrlInfoPtr resctrl)
>> +{
>> +    DIR *dirp = NULL;
>> +    int ret = -1;
>> +
>> +    ret = virDirOpenIfExists(&dirp, SYSFS_RESCTRL_PATH "/info");
>> +    if (ret <= 0)
>> +        goto cleanup;
>> +
>> +    ret = virResctrlGetCacheInfo(resctrl, dirp);
>> +    if (ret < 0)
>> +        goto cleanup;
>> +
>> +    ret = 0;
>> + cleanup:
>> +    VIR_DIR_CLOSE(dirp);
>> +    return ret;
>> +}
>> +
>> +
> 
> The refactor of virResctrlGetInfo should get its own patch...
Will do in next version.
> 
>>   virResctrlInfoPtr
>>   virResctrlInfoNew(void)
>>   {
>> @@ -773,8 +786,8 @@ virResctrlAllocSetSize(virResctrlAllocPtr alloc,
>>   
>>   
>>   int
>> -virResctrlAllocForeachSize(virResctrlAllocPtr alloc,
>> -                           virResctrlAllocForeachSizeCallback cb,
>> +virResctrlAllocForeachCache(virResctrlAllocPtr alloc,
>> +                           virResctrlAllocForeachCacheCallback cb,
>>                              void *opaque)
> 
> Again here we need to add space so that the 2nd/3rd args align under
> virResctrlAllocPtr.  This is part of the rename patch.
Will do in next version.
> 
>>   {
>>       int ret = 0;
>> @@ -835,17 +848,14 @@ virResctrlAllocGetID(virResctrlAllocPtr alloc)
>>   }
>>   
>>   
>> -char *
>> -virResctrlAllocFormat(virResctrlAllocPtr alloc)
>> +static int
>> +virResctrlAllocFormatCache(virResctrlAllocPtr alloc, virBufferPtr buf)
> 
> And here we have a 3rd patch possibility to refactor
> virResctrlAllocFormat.  Also one line per argument e.g.:
Will do in next version. Have a curious question about "one line per 
argument". Usually we separate arguments into multiple lines only if the 
line length for putting in one line beyonds 80m character.
So in libvert's coding convention, we need put one arguments one line 
regardless of line length? Because above line doesn't exceed 80 characters.

> 
> static int
> virResctrlAllocFormatCache(virResctrlAllocPtr alloc,
>                             virBufferPtr buf)
> 
>>   {
>> -    virBuffer buf = VIR_BUFFER_INITIALIZER;
>> +    int ret = -1;
> 
> I think this'll be unnecessary as there's only two places to return
> either -1 or 0.
Will fix in next version.
> 
>>       unsigned int level = 0;
>>       unsigned int type = 0;
>>       unsigned int cache = 0;
>>   
>> -    if (!alloc)
>> -        return NULL;
>> -
>>       for (level = 0; level < alloc->nlevels; level++) {
>>           virResctrlAllocPerLevelPtr a_level = alloc->levels[level];
>>   
>> @@ -858,7 +868,7 @@ virResctrlAllocFormat(virResctrlAllocPtr alloc)
>>               if (!a_type)
>>                   continue;
>>   
>> -            virBufferAsprintf(&buf, "L%u%s:", level, virResctrlTypeToString(type));
>> +            virBufferAsprintf(buf, "L%u%s:", level, virResctrlTypeToString(type));
>>   
>>               for (cache = 0; cache < a_type->nmasks; cache++) {
>>                   virBitmapPtr mask = a_type->masks[cache];
>> @@ -868,20 +878,37 @@ virResctrlAllocFormat(virResctrlAllocPtr alloc)
>>                       continue;
>>   
>>                   mask_str = virBitmapToString(mask, false, true);
>> -                if (!mask_str) {
>> -                    virBufferFreeAndReset(&buf);
>> -                    return NULL;
>> -                }
>> +                if (!mask_str)
>> +                    return ret;
> 
> Just return -1
OK.
> 
>>   
>> -                virBufferAsprintf(&buf, "%u=%s;", cache, mask_str);
>> +                virBufferAsprintf(buf, "%u=%s;", cache, mask_str);
>>                   VIR_FREE(mask_str);
>>               }
>>   
>> -            virBufferTrim(&buf, ";", 1);
>> -            virBufferAddChar(&buf, '\n');
>> +            virBufferTrim(buf, ";", 1);
>> +            virBufferAddChar(buf, '\n');
>>           }
>>       }
>>   
>> +    ret = 0;
>> +    virBufferCheckError(buf);
> 
> ^^^^^^ [1]
Ok. I will remove it from caller and let each callee do 
virBufferCheckError. And add return value testing parts.
> 
>> +    return ret;
> 
> Just return 0
OK!
> 
>> +}
>> +
>> +
>> +char *
>> +virResctrlAllocFormat(virResctrlAllocPtr alloc)
>> +{
>> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
>> +
>> +    if (!alloc)
>> +        return NULL;
>> +
>> +    if (virResctrlAllocFormatCache(alloc, &buf) < 0) {
>> +        virBufferFreeAndReset(&buf);
>> +        return NULL;
>> +    }
>> +
>>       virBufferCheckError(&buf);
> 
> ^^^^^
> [1] This is duplicated from the called function - I think you keep it
> here and remove it from the called function, but I reserve the right to
> change my mind.
> 
> It's also 'of note' (and existing) that virBufferCheckError (or actually
> virBufferCheckErrorInternal) is not a void function. Because you (and
> other places in libvirt) don't check return status, I get Coverity
> warnings in my Coverity checker environment. There is of course a bite
> sized task related to this:
> 
> https://wiki.libvirt.org/page/BiteSizedTasks#Clean_up_virBufferCheckError.28.29_callers
> 
> but it probably needs to be expanded to combine virBufferCheckError,
> virBufferContentAndReset, and virBufferFreeAndReset. Which probably
> means it goes beyond a bite sized task.
Thanks for the clarification here. :)
> 
>>       return virBufferContentAndReset(&buf);
>>   }
> 
> The refactor of virResctrlAllocFormat would be a separate patch.
> 
>> @@ -939,9 +966,9 @@ virResctrlAllocParseProcessCache(virResctrlInfoPtr resctrl,
>>   
>>   
>>   static int
>> -virResctrlAllocParseProcessLine(virResctrlInfoPtr resctrl,
>> -                                virResctrlAllocPtr alloc,
>> -                                char *line)
>> +virResctrlAllocParseCacheLine(virResctrlInfoPtr resctrl,
>> +                              virResctrlAllocPtr alloc,
>> +                              char *line)
> 
> Here's one for the rename pile...
OK!
> 
>>   {
>>       char **caches = NULL;
>>       char *tmp = NULL;
>> @@ -1009,7 +1036,7 @@ virResctrlAllocParse(virResctrlInfoPtr resctrl,
>>   
>>       lines = virStringSplitCount(schemata, "\n", 0, &nlines);
>>       for (i = 0; i < nlines; i++) {
>> -        if (virResctrlAllocParseProcessLine(resctrl, alloc, lines[i]) < 0)
>> +        if (virResctrlAllocParseCacheLine(resctrl, alloc, lines[i]) < 0)
>>               goto cleanup;
>>       }
>>   
>> @@ -1401,8 +1428,8 @@ virResctrlAllocCopyMasks(virResctrlAllocPtr dst,
>>    * transforming `sizes` into `masks`.
>>    */
>>   static int
>> -virResctrlAllocMasksAssign(virResctrlInfoPtr resctrl,
>> -                           virResctrlAllocPtr alloc)
>> +virResctrlAllocAssign(virResctrlInfoPtr resctrl,
>> +                      virResctrlAllocPtr alloc)
> 
> Another in the rename pile...  Although this one becomes more
> interesting in the next patch.
OK!

Bing
> 
> John
> 
>>   {
>>       int ret = -1;
>>       unsigned int level = 0;
>> @@ -1526,7 +1553,7 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl,
>>       if (lockfd < 0)
>>           goto cleanup;
>>   
>> -    if (virResctrlAllocMasksAssign(resctrl, alloc) < 0)
>> +    if (virResctrlAllocAssign(resctrl, alloc) < 0)
>>           goto cleanup;
>>   
>>       alloc_str = virResctrlAllocFormat(alloc);
> 
> [...]
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 07/06/2018 03:00 AM, bing.niu wrote:
> 
> 
> On 2018年06月30日 06:36, John Ferlan wrote:
>>
>>
>> On 06/15/2018 05:29 AM, bing.niu@intel.com wrote:
>>> From: Bing Niu <bing.niu@intel.com>
>>>
>>> Renaming some functions and packing some CAT logic into functions
>>
>> Try to do one "thing" per patch - the "and" gives it away...
>>
>> Thus one patch could rename various functions and other one(s) do the
>> "refactor" (not packing) of functions (one per refactor).
>>
>> While it seems a bit odd - it helps keep reviews/changes quick and easy.
>> It's also very useful when doing bisects to have smaller sets of change
>> in order to more easily ascertain what broke something else.OK. I will
>> put renaming part and refactor into individual patches
>>>
>>> Signed-off-by: Bing Niu <bing.niu@intel.com>
>>> ---
>>>   src/conf/domain_conf.c   |  2 +-
>>>   src/libvirt_private.syms |  2 +-
>>>   src/util/virresctrl.c    | 93
>>> +++++++++++++++++++++++++++++++-----------------
>>>   src/util/virresctrl.h    | 16 ++++-----
>>>   4 files changed, 70 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index 522e0c2..62c412e 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -26882,7 +26882,7 @@ virDomainCachetuneDefFormat(virBufferPtr buf,
>>>       int ret = -1;
>>>         virBufferSetChildIndent(&childrenBuf, buf);
>>> -    virResctrlAllocForeachSize(cachetune->alloc,
>>> +    virResctrlAllocForeachCache(cachetune->alloc,
>>>                                  virDomainCachetuneDefFormatHelper,
>>>                                  &childrenBuf);
>>
>> You added a character to the name and thus will need to adjust the args
>> to have one more space for proper alignment (e.g. 2nd/3rd args need
>> another space to align under "cachetune".
> Yes. I ran syntax-check, unfortunately it didn't throw a error to me. :(

Try to leave a couple of blank lines around your responses - easier to
find that way!

>>
>>>   diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>>> index ea24f28..fc17059 100644
>>> --- a/src/libvirt_private.syms
>>> +++ b/src/libvirt_private.syms
>>> @@ -2627,7 +2627,7 @@ virCacheTypeToString;
>>>   virResctrlAllocAddPID;
>>>   virResctrlAllocCreate;
>>>   virResctrlAllocDeterminePath;
>>> -virResctrlAllocForeachSize;
>>> +virResctrlAllocForeachCache;
>>>   virResctrlAllocFormat;
>>>   virResctrlAllocGetID;
>>>   virResctrlAllocGetUnused;
>>> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
>>> index e492a63..e62061f 100644
>>> --- a/src/util/virresctrl.c
>>> +++ b/src/util/virresctrl.c
>>> @@ -316,11 +316,9 @@ virResctrlUnlock(int fd)
>>>   }
>>>     -/* virResctrlInfo-related definitions */
>>
>> You could have kept this here instead of keeping it with the new code.
> That is due to I refactor virResctrlGetInfo and add a
> virResctrlGetCacheInfo before virResctrlGetInfo. Actually I didn't touch
> this line, however the git format diff as that. :(

Ahhh - makes sense. I've seen git am do strange things in the past.
Sometimes I have to run with -3 for diff resolution and that seems to
bring out the phenomena more. Cannot remember for this series though.

>>

[...]

>>> -virResctrlAllocForeachSize(virResctrlAllocPtr alloc,
>>> -                           virResctrlAllocForeachSizeCallback cb,
>>> +virResctrlAllocForeachCache(virResctrlAllocPtr alloc,
>>> +                           virResctrlAllocForeachCacheCallback cb,
>>>                              void *opaque)
>>
>> Again here we need to add space so that the 2nd/3rd args align under
>> virResctrlAllocPtr.  This is part of the rename patch.
> Will do in next version.
>>
>>>   {
>>>       int ret = 0;
>>> @@ -835,17 +848,14 @@ virResctrlAllocGetID(virResctrlAllocPtr alloc)
>>>   }
>>>     -char *
>>> -virResctrlAllocFormat(virResctrlAllocPtr alloc)
>>> +static int
>>> +virResctrlAllocFormatCache(virResctrlAllocPtr alloc, virBufferPtr buf)
>>
>> And here we have a 3rd patch possibility to refactor
>> virResctrlAllocFormat.  Also one line per argument e.g.:
> Will do in next version. Have a curious question about "one line per
> argument". Usually we separate arguments into multiple lines only if the
> line length for putting in one line beyonds 80m character.
> So in libvert's coding convention, we need put one arguments one line
> regardless of line length? Because above line doesn't exceed 80 characters.
> 

Right, understood. Just trying to follow what other patches have
requested - cannot recall if it's a "strict policy" on the hacking page
though.  The formatting bikeshed also can differ depending on reviewers.

John

[...]

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list