[libvirt] [RFCv2 PATCH 1/5] util: Rename and packing parts of virresctrl

[John Ferlan]

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 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. :(

Try to leave a couple of blank lines around your responses - easier to
find that way!

>>
>>>   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

[...]