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... 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);
[...]