5:36 p.m.
On 06/15/2018 05:29 AM, bing.niu(a)intel.com wrote:
From: Bing Niu <bing.niu(a)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(a)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...
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);
[...]