On Mon, May 28, 2018 at 09:40:41PM +0530, Sukrit Bhatnagar wrote:
> On 28 May 2018 at 13:54, Pavel Hrdina <phrdina@xxxxxxxxxx> wrote:
> > On Mon, May 28, 2018 at 01:04:28PM +0530, Sukrit Bhatnagar wrote:
> >> On 25 May 2018 at 16:20, Pavel Hrdina <phrdina@xxxxxxxxxx> wrote:
> >> > On Fri, May 25, 2018 at 12:06:50PM +0200, Andrea Bolognani wrote:
> >> >> On Fri, 2018-05-25 at 10:04 +0200, Pavel Hrdina wrote:
> >> >> > On Fri, May 25, 2018 at 09:13:51AM +0200, Andrea Bolognani wrote:
> >> >> > > However, I realize it might not be possible to register free
> >> >> > > functions for a native type without having to introduce something
> >> >> > > like
> >> >> > >
> >> >> > > typedef char * virString;
> >> >> > >
> >> >> > > thus causing massive churn. How does GLib deal with that?
> >> >> >
> >> >> > If you would look into GLib documentation you would see that this
> >> >> > design basically copies the one in GLib:
> >> >>
> >> >> Sorry, I should have looked up the documentation and implementation
> >> >> before asking silly questions. Guess the morning coffee hadn't quite
> >> >> kicked in yet :/
> >> >>
> >> >> > GLib libvirt
> >> >> >
> >> >> > g_autofree VIR_AUTOFREE
> >> >> > g_autoptr VIR_AUTOPTR
> >> >> > g_auto VIR_AUTOCLEAR
> >> >>
> >> >> For what it's worth, I think VIR_AUTOCLEAR is a much better name
> >> >> than g_auto :)
> >> >>
> >> >> > In GLib you are using them like this:
> >> >> >
> >> >> > g_autofree char *string = NULL;
> >> >> > g_autoptr(virDomain) dom = NULL;
> >> >> > g_auto(virDomain) dom = { 0 };
> >> >> >
> >> >> > So yes it would require to introduce a lot of typedefs for basic types
> >> >> > and that is not worth it.
> >> >>
> >> >> I'm not sure we would need so many typedefs, but there would
> >> >> certainly be a lot of churn involved.
> >> >>
> >> >> Personally, I'm not so sure it wouldn't be worth the effort,
> >> >> but it's definitely something that we can experiment with it at
> >> >> a later time instead of holding up what's already a pretty
> >> >> significant improvement.
> >> >>
> >> >> > In libvirt we would have:
> >> >> >
> >> >> > VIR_AUTOFREE char *string = NULL;
> >> >> > VIR_AUTOPTR(virDomainPtr) dom = NULL;
> >> >> > VIR_AUTOCLEAR(virDomain) dom = { 0 };
> >> >> >
> >> >> > If you notice the difference, in libvirt we can use virDomainPtr
> >> >> > directly because we have these typedefs, in GLib macro
> >> >> > G_DEFINE_AUTOPTR_CLEANUP_FUNC creates similar typedef.
> >> >>
> >> >> While I'm not a fan of our *Ptr typedefs in general, I guess this
> >> >> time I'm glad we have them because VIR_AUTOPTR() doesn't hide the
> >> >> fact that what you're declaring is a pointer; that is, the macro
> >> >> argument is also exactly the type of the variable.
> >> >
> >> > So let's make a summary of how it could look like:
> >> >
> >> > VIR_AUTOFREE(char *) string = NULL;
> >> > VIR_AUTOPTR(virDomainPtr) vm = NULL;
> >> > VIR_AUTOCLEAR(virDomain) dom = { 0 };
> >> >
> >> > VIR_DEFINE_AUTOFREE_FUNC(virDomainPtr, virDomainFree);
> >> > VIR_DEFINE_AUTOCLEAR_FUNC(virDomain, virDomainClear);
> >>
> >> Do we define new functions for freeing/clearing, because that is what
> >> VIR_DEFINE_AUTOFREE_FUNC seems to do.
> >>
> >>
> >> This is what new macros will look like:
> >>
> >> # define _VIR_TYPE_PTR(type) type##Ptr
> >>
> >> # define _VIR_ATTR_AUTOFREE_PTR(type) __attribute__((cleanup(type##Free)))
> >> # define _VIR_ATTR_AUTOCLOSE_PTR(type) __attribute__((cleanup(type##Close)))
> >> # define _VIR_ATTR_AUTOCLEAN_PTR(type) __attribute__((cleanup(type##Clean)))
> >>
> >> # define VIR_AUTOFREE_PTR(type) _VIR_ATTR_AUTOFREE_PTR(type) _VIR_TYPE_PTR(type)
> >
> > NACK to this, please look few lines above how the macros should be named
> > and which macros we would like to have implemented.
> >
> > There is no need to have the _VIR* helper macros and you need to
> > implement the VIR_DEFINE_* macros.
> >
> >> The problem is that our vir*Free functions take on vir*Ptr as the
> >> parameter and not
> >> vir*Ptr * (pointer to it).
> >
> > The problem is only with your current implementation, the original
> > design should not have this issue.
> >
> > The part of VIR_DEFINE_* macros is definition of new wrapper function
> > for the cleanup function which means that our existing free functions
> > are not used directly.
> >
> > GLib version of the define macro:
> >
> > #define G_DEFINE_AUTOPTR_CLEANUP_FUNC(TypeName, func) \
> > typedef TypeName *_GLIB_AUTOPTR_TYPENAME(TypeName); \
> > G_GNUC_BEGIN_IGNORE_DEPRECATIONS \
> > static inline void _GLIB_AUTOPTR_FUNC_NAME(TypeName) (TypeName **_ptr) \
> > { \
> > if (*_ptr) \
> > (func) (*_ptr); \
> > } \
> > G_GNUC_END_IGNORE_DEPRECATIONS
> >
> > Obviously we don't need the "typedef" line and the DEPRECATIONS macros.
>
>
> So, using the discussed design, I have added the following lines:
>
> # define VIR_AUTOPTR_FUNC_NAME(type) type##Free
> # define VIR_AUTOCLEAR_FUNC_NAME(type) type##Clear
I would suggest to make the generated function name more unique and not
that similar to our existing functions. For example:
virAutoPtr##type
virAutoClear##type
> # define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> static inline void VIR_AUTOPTR_FUNC_NAME(type) (type *_ptr) \
> { \
> if (*_ptr) \
> (func) (*_ptr); \
Theoretically the if is not required because our existing free functions
already check the given pointer whether it's null or not.
Also remove the extra space in (func) (*_ptr), that is GLib code-style.
In libvirt we don't put extra space after function name.
> } \
>
> # define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \
> static inline void VIR_AUTOCLEAR_FUNC_NAME(type) (type *_ptr) \
> { \
> if (*_ptr) \
> (func) (*_ptr); \
Here the if is not desired at all, usually the *_ptr would be directly
some structure so we need to pass the pointer itself:
(func)(_ptr);
> } \
>
> # define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
> # define VIR_AUTOPTR(type) \
> __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) type
> # define VIR_AUTOCLEAR(type) \
> __attribute__((cleanup(VIR_AUTOCLEAR_FUNC_NAME(type)))) type
>
>
> We need to add something like this for each type to autofree:
> VIR_DEFINE_AUTOFREE_FUNC(virArpTablePtr, virArpTableFree)
That's correct, ideally this call should be together with the free
function in the same header file.
> Then we would use it like:
> VIR_AUTOFREEE(void *) nlData = NULL;
> VIR_AUTOPTR(virArpTablePtr) table = NULL;
>
> Does this seem fine?
>
> Also, I am still getting the errors by doing the above:
> ...
> *** Error in `/home/skrtbhtngr/libvirt/tests/.libs/lt-virbuftest':
> free(): invalid pointer: 0x00007ffdd073a208 ***
> ...
> *** Error in `/home/skrtbhtngr/libvirt/tools/.libs/lt-virsh':
> free(): invalid pointer: 0x00007fc42349dcf9 ***
> ...
This is strange and seems to be unrelated as you've already confirmed
>
> I am not changing or adding a new function (beside the one defined in macro),
> just using original virArpTableFree.
>
>
> Also, since I am defining these in viralloc.h, we would need to
> include ALL header
> files where *Ptr typedefs are made.
>
> We can do this in three ways:
> - Include all the needed header files in viralloc.h and invoke all
> VIR_DEFINE_AUTOFREE_FUNC macros in viralloc.h itself.
> - We make a separate file for VIR_DEFINE_AUTOFREE_FUNC invocations
> where all the required *Ptr headers can be included. Then directly include that
> new header file in all the relevant .c files.
> - We invoke VIR_DEFINE_AUTOFREE_FUNC in respective header files,
> for example, invoke:
>
> VIR_DEFINE_AUTOFREE_FUNC(virArpTablePtr, virArpTableFree)
>
> in virarptable.h, and also include viralloc.h in it. Then, we do not need extra
> includes in .c files.
As I've already noted, this is the only solution, the other two
solutions are really bad :).
Anyway, with all of these notes it would be nice to see some patches
posted to this list so everyone can see the resulting code.
Pavel
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
