2 a.m.
On 2018年06月30日 06:36, John Ferlan wrote:
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.OK. I will put renaming part
and refactor into individual patches
>
> 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".
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...
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);
[...]