5:54 p.m.
On 07/06/2018 03:00 AM, bing.niu wrote:
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>
Try to leave a couple of blank lines around your responses - easier to
find that way!
>
>> 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. :(
Ahhh - makes sense. I've seen git am do strange things in the past.
Sometimes I have to run with -3 for diff resolution and that seems to
bring out the phenomena more. Cannot remember for this series though.
>
[...]
>>> -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.
>
Right, understood. Just trying to follow what other patches have
requested - cannot recall if it's a "strict policy" on the hacking page
though. The formatting bikeshed also can differ depending on reviewers.
John
[...]