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@intel.com wrote:
From: Bing Niu <bing.niu@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@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 [...]