3:25 p.m.
Hi,
I am interested in implementing the GCC cleanup attribute for automatic
resource freeing as part of GSoC'18. I have shared a proposal for the same.
This mail is to discuss the code design for implementing it.
Here are some of my ideas:
This attribute requires a cleanup function that is called automatically
when the corresponding variable goes out of scope. There are some functions
whose logic can be reused:
- Functions such as virCommandFree, virConfFreeList and virCgroupFree can
be directly used as cleanup functions. They have parameter and return type
valid for a cleanup function.
- Functions such as virFileClose and virFileFclose need some additional
consideration as they return a value. I think we can set some global
variable in a separate source file (just like errno variable from errno.h).
Then the value to be returned can be accessed globally.
- Functions such as virDomainEventGraphicsDispose need an entirely new
design. They are used as callbacks in object classes and passed as an
argument in virClassNew. This would require making changes to
virObjectUnref's code too. *This is the part I am not sure how to implement
cleanup logic for.*
Also, since the __attribute__((__cleanup__(anyfunc))) looks ugly, a macro
like autoclean (ideas for macro name welcome!) can be used instead. As
Martin pointed out in my proposal, for some types, this can be done right
after typedef declarations, so that the type itself contains this attribute.
Basically, at most places where VIR_FREE is used to release memory
explicitly, the corresponding variable can use the attribute. The existing
virFree function also can be reused as it takes void pointer as an argument
and returns nothing.
One of the exceptions to this will be those variables which are struct
members. The cleanup of member has to be done when the enclosing struct
variable is cleaned.
I can create new files vircleanup.{c,h} for defining cleanup functions for
types which do not have an existing cleanup/free function. This can be done
separately for each driver supported.
For example, cleanups pertaining to lxc driver will be in
src/lxc/lxc_cleanup.c.
Your suggestions are welcome.
Thanks,
Sukrit Bhatnagar
3:43 a.m.
On Sun, Mar 25, 2018 at 01:55:07AM +0530, Sukrit Bhatnagar wrote:
Hi,
I am interested in implementing the GCC cleanup attribute for automatic
resource freeing as part of GSoC'18. I have shared a proposal for the same.
This mail is to discuss the code design for implementing it.
Here are some of my ideas:
This attribute requires a cleanup function that is called automatically
when the corresponding variable goes out of scope. There are some functions
whose logic can be reused:
- Functions such as virCommandFree, virConfFreeList and virCgroupFree can
be directly used as cleanup functions. They have parameter and return type
valid for a cleanup function.
- Functions such as virFileClose and virFileFclose need some additional
consideration as they return a value. I think we can set some global
variable in a separate source file (just like errno variable from errno.h).
Then the value to be returned can be accessed globally.
Note we call VIR_FORCE_CLOSE and VIR_FORCE_FCLOSE in cleanup scenarios
to explicitly ignore the return value. We only care about checking
return value for VIR_CLOSE and VIR_FCLOSE in a tiny set of cases.
- Functions such as virDomainEventGraphicsDispose need an entirely new
design. They are used as callbacks in object classes and passed as an
argument in virClassNew. This would require making changes to
virObjectUnref's code too. *This is the part I am not sure how to implement
cleanup logic for.*
There shouldn't be any need to touch the DIspose funtions - that's internal
helper code. To "free" an object variable, you just want to end up invoking
the regular virObjectUnref function as the cleanup func.
Also, since the __attribute__((__cleanup__(anyfunc))) looks ugly, a
macro
like autoclean (ideas for macro name welcome!) can be used instead. As
Martin pointed out in my proposal, for some types, this can be done right
after typedef declarations, so that the type itself contains this attribute.
Just suffix it with "Auto", eg virDomainAutoPtr, vs virDomainPtr.
I can create new files vircleanup.{c,h} for defining cleanup
functions for
types which do not have an existing cleanup/free function. This can be done
separately for each driver supported.
For example, cleanups pertaining to lxc driver will be in
src/lxc/lxc_cleanup.c.
I'm not rally seeing a compelling need for this - just add it in appropriate
existing files.
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 :|
11:05 a.m.
On Sun, Mar 25, 2018 at 01:55:07AM +0530, Sukrit Bhatnagar wrote:
Hi,
I am interested in implementing the GCC cleanup attribute for automatic
resource freeing as part of GSoC'18. I have shared a proposal for the same.
This mail is to discuss the code design for implementing it.
Here are some of my ideas:
This attribute requires a cleanup function that is called automatically
when the corresponding variable goes out of scope. There are some functions
whose logic can be reused:
- Functions such as virCommandFree, virConfFreeList and virCgroupFree can
be directly used as cleanup functions. They have parameter and return type
valid for a cleanup function.
- Functions such as virFileClose and virFileFclose need some additional
consideration as they return a value. I think we can set some global
variable in a separate source file (just like errno variable from errno.h).
Then the value to be returned can be accessed globally.
- Functions such as virDomainEventGraphicsDispose need an entirely new
design. They are used as callbacks in object classes and passed as an
argument in virClassNew. This would require making changes to
virObjectUnref's code too. *This is the part I am not sure how to implement
cleanup logic for.*
Also, since the __attribute__((__cleanup__(anyfunc))) looks ugly, a macro
like autoclean (ideas for macro name welcome!) can be used instead. As
Martin pointed out in my proposal, for some types, this can be done right
after typedef declarations, so that the type itself contains this attribute.
Basically, at most places where VIR_FREE is used to release memory
explicitly, the corresponding variable can use the attribute. The existing
virFree function also can be reused as it takes void pointer as an argument
and returns nothing.
One of the exceptions to this will be those variables which are struct
members. The cleanup of member has to be done when the enclosing struct
variable is cleaned.
I can create new files vircleanup.{c,h} for defining cleanup functions for
types which do not have an existing cleanup/free function. This can be done
separately for each driver supported.
For example, cleanups pertaining to lxc driver will be in
src/lxc/lxc_cleanup.c.
Hi,
I would like to apologize that it took me that long to reply.
We've already discussed this a little bit off-list so I would like to
summarize my idea about the design of this effort.
I liked the way how GLib is solving the issue so we can simply use the
same approach since it looks reasonable.
There would be three different macros that would be used to annotate
variable with attribute cleanup:
VIR_AUTOFREE char *str = NULL;
- this would call virFree on that variable
VIR_AUTOPTR(virDomain) domain = NULL;
- this would call registered free function on that variable
- to register the free function you would use:
VIR_DEFINE_AUTOPTR_FUNC(virDomain, virDomainFree);
VIR_AUTOCLEAR(virDomain) domain = { 0 };
- this would call registered clear function to free the content of
that structure
- to register that clear function you would use:
VIR_DEFINE_AUTOCLEAR_FUNC(virDomain, virDomainClear);
In GLib there are three "define" macros and all of them creates a
wrapper function to make sure that the Free/Clear function is called
only if necessary. This is a safe-guard because it's a public API.
We don't have to have this safe-guard in place since almost all of our
Free functions already check whether the pointer is null or not.
For reference how it works in GLib you can look here [1].
Pavel
[1] <https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html>
11:23 a.m.
On Wed, May 23, 2018 at 18:05:17 +0200, Pavel Hrdina wrote:
[...]
I liked the way how GLib is solving the issue so we can simply use
the
same approach since it looks reasonable.
There would be three different macros that would be used to annotate
variable with attribute cleanup:
VIR_AUTOFREE char *str = NULL;
For consistency I'd prefer if the argument is in parentheses similarly
to the ones below.
- this would call virFree on that variable
VIR_AUTOPTR(virDomain) domain = NULL;
- this would call registered free function on that variable
- to register the free function you would use:
VIR_DEFINE_AUTOPTR_FUNC(virDomain, virDomainFree);
Did you mean virDomainPtr?
How is the type matched? Does it have to be a typedef'd type to use
this?
2:26 p.m.
On Wed, May 23, 2018 at 06:23:01PM +0200, Peter Krempa wrote:
On Wed, May 23, 2018 at 18:05:17 +0200, Pavel Hrdina wrote:
[...]
> I liked the way how GLib is solving the issue so we can simply use the
> same approach since it looks reasonable.
>
> There would be three different macros that would be used to annotate
> variable with attribute cleanup:
>
> VIR_AUTOFREE char *str = NULL;
For consistency I'd prefer if the argument is in parentheses similarly
to the ones below.
Yes, for consistency it can take the type as an argument but it's not
necessary since the type is not used by that macro.
> - this would call virFree on that variable
>
> VIR_AUTOPTR(virDomain) domain = NULL;
>
> - this would call registered free function on that variable
> - to register the free function you would use:
>
> VIR_DEFINE_AUTOPTR_FUNC(virDomain, virDomainFree);
Did you mean virDomainPtr?
Right, it can be virDomainPtr since we already have these typedefs,
GLib implementation actually creates the same typedef inside the
G_DEFINE_AUTOPTR_CLEANUP_FUNC macro, we don't have to do it.
It also creates a wrapper function with a name based on the passed type
and that wrapper function is then used by g_autoptr() again based on the
passed type.
Pavel
2:01 a.m.
On Wed, 2018-05-23 at 18:23 +0200, Peter Krempa wrote:
On Wed, May 23, 2018 at 18:05:17 +0200, Pavel Hrdina wrote:
[...]
> VIR_AUTOFREE char *str = NULL;
For consistency I'd prefer if the argument is in parentheses similarly
to the ones below.
Seconded.
--
Andrea Bolognani / Red Hat / Virtualization
3:08 a.m.
On Fri, May 25, 2018 at 09:01:03AM +0200, Andrea Bolognani wrote:
On Wed, 2018-05-23 at 18:23 +0200, Peter Krempa wrote:
> On Wed, May 23, 2018 at 18:05:17 +0200, Pavel Hrdina wrote:
[...]
> > VIR_AUTOFREE char *str = NULL;
>
> For consistency I'd prefer if the argument is in parentheses similarly
> to the ones below.
Seconded.
Well that would mean having this macro:
#define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type *
and the usage would be:
VIR_AUTOFREE(char) string = NULL;
Yes, for consistency it make sense but sometimes exception makes it look
better and IMHO this is the case so I would prefer
#define VIR_AUTOFREE(type) __attribute__((cleanup(virFree)))
and
VIR_AUTOFREE char *string = NULL;
The only sensible usage would be to have only VIR_AUTOPTR as you've
mentioned in other mail, but that would require a lot of typedefs.
Pavel
3:32 a.m.
On Fri, 2018-05-25 at 10:08 +0200, Pavel Hrdina wrote:
On Fri, May 25, 2018 at 09:01:03AM +0200, Andrea Bolognani wrote:
> On Wed, 2018-05-23 at 18:23 +0200, Peter Krempa wrote:
> > On Wed, May 23, 2018 at 18:05:17 +0200, Pavel Hrdina wrote:
>
> [...]
> > > VIR_AUTOFREE char *str = NULL;
> >
> > For consistency I'd prefer if the argument is in parentheses similarly
> > to the ones below.
>
> Seconded.
Well that would mean having this macro:
#define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type *
and the usage would be:
VIR_AUTOFREE(char) string = NULL;
Yes, for consistency it make sense but sometimes exception makes it look
better and IMHO this is the case so I would prefer
#define VIR_AUTOFREE(type) __attribute__((cleanup(virFree)))
and
VIR_AUTOFREE char *string = NULL;
I'm probably missing something, but couldn't you just have
#define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
which you would then use as
VIR_AUTOFREE(char *) string = NULL;
instead?
--
Andrea Bolognani / Red Hat / Virtualization
3:46 a.m.
On Fri, May 25, 2018 at 10:32:04AM +0200, Andrea Bolognani wrote:
On Fri, 2018-05-25 at 10:08 +0200, Pavel Hrdina wrote:
> On Fri, May 25, 2018 at 09:01:03AM +0200, Andrea Bolognani wrote:
> > On Wed, 2018-05-23 at 18:23 +0200, Peter Krempa wrote:
> > > On Wed, May 23, 2018 at 18:05:17 +0200, Pavel Hrdina wrote:
> >
> > [...]
> > > > VIR_AUTOFREE char *str = NULL;
> > >
> > > For consistency I'd prefer if the argument is in parentheses
similarly
> > > to the ones below.
> >
> > Seconded.
>
> Well that would mean having this macro:
>
> #define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type *
>
> and the usage would be:
>
> VIR_AUTOFREE(char) string = NULL;
>
> Yes, for consistency it make sense but sometimes exception makes it look
> better and IMHO this is the case so I would prefer
>
> #define VIR_AUTOFREE(type) __attribute__((cleanup(virFree)))
>
> and
>
> VIR_AUTOFREE char *string = NULL;
I'm probably missing something, but couldn't you just have
#define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
which you would then use as
VIR_AUTOFREE(char *) string = NULL;
instead?
Yes you can have that as well, but it doesn't look ugly to you? :)
If majority will agree on that I don't care, just my opinion.
Pavel
4:03 a.m.
On Fri, 2018-05-25 at 10:46 +0200, Pavel Hrdina wrote:
On Fri, May 25, 2018 at 10:32:04AM +0200, Andrea Bolognani wrote:
> I'm probably missing something, but couldn't you just have
>
> #define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
>
> which you would then use as
>
> VIR_AUTOFREE(char *) string = NULL;
>
> instead?
Yes you can have that as well, but it doesn't look ugly to you? :)
Quite the opposite - not only it's consistent with the other
macros, but it also cleanly separates the type from the variable
name, which I consider a plus.
Personally, even though
char *string;
and friends are the accepted way to declare pointer variables,
I've always been slightly annoyed by the fact that type name and
variable name end up being partially mixed together.
Don't get me wrong, something like
char* string;
would still look wrong to me, because I'm just so used to the
other way! But in the context of that macro I think we really get
the best of both worlds :)
If majority will agree on that I don't care, just my opinion.
Yeah, same here.
--
Andrea Bolognani / Red Hat / Virtualization
4:17 a.m.
On Fri, May 25, 2018 at 11:03:01AM +0200, Andrea Bolognani wrote:
On Fri, 2018-05-25 at 10:46 +0200, Pavel Hrdina wrote:
> On Fri, May 25, 2018 at 10:32:04AM +0200, Andrea Bolognani wrote:
> > I'm probably missing something, but couldn't you just have
> >
> > #define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
> >
> > which you would then use as
> >
> > VIR_AUTOFREE(char *) string = NULL;
> >
> > instead?
>
> Yes you can have that as well, but it doesn't look ugly to you? :)
Quite the opposite - not only it's consistent with the other
macros, but it also cleanly separates the type from the variable
name, which I consider a plus.
I also like the alternative approach Andrea mentioned, especially since you
can reuse it for structures using plain scalar types, e.g. a structure like
typedef _virSomething virSomething;
typedef virSomething * virSomethingPtr;
struct _virSomething {
int a;
long b;
bool c;
};
..where you don't really need to use VIR_AUTOPTR's complex clean functions
since a simple virFree works just fine and it lets you to stay consistent with
the usage of XPtr types which has also been mentioned already, just my 2c.
Erik
5:16 a.m.
On Fri, 2018-05-25 at 11:17 +0200, Erik Skultety wrote:
I also like the alternative approach Andrea mentioned, especially
since you
can reuse it for structures using plain scalar types, e.g. a structure like
typedef _virSomething virSomething;
typedef virSomething * virSomethingPtr;
struct _virSomething {
int a;
long b;
bool c;
};
..where you don't really need to use VIR_AUTOPTR's complex clean functions
since a simple virFree works just fine and it lets you to stay consistent with
the usage of XPtr types which has also been mentioned already, just my 2c.
I have to disagree here: I think every virSomething should have
a corresponding virSomethingFree() function, even when it ends up
doing nothing but calling VIR_FREE() or virObjectUnref().
Even when automatic cleaning will be in place, there will still
be cases in which you have to allocate an object and return it
for the caller to free, and not having to look up what specific
free function needs to be used because you can rely on the
existence of a consistently named one is a big plus in my book.
We're pretty bad at this already, let's not make it worse :)
--
Andrea Bolognani / Red Hat / Virtualization
12:46 p.m.
On 23 May 2018 at 21:35, Pavel Hrdina <phrdina(a)redhat.com> wrote:
On Sun, Mar 25, 2018 at 01:55:07AM +0530, Sukrit Bhatnagar wrote:
> Hi,
>
> I am interested in implementing the GCC cleanup attribute for automatic
> resource freeing as part of GSoC'18. I have shared a proposal for the same.
>
> This mail is to discuss the code design for implementing it.
>
>
> Here are some of my ideas:
>
> This attribute requires a cleanup function that is called automatically
> when the corresponding variable goes out of scope. There are some functions
> whose logic can be reused:
>
> - Functions such as virCommandFree, virConfFreeList and virCgroupFree can
> be directly used as cleanup functions. They have parameter and return type
> valid for a cleanup function.
>
> - Functions such as virFileClose and virFileFclose need some additional
> consideration as they return a value. I think we can set some global
> variable in a separate source file (just like errno variable from errno.h).
> Then the value to be returned can be accessed globally.
>
> - Functions such as virDomainEventGraphicsDispose need an entirely new
> design. They are used as callbacks in object classes and passed as an
> argument in virClassNew. This would require making changes to
> virObjectUnref's code too. *This is the part I am not sure how to implement
> cleanup logic for.*
>
>
> Also, since the __attribute__((__cleanup__(anyfunc))) looks ugly, a macro
> like autoclean (ideas for macro name welcome!) can be used instead. As
> Martin pointed out in my proposal, for some types, this can be done right
> after typedef declarations, so that the type itself contains this attribute.
>
> Basically, at most places where VIR_FREE is used to release memory
> explicitly, the corresponding variable can use the attribute. The existing
> virFree function also can be reused as it takes void pointer as an argument
> and returns nothing.
> One of the exceptions to this will be those variables which are struct
> members. The cleanup of member has to be done when the enclosing struct
> variable is cleaned.
>
> I can create new files vircleanup.{c,h} for defining cleanup functions for
> types which do not have an existing cleanup/free function. This can be done
> separately for each driver supported.
> For example, cleanups pertaining to lxc driver will be in
> src/lxc/lxc_cleanup.c.
Hi,
I would like to apologize that it took me that long to reply.
We've already discussed this a little bit off-list so I would like to
summarize my idea about the design of this effort.
I liked the way how GLib is solving the issue so we can simply use the
same approach since it looks reasonable.
There would be three different macros that would be used to annotate
variable with attribute cleanup:
VIR_AUTOFREE char *str = NULL;
- this would call virFree on that variable
This seems fine for basic native datatypes.
But for the complex types, why can't we do something like the following, as was
discussed previously?
* Define simple macros for the attribute per struct type:
#define VIRCOMMAND_AUTOFREE __attribute__((__cleanup__(freefunction))
#define VIRCOMMAND_AUTOCLEAN __attribute__((__cleanup__(cleanfunction))
* Create new datatypes which include this attribute:
#define virCommandAutoFreePtr VIRCOMMAND_AUTOFREE virCommandPtr
#define virCommandAutoCleanPtr VIRCOMMAND_AUTOCLEAN virCommandPtr
* Then simply declare variables as:
virCommandAutoFreePtr cmd = NULL;
We just have to make sure that all Ptr variables are initialized,
atleast to NULL.
Also, we can create and use macros to initialize the variables:
#define CREATE_CMD_PTR(var, val) \
VIRCOMMAND_AUTOFREE virCommandPtr var = (val);
CREATE_CMD_PTR(cmd, virCommandNewArgs(args))
This is equivalent to:
VIRCOMMAND_AUTOFREE virCommandPtr cmd = virCommandNewArgs(args);
Many existing Free functions such as virDomainFree, return a value
which is not allowed for a function that is to be used with cleanup attribute.
Do we ignore all such return values?
VIR_AUTOPTR(virDomain) domain = NULL;
- this would call registered free function on that variable
- to register the free function you would use:
VIR_DEFINE_AUTOPTR_FUNC(virDomain, virDomainFree);
VIR_AUTOCLEAR(virDomain) domain = { 0 };
- this would call registered clear function to free the content of
that structure
- to register that clear function you would use:
VIR_DEFINE_AUTOCLEAR_FUNC(virDomain, virDomainClear);
In GLib there are three "define" macros and all of them creates a
wrapper function to make sure that the Free/Clear function is called
only if necessary. This is a safe-guard because it's a public API.
We don't have to have this safe-guard in place since almost all of our
Free functions already check whether the pointer is null or not.
For reference how it works in GLib you can look here [1].
Pavel
[1] <https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html>
2:23 p.m.
On Thu, May 24, 2018 at 11:16:40PM +0530, Sukrit Bhatnagar wrote:
On 23 May 2018 at 21:35, Pavel Hrdina <phrdina(a)redhat.com>
wrote:
> On Sun, Mar 25, 2018 at 01:55:07AM +0530, Sukrit Bhatnagar wrote:
>> Hi,
>>
>> I am interested in implementing the GCC cleanup attribute for automatic
>> resource freeing as part of GSoC'18. I have shared a proposal for the same.
>>
>> This mail is to discuss the code design for implementing it.
>>
>>
>> Here are some of my ideas:
>>
>> This attribute requires a cleanup function that is called automatically
>> when the corresponding variable goes out of scope. There are some functions
>> whose logic can be reused:
>>
>> - Functions such as virCommandFree, virConfFreeList and virCgroupFree can
>> be directly used as cleanup functions. They have parameter and return type
>> valid for a cleanup function.
>>
>> - Functions such as virFileClose and virFileFclose need some additional
>> consideration as they return a value. I think we can set some global
>> variable in a separate source file (just like errno variable from errno.h).
>> Then the value to be returned can be accessed globally.
>>
>> - Functions such as virDomainEventGraphicsDispose need an entirely new
>> design. They are used as callbacks in object classes and passed as an
>> argument in virClassNew. This would require making changes to
>> virObjectUnref's code too. *This is the part I am not sure how to implement
>> cleanup logic for.*
>>
>>
>> Also, since the __attribute__((__cleanup__(anyfunc))) looks ugly, a macro
>> like autoclean (ideas for macro name welcome!) can be used instead. As
>> Martin pointed out in my proposal, for some types, this can be done right
>> after typedef declarations, so that the type itself contains this attribute.
>>
>> Basically, at most places where VIR_FREE is used to release memory
>> explicitly, the corresponding variable can use the attribute. The existing
>> virFree function also can be reused as it takes void pointer as an argument
>> and returns nothing.
>> One of the exceptions to this will be those variables which are struct
>> members. The cleanup of member has to be done when the enclosing struct
>> variable is cleaned.
>>
>> I can create new files vircleanup.{c,h} for defining cleanup functions for
>> types which do not have an existing cleanup/free function. This can be done
>> separately for each driver supported.
>> For example, cleanups pertaining to lxc driver will be in
>> src/lxc/lxc_cleanup.c.
>
> Hi,
>
> I would like to apologize that it took me that long to reply.
>
> We've already discussed this a little bit off-list so I would like to
> summarize my idea about the design of this effort.
>
> I liked the way how GLib is solving the issue so we can simply use the
> same approach since it looks reasonable.
>
> There would be three different macros that would be used to annotate
> variable with attribute cleanup:
>
> VIR_AUTOFREE char *str = NULL;
>
> - this would call virFree on that variable
This seems fine for basic native datatypes.
But for the complex types, why can't we do something like the following, as was
discussed previously?
* Define simple macros for the attribute per struct type:
#define VIRCOMMAND_AUTOFREE __attribute__((__cleanup__(freefunction))
#define VIRCOMMAND_AUTOCLEAN __attribute__((__cleanup__(cleanfunction))
* Create new datatypes which include this attribute:
#define virCommandAutoFreePtr VIRCOMMAND_AUTOFREE virCommandPtr
#define virCommandAutoCleanPtr VIRCOMMAND_AUTOCLEAN virCommandPtr
* Then simply declare variables as:
virCommandAutoFreePtr cmd = NULL;
We just have to make sure that all Ptr variables are initialized,
atleast to NULL.
Also, we can create and use macros to initialize the variables:
#define CREATE_CMD_PTR(var, val) \
VIRCOMMAND_AUTOFREE virCommandPtr var = (val);
CREATE_CMD_PTR(cmd, virCommandNewArgs(args))
This is equivalent to:
VIRCOMMAND_AUTOFREE virCommandPtr cmd = virCommandNewArgs(args);
Yes, this solution would work but it looks way too complicated.
It would require to create new macro for every structure that we have
in libvirt which seems redundant if we can have few universal macros
that can be used for everything.
Each macro should be simple and should do one thing and should not hide
too much logic.
The design that I was proposing would be used like this for the
virCommandPtr:
src/util/viralloc.h:
#define VIR_AUTOFREE __attribute__((cleanup(virFree)))
#define VIR_AUTOPTR(type) ...
#define VIR_AUTOCLEAR(type) ...
#define VIR_DEFINE_AUTOPTR_FUNC(type, freeFunc) ...
#define VIR_DEFINE_AUTOCLEAR_FUNC(type, clearFunc) ...
src/util/vircommand.h:
VIR_DEFINE_AUTOPTR_FUNC(virCommandPtr, virCommandFree);
src/qemu/qemu_driver.c: (the virCommandPtr is used there)
...
VIR_AUTOPTR(virCommandPtr) cmd = NULL;
...
Now the fist design would require to introduce a lot of new macros to
achieve the same result, let's summarize it for comparison:
src/util/vircommand.h:
#define VIRCOMMAND_AUTOFREE __attribute__((__cleanup__(virCommandFree))
#define virCommandAutoFreePtr VIRCOMMAND_AUTOFREE virCommandPtr
//optional
#define CREATE_CMD_PTR(var, val) \
VIRCOMMAND_AUTOFREE virCommandPtr var = (val);
src/qemu/qemu_driver.c:
...
virCommandAutoFreePtr cmd = NULL;
...
or
...
CREATE_CMD_PTR(cmd, virCommandNewArgs(args))
...
As you can see, the original solution doesn't have any generic macro
but it requires at least 2 new macros for every single structure
whereas the new design requires only 1 call of existing macro for every
structure. It's more flexible and all the logic is only in once place.
Many existing Free functions such as virDomainFree, return a value
which is not allowed for a function that is to be used with cleanup attribute.
Do we ignore all such return values?
virDomainFree is a public API and we don't have to use them in our
internal code so this should not be an issue. There are some internal
free functions that also return some value, for example virObjectUnref,
but that is not an issue as well.
Look at the documentation of GLib functions and also at the code to get
the idea of how it works.
Pavel
2:13 a.m.
On Wed, 2018-05-23 at 18:05 +0200, Pavel Hrdina wrote:
I liked the way how GLib is solving the issue so we can simply use
the
same approach since it looks reasonable.
There would be three different macros that would be used to annotate
variable with attribute cleanup:
VIR_AUTOFREE char *str = NULL;
- this would call virFree on that variable
VIR_AUTOPTR(virDomain) domain = NULL;
- this would call registered free function on that variable
- to register the free function you would use:
VIR_DEFINE_AUTOPTR_FUNC(virDomain, virDomainFree);
VIR_AUTOCLEAR(virDomain) domain = { 0 };
- this would call registered clear function to free the content of
that structure
- to register that clear function you would use:
VIR_DEFINE_AUTOCLEAR_FUNC(virDomain, virDomainClear);
I assume you would get a compilation error when trying to eg. use
VIR_AUTOCLEAR() with a type that doesn't have a clear function
registered?
As for VIR_AUTOFREE() and VIR_AUTOPTR(), I'd very much prefer if we
could have a single macro, since from the high-level point of view
they're both doing the same thing, that is, freeing memory that was
allocated on the heap.
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?
--
Andrea Bolognani / Red Hat / Virtualization
3:04 a.m.
On Fri, May 25, 2018 at 09:13:51AM +0200, Andrea Bolognani wrote:
On Wed, 2018-05-23 at 18:05 +0200, Pavel Hrdina wrote:
> I liked the way how GLib is solving the issue so we can simply use the
> same approach since it looks reasonable.
>
> There would be three different macros that would be used to annotate
> variable with attribute cleanup:
>
> VIR_AUTOFREE char *str = NULL;
>
> - this would call virFree on that variable
>
> VIR_AUTOPTR(virDomain) domain = NULL;
>
> - this would call registered free function on that variable
> - to register the free function you would use:
>
> VIR_DEFINE_AUTOPTR_FUNC(virDomain, virDomainFree);
>
> VIR_AUTOCLEAR(virDomain) domain = { 0 };
>
> - this would call registered clear function to free the content of
> that structure
> - to register that clear function you would use:
>
> VIR_DEFINE_AUTOCLEAR_FUNC(virDomain, virDomainClear);
I assume you would get a compilation error when trying to eg. use
VIR_AUTOCLEAR() with a type that doesn't have a clear function
registered?
Yes, because the macro would call non existing function.
As for VIR_AUTOFREE() and VIR_AUTOPTR(), I'd very much prefer if
we
could have a single macro, since from the high-level point of view
they're both doing the same thing, that is, freeing memory that was
allocated on the heap.
It would be nice but I don't think it's worth it.
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:
GLib libvirt
g_autofree VIR_AUTOFREE
g_autoptr VIR_AUTOPTR
g_auto VIR_AUTOCLEAR
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.
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.
Pavel
5:06 a.m.
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.
--
Andrea Bolognani / Red Hat / Virtualization
5:50 a.m.
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);
Note: don't take the types and function names as something that actually
exists and be used like that, it's just an example to show how it would
work :).
Pavel
2:34 a.m.
On 25 May 2018 at 16:20, Pavel Hrdina <phrdina(a)redhat.com> 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)
The problem is that our vir*Free functions take on vir*Ptr as the
parameter and not
vir*Ptr * (pointer to it).
For example, instead of:
void virArpTableFree(virArpTablePtr table);
we would need:
void virArpTableFree(virArpTablePtr *table);
if we declare something like:
VIR_AUTOFREE_PTR(virArpTable) table = NULL;
Also, I tried to add a new function:
void virArpTablePtrFree(virArpTablePtr *table)
{
size_t i;
if (!*table)
return;
for (i = 0; i < (*table)->n; i++) {
VIR_FREE((*table)->t[i].ipaddr);
VIR_FREE((*table)->t[i].mac);
}
VIR_FREE((*table)->t);
VIR_FREE((*table));
VIR_FREE(table);
}
but I am getting the errors:
*** Error in `/home/skrtbhtngr/libvirt/tests/.libs/lt-virbuftest':
free(): invalid pointer: 0x00007ffc7be60d48 ***
...
*** Error in `/home/skrtbhtngr/libvirt/tests/.libs/lt-commandtest':
free(): invalid pointer: 0x00007fff727583fc ***
...
I am not quite sure how to debug this. Am I missing something basic?
Note: don't take the types and function names as something that actually
exists and be used like that, it's just an example to show how it would
work :).
Pavel
3:02 a.m.
On Mon, May 28, 2018 at 01:04:28PM +0530, Sukrit Bhatnagar wrote:
On 25 May 2018 at 16:20, Pavel Hrdina <phrdina(a)redhat.com>
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)
The problem is that our vir*Free functions take on vir*Ptr as the
parameter and not
vir*Ptr * (pointer to it).
For example, instead of:
void virArpTableFree(virArpTablePtr table);
we would need:
void virArpTableFree(virArpTablePtr *table);
Hmm, so gcc claims the cleanup attribute to take a function which is required
to take a pointer to the type which it already is, it's just typedef'd, I
haven't tried this myself but do you get a compiler error if you try it with
the existing function, i.e. the one which doesn't mention the explicit '*' to
signal a pointer type? If so and we can't use the our typedef'd Ptr type
directly, then we'll need to switch all the free callback signatures to a plain
virSomeType *arg rather than trying to pass a double pointer which is just
unnecessary and ugly.
if we declare something like:
VIR_AUTOFREE_PTR(virArpTable) table = NULL;
Also, I tried to add a new function:
void virArpTablePtrFree(virArpTablePtr *table)
{
size_t i;
if (!*table)
return;
for (i = 0; i < (*table)->n; i++) {
VIR_FREE((*table)->t[i].ipaddr);
VIR_FREE((*table)->t[i].mac);
}
VIR_FREE((*table)->t);
VIR_FREE((*table));
VIR_FREE(table);
My honest guess would be ^this you're trying to free the double pointer itself
which you didn't and you're also not supposed to allocate.
Erik
3:24 a.m.
On Mon, May 28, 2018 at 01:04:28PM +0530, Sukrit Bhatnagar wrote:
On 25 May 2018 at 16:20, Pavel Hrdina <phrdina(a)redhat.com>
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.
Pavel
11:10 a.m.
On 28 May 2018 at 13:54, Pavel Hrdina <phrdina(a)redhat.com> wrote:
On Mon, May 28, 2018 at 01:04:28PM +0530, Sukrit Bhatnagar wrote:
> On 25 May 2018 at 16:20, Pavel Hrdina <phrdina(a)redhat.com> 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
# define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
static inline void VIR_AUTOPTR_FUNC_NAME(type) (type *_ptr) \
{ \
if (*_ptr) \
(func) (*_ptr); \
} \
# define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \
static inline void VIR_AUTOCLEAR_FUNC_NAME(type) (type *_ptr) \
{ \
if (*_ptr) \
(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)
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 ***
...
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.
7:58 a.m.
On Mon, May 28, 2018 at 09:40:41PM +0530, Sukrit Bhatnagar wrote:
On 28 May 2018 at 13:54, Pavel Hrdina <phrdina(a)redhat.com>
wrote:
> On Mon, May 28, 2018 at 01:04:28PM +0530, Sukrit Bhatnagar wrote:
>> On 25 May 2018 at 16:20, Pavel Hrdina <phrdina(a)redhat.com> 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
12:06 a.m.
On 05/28/2018 09:34 AM, Sukrit Bhatnagar wrote:
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)
The problem is that our vir*Free functions take on vir*Ptr as the
parameter and not
vir*Ptr * (pointer to it).
For example, instead of:
void virArpTableFree(virArpTablePtr table);
we would need:
void virArpTableFree(virArpTablePtr *table);
if we declare something like:
VIR_AUTOFREE_PTR(virArpTable) table = NULL;
Also, I tried to add a new function:
void virArpTablePtrFree(virArpTablePtr *table)
{
size_t i;
if (!*table)
return;
for (i = 0; i < (*table)->n; i++) {
VIR_FREE((*table)->t[i].ipaddr);
VIR_FREE((*table)->t[i].mac);
}
VIR_FREE((*table)->t);
VIR_FREE((*table));
VIR_FREE(table);
As Erik pointed out, this last VIR_FREE(table) looks fishy. However, do
you have patch that I can apply and reproduce?
Michal
1:47 a.m.
On 29 May 2018 at 10:36, Michal Privoznik <mprivozn(a)redhat.com> wrote:
On 05/28/2018 09:34 AM, Sukrit Bhatnagar wrote:
>
> 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)
>
>
> The problem is that our vir*Free functions take on vir*Ptr as the
> parameter and not
> vir*Ptr * (pointer to it).
>
> For example, instead of:
> void virArpTableFree(virArpTablePtr table);
>
> we would need:
> void virArpTableFree(virArpTablePtr *table);
>
> if we declare something like:
> VIR_AUTOFREE_PTR(virArpTable) table = NULL;
>
>
> Also, I tried to add a new function:
> void virArpTablePtrFree(virArpTablePtr *table)
> {
> size_t i;
>
> if (!*table)
> return;
>
> for (i = 0; i < (*table)->n; i++) {
> VIR_FREE((*table)->t[i].ipaddr);
> VIR_FREE((*table)->t[i].mac);
> }
> VIR_FREE((*table)->t);
> VIR_FREE((*table));
> VIR_FREE(table);
As Erik pointed out, this last VIR_FREE(table) looks fishy. However, do
you have patch that I can apply and reproduce?
When I was using the above function, even without the last VIR_FREE(table),
I got the same error.
I am now using the original virArpTableFree function, not this.
The error is resolved for now. It seems like some other files I modified were
creating trouble. I reset them.
5:55 a.m.
On Mon, May 28, 2018 at 01:04:28PM +0530, Sukrit Bhatnagar wrote:
On 25 May 2018 at 16:20, Pavel Hrdina <phrdina(a)redhat.com>
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)
The problem is that our vir*Free functions take on vir*Ptr as the
parameter and not
vir*Ptr * (pointer to it).
For example, instead of:
void virArpTableFree(virArpTablePtr table);
we would need:
void virArpTableFree(virArpTablePtr *table);
This is actually a *good* thing. Passing in 'Ptr *' allows the
virXXXFree function to set the pointer to NULL after free'ing
it which prevents a double-free.
If we need to change these free functions, that's a worthwhile
improvement in general IMHO.
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 :|
2179
days inactive
2248
days old
25 comments
7 participants
participants (7)
-
Andrea Bolognani -
Daniel P. Berrangé -
Erik Skultety -
Michal Privoznik -
Pavel Hrdina -
Peter Krempa -
Sukrit Bhatnagar