Re: [libvirt PATCH 01/13] util: Helper functions to get process info

On 11/10/21 9:49 PM, Praveen K Paladugu wrote: > > > On 10/29/2021 7:31 AM, Michal Prívozník wrote: >> On 10/22/21 5:37 PM, Praveen K Paladugu wrote: >>> From: Vineeth Pillai <viremana(a)linux.microsoft.com&gt; >>> >>> These helper methods are used to capture vcpu information >>> in ch driver. >>> >>> Signed-off-by: Vineeth Pillai <viremana(a)linux.microsoft.com&gt; >>> Signed-off-by: Praveen K Paladugu <prapal(a)linux.microsoft.com&gt; >>> --- >>>   src/util/virprocess.c | 136 ++++++++++++++++++++++++++++++++++++++++++ >>>   src/util/virprocess.h |   5 ++ >>>   2 files changed, 141 insertions(+) >>> >>> diff --git a/src/util/virprocess.c b/src/util/virprocess.c >>> index 6de3f36f52..0164d70df6 100644 >>> --- a/src/util/virprocess.c >>> +++ b/src/util/virprocess.c >>> @@ -1721,3 +1721,139 @@ virProcessSetScheduler(pid_t pid G_GNUC_UNUSED, >>>   } >>>     #endif /* !WITH_SCHED_SETSCHEDULER */ >>> + >>> +/* >>> +TODO: This method was cloned from qemuGetProcessInfo in >>> src/qemu/qemu_driver.c. >>> +Need to refactor qemu driver to use this shared function. >> >> There's no harm in doing that in this patch. In fact, it's desired. You >> can move a qemu function into src/util, rename it and fix all places >> where it is called, all in one patch. >> >> Also, please don't forget to export this function in >> src/libvirt_private.syms. >> > I am working on this now. Will make this part of next version. >>> +*/ >>> +int >>> +virProcessGetStatInfo(unsigned long long *cpuTime, int *lastCpu, >>> long *vm_rss, >>> +                   pid_t pid, pid_t tid) >> >> Indentation's off. I've noticed other patches in the series suffer from >> mis-indentation too. Please fix that in another version. >> > Regarding indentation, many of these patches add/modify existing files. > Running indent will modify sections of the files not relevant to the > code changes in this patch set. > Should I run "indent" either way? Or should I make all indentation > changes for all files as a single commit? We like to have one semantic change per commit. Therefore, if you are fixing indentation in a code that's unrelated (i.e. you are fixing indentation in a function you are not touching) then that's considered as another semantic change. What I was aiming at is that new code should be indented well from the beginning. That's obviously one semantic change and as such should be in one patch (i.e. new code that's well formatted). Have I answered your question or did I misunderstand? Michal