Re: [RFCv2 PATCH 1/5] util: Rename and packing parts of virresctrl

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

 




On 06/15/2018 05:29 AM, bing.niu@xxxxxxxxx wrote:
> From: Bing Niu <bing.niu@xxxxxxxxx>
> 
> 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@xxxxxxxxx>
> ---
>  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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux