Re: [libvirt] [PATCH v1 01/18] add macros for implementing automatic cleanup functionality
On Wed, Jun 06, 2018 at 06:42:29PM +0530, Sukrit Bhatnagar wrote:
On Tue, 5 Jun 2018 at 21:00, Erik Skultety
<eskultet(a)redhat.com> wrote:
>
> On Sun, Jun 03, 2018 at 01:41:59PM +0530, Sukrit Bhatnagar wrote:
> > New macros are added to src/util/viralloc.h which help in
> > adding cleanup attribute to variable declarations.
> >
> > Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
>
> If you recall the recent RFC thread [1], I agreed with Pavel that we should
> introduce complete changes to each file, so that we don't need to track was has
> been already done for what file and what is still missing, so the series would
> look something like this:
>
> 1) vir<some_file>.c preparations (especially VIR_{AUTOPTR,AUTOCLEAR} might
need
> that)
> 2) Use VIR_AUTOFREE across vir<some_file>.c
> 3) Use VIR_AUTOPTR across vir<some_file>.c
> 4) Use VIR_AUTOCLEAR across vir<some_file>.c
>
> ...One more thing, the commit subject of every patch in the current series
> should probably look like this:
> util: <module>: Use VIR_AUTOFREE instead of VIR_FREE for scalar types
>
> and the corresponding commit message:
> By making use of the GCC's __attribute__((cleanup)) handled by VIR_AUTOFREE
> macro, majority of the VIR_FREE calls can be dropped, which in turn leads to
> getting rid of most of our cleanup sections.
>
> [1] https://www.redhat.com/archives/libvir-list/2018-May/msg02391.html
I have some rudimentary queries regarding this approach:
- The <module> in commit subject should be like "vircgroup" or just
"cgroup"?
preferably "cgroup" (you'll save 3 chars :)), but doesn't really matter,
I
think I pushed a patch yesterday where I actually added the 'vir' prefix, so
whatever...
- Do I create a separate series of patch even for those files who do
not require
VIR_AUTOPTR and VIR_AUTOCLEAR but only VIR_AUTOFREE,
like viraudit.c?
I'm getting the impression we're not on the same page yet, I suppose I should
have added something like "..." in the example above or simply be more clear in
general.
So, let's be more specific, this series was 18 patches long and you need to add
2 more patches per file, so that would be 54 patches in a single series, that's
big, so how about taking 5-10 files instead of 18, that would produce roughly
15-30 patches in a single series (a bit more manageable and as I mentioned
somewhere else, complete changes that are fine can be merged independently) -
so it's not going to be a separate series per file, rather a single series,
following the pattern I mentioned in my previous response above, performing
changes to up to N selected files, does it make more sense now?
- Do I use the same commit subject and message, with minor changes of
course,
for all patches in a series (AUTOFREE, AUTOPTR and AUTOCLEAR)?
Yep.
- In the case of files like virauth.c which require changes in
virauthconfig.h instead
of virauth.h, do I batch all the files virauth* together?
Yes, well, it should be coupled based on the change you're making, but you just
mentioned it - "required changes" - IOW the patch would not otherwise compile.
I am assuming that the changes to header file vir<some_file>.h
corresponding to vir<some_file>.c will be in the same series if needed.
See above, but yes.
Let me know if there's still something which is not clear.
Erik