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@xxxxxxxxx wrote:
>>> From: Bing Niu <bing.niu@xxxxxxxxx>
>>>
>>> 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@xxxxxxxxx>
>>> ---
>>> 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
[...]
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
