On Mon, Jul 30, 2018 at 12:30:09PM +0200, Pavel Hrdina wrote:
> On Mon, Jul 30, 2018 at 09:58:41AM +0100, Daniel P. Berrangé wrote:
> > On Mon, Jul 30, 2018 at 09:48:51AM +0100, Daniel P. Berrangé wrote:
> > > On Mon, Jul 30, 2018 at 10:25:05AM +0200, Michal Privoznik wrote:
> > > > One of the attributes that original virCgroupFree() had was it
> > > > set passed pointer to NULL. For instance in the following code
> > > > the latter call would be practically a no-op:
> > > >
> > > > virCgroupFree(&var);
> > > > virCgroupFree(&var);
> > > >
> > > > However, this behaviour of the function was changed in
> > > > 0f80c71822d824 but corresponding 'var = NULL' lines were not
> > > > added leading to double free:
> > >
> > > Sigh, can we please just revert that change. It is going in completely the
> > > oppposite of what we should be doing. We want to change more functions to
> > > take a ptr to a ptr, precisely because it avoids this double-free problem.
>
> I agree that this change was not correct and we should revert it, the
> ultimate goal should be to have all the *Free() functions to take
> double pointer and set the variable to NULL as well.
>
> > Even more crazy, this change was done so that VIR_DEFINE_AUTOPTR_FUNC()
> > could be used to define a free function which takes a ptr to a ptr.
> >
> > Both of these changes should be reverted, as the previously existing
> > virCgroupFree should be used as the attribute((cleanup)) function directly
> > with no wrapper created.
>
> We already had this discussion, there are two possibilities:
>
> 1) as the end goal simple wrapper function defined by MACRO:
>
> # define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> static inline void VIR_AUTOPTR_FUNC_NAME(type)(type **_ptr) \
> { \
> (func)(_ptr); \
> } \
>
> It's a static inline so it will disappear during compilation. As it
> may look like unnecessary code there is one benefit while using
> VIR_AUTOPTR()
>
> 2) the second option is to have a macro that would replace
> __attribute__(cleanup()), for example:
>
> VIR_CLEANUP(func) __attribute__(cleanup(func))
>
>
> If you compare the usage:
>
> VIR_AUTOPTR(virCgroup) group = NULL;
>
> VIR_CLEANUP(virCgroupFree) virCgroupPtr group = NULL;
>
> I personally prefer the first option, it's cleaner and the line is
> shorter and it's perfectly readable even though some logic is hidden.
> The second usage has a benefit that the logic is not hidden and you can
> use any function and you don't have to define anything but the line can
> get really long and ugly as we have some really long names for Free
> functions and types.
>
> For example:
>
> VIR_AUTOPTR(virDomainGraphicsDef) def = NULL;
>
> VIR_CLEANUP(virDomainGraphicsDefFree) virDomainGraphicsDefPtr def = NULL;
>
> And that's not even the longest free function.
>
> Since the wrapper defined by VIR_DEFINE_AUTOPTR_FUNC() will be expanded
> during compilation I prefer that option. It would essentially work the
> same way is VIR_CLEANUP but with the benefit that the declaration line
> is short, nice and clean.
I'm fine with either option, as long as we keep the ptr to a ptr aspect
of the original function. Probably a slight pref for the first option.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
