[libvirt] [RFCv2 PATCH 1/5] util: Rename and packing parts of virresctrl
bing.niu
bing.niu at intel.com
Fri Jul 6 07:00:11 UTC 2018
On 2018年06月30日 06:36, John Ferlan wrote:
>
>
> On 06/15/2018 05:29 AM, bing.niu at intel.com wrote:
>> From: Bing Niu <bing.niu at 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 at 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.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.
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);
>
> [...]
>
More information about the libvir-list
mailing list