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
