[libvirt] [PATCH v1 01/18] add macros for implementing automatic cleanup functionality

[Sukrit Bhatnagar]

On Wed, 6 Jun 2018 at 21:25, Erik Skultety <eskultet at redhat.com> wrote:
>
> On Wed, Jun 06, 2018 at 06:42:29PM +0530, Sukrit Bhatnagar wrote:
> > On Tue, 5 Jun 2018 at 21:00, Erik Skultety <eskultet at 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 at 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?

Yes, it does 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.

No other queries for now, thanks!