4 p.m.
This series of patches aim at augmenting our discussion about the
design for implementing the cleanup attribute.
A set of macros have been added at the end of viralloc.h
A few files have been modified to use the newly created macros.
Sukrit Bhatnagar (4):
add macros for implementing automatic cleanup functionality
add automatic cleanup support in src/util/virarptable.c
add automatic cleanup support in src/util/virauthconfig.c
add automatic cleanup support in src/util/virauth.c
src/util/viralloc.h | 69 ++++++++++++++++++++++++++++++++++++++++++++++++
src/util/virarptable.c | 9 ++-----
src/util/virauth.c | 66 +++++++++++++++++----------------------------
src/util/virauthconfig.c | 34 +++++++++---------------
src/util/virauthconfig.h | 3 +++
5 files changed, 110 insertions(+), 71 deletions(-)
--
1.8.3.1
4 p.m.
New subject: [libvirt] [RFC 1/4] add macros for implementing automatic cleanup functionality
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>
---
src/util/viralloc.h | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 69 insertions(+)
diff --git a/src/util/viralloc.h b/src/util/viralloc.h
index 69d0f90..e1c31c3 100644
--- a/src/util/viralloc.h
+++ b/src/util/viralloc.h
@@ -596,4 +596,73 @@ void virAllocTestInit(void);
int virAllocTestCount(void);
void virAllocTestOOM(int n, int m);
void virAllocTestHook(void (*func)(int, void*), void *data);
+
+# define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
+# define VIR_AUTOCLEAR_FUNC_NAME(type) virAutoClear##type
+
+/**
+ * VIR_DEFINE_AUTOPTR_FUNC:
+ * @type: type of the variables(s) to free automatically
+ * @func: cleanup function to be automatically called
+ *
+ * Thos macro defines a function for automatically freeing
+ * resources allocated to a variable of type @type. The newly
+ * defined function calls the corresponding pre-defined
+ * function @func.
+ */
+# define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
+ static inline void VIR_AUTOPTR_FUNC_NAME(type)(type *_ptr) \
+ { \
+ (func)(*_ptr); \
+ } \
+
+/**
+ * VIR_DEFINE_AUTOCLEAR_FUNC:
+ * @type: type of the variables(s) to free automatically
+ * @func: cleanup function to be automatically called
+ *
+ * This macro defines a function for automatically clearing
+ * a variable of type @type. The newly deifined function calls
+ * the corresponding pre-defined function @func.
+ */
+# define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \
+ static inline void VIR_AUTOCLEAR_FUNC_NAME(type)(type *_ptr) \
+ { \
+ (func)(_ptr); \
+ } \
+
+/**
+ * VIR_AUTOFREE:
+ * @type: type of the variables(s) to free automatically
+ *
+ * Macro to automatically free the memory allocated to
+ * the variable(s) declared with it by calling virFree
+ * when the variable goes out of scope.
+ */
+# define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
+
+/**
+ * VIR_AUTOPTR:
+ * @type: type of the variables(s) to free automatically
+ *
+ * Macro to automatically free the memory allocated to
+ * the variable(s) declared with it by calling the function
+ * defined by VIR_DEFINE_AUTOPTR_FUNC when the variable
+ * goes out of scope.
+ */
+# define VIR_AUTOPTR(type) \
+ __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) type
+
+/**
+ * VIR_AUTOCLEAR:
+ * @type: type of the variables(s) to free automatically
+ *
+ * Macro to automatically clear the variable(s) declared
+ * with it by calling the function defined by
+ * VIR_DEFINE_AUTOCLEAR_FUNC when the variabl* goes out
+ * of scope.
+ */
+# define VIR_AUTOCLEAR(type) \
+ __attribute__((cleanup(VIR_AUTOCLEAR_FUNC_NAME(type)))) type
+
#endif /* __VIR_MEMORY_H_ */
--
1.8.3.1
7:50 a.m.
New subject: [libvirt] [RFC 1/4] add macros for implementing automatic cleanup functionality
On Wed, May 30, 2018 at 02:30:04AM +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>
---
src/util/viralloc.h | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 69 insertions(+)
diff --git a/src/util/viralloc.h b/src/util/viralloc.h
index 69d0f90..e1c31c3 100644
--- a/src/util/viralloc.h
+++ b/src/util/viralloc.h
@@ -596,4 +596,73 @@ void virAllocTestInit(void);
int virAllocTestCount(void);
void virAllocTestOOM(int n, int m);
void virAllocTestHook(void (*func)(int, void*), void *data);
+
+# define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
+# define VIR_AUTOCLEAR_FUNC_NAME(type) virAutoClear##type
+
+/**
+ * VIR_DEFINE_AUTOPTR_FUNC:
+ * @type: type of the variables(s) to free automatically
+ * @func: cleanup function to be automatically called
+ *
+ * Thos macro defines a function for automatically freeing
s/Thos/This
s/automatically freeing resources/automatic freeing of resources
+ * resources allocated to a variable of type @type. The newly
+ * defined function calls the corresponding pre-defined
+ * function @func.
+ */
+# define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
+ static inline void VIR_AUTOPTR_FUNC_NAME(type)(type *_ptr) \
+ { \
+ (func)(*_ptr); \
+ } \
+
+/**
+ * VIR_DEFINE_AUTOCLEAR_FUNC:
+ * @type: type of the variables(s) to free automatically
+ * @func: cleanup function to be automatically called
+ *
+ * This macro defines a function for automatically clearing
same minor nit as above...
+ * a variable of type @type. The newly deifined function calls
+ * the corresponding pre-defined function @func.
+ */
+# define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \
+ static inline void VIR_AUTOCLEAR_FUNC_NAME(type)(type *_ptr) \
+ { \
+ (func)(_ptr); \
+ } \
+
+/**
+ * VIR_AUTOFREE:
+ * @type: type of the variables(s) to free automatically
+ *
+ * Macro to automatically free the memory allocated to
+ * the variable(s) declared with it by calling virFree
+ * when the variable goes out of scope.
+ */
+# define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
+
+/**
+ * VIR_AUTOPTR:
+ * @type: type of the variables(s) to free automatically
+ *
+ * Macro to automatically free the memory allocated to
+ * the variable(s) declared with it by calling the function
+ * defined by VIR_DEFINE_AUTOPTR_FUNC when the variable
+ * goes out of scope.
+ */
+# define VIR_AUTOPTR(type) \
+ __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) type
+
+/**
+ * VIR_AUTOCLEAR:
+ * @type: type of the variables(s) to free automatically
+ *
+ * Macro to automatically clear the variable(s) declared
+ * with it by calling the function defined by
+ * VIR_DEFINE_AUTOCLEAR_FUNC when the variabl* goes out
s/*/e
+ * of scope.
+ */
+# define VIR_AUTOCLEAR(type) \
+ __attribute__((cleanup(VIR_AUTOCLEAR_FUNC_NAME(type)))) type
I don't see any functional problems here, I like the changes, however, if we go
ahead with merging any future patch sets, I'd probably like to see them to
follow the steps we discussed earlier (what the initial focus should be) since,
subjectively, it poses a better logical order:
1 patch series
1) Introduce VIR_AUTOFREE macro
2) use it at as many modules and places as possible (doesn't need to 100%,
since there probably be a few caveats)
another patch series
3) Introduce VIR_AUTOPTR and friends
4) use those in as many places as the simple straightforward replacement goes
another patch series
5) Introduce VIR_AUTOCLEAR and friends
6) use those
Erik
8:12 a.m.
New subject: [libvirt] [RFC 1/4] add macros for implementing automatic cleanup functionality
On Thu, May 31, 2018 at 02:50:31PM +0200, Erik Skultety wrote:
On Wed, May 30, 2018 at 02:30:04AM +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>
> ---
> src/util/viralloc.h | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 69 insertions(+)
>
> diff --git a/src/util/viralloc.h b/src/util/viralloc.h
> index 69d0f90..e1c31c3 100644
> --- a/src/util/viralloc.h
> +++ b/src/util/viralloc.h
> @@ -596,4 +596,73 @@ void virAllocTestInit(void);
> int virAllocTestCount(void);
> void virAllocTestOOM(int n, int m);
> void virAllocTestHook(void (*func)(int, void*), void *data);
> +
> +# define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
> +# define VIR_AUTOCLEAR_FUNC_NAME(type) virAutoClear##type
> +
> +/**
> + * VIR_DEFINE_AUTOPTR_FUNC:
> + * @type: type of the variables(s) to free automatically
> + * @func: cleanup function to be automatically called
> + *
> + * Thos macro defines a function for automatically freeing
s/Thos/This
s/automatically freeing resources/automatic freeing of resources
> + * resources allocated to a variable of type @type. The newly
> + * defined function calls the corresponding pre-defined
> + * function @func.
> + */
> +# define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> + static inline void VIR_AUTOPTR_FUNC_NAME(type)(type *_ptr) \
> + { \
> + (func)(*_ptr); \
> + } \
> +
> +/**
> + * VIR_DEFINE_AUTOCLEAR_FUNC:
> + * @type: type of the variables(s) to free automatically
> + * @func: cleanup function to be automatically called
> + *
> + * This macro defines a function for automatically clearing
same minor nit as above...
> + * a variable of type @type. The newly deifined function calls
> + * the corresponding pre-defined function @func.
> + */
> +# define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \
> + static inline void VIR_AUTOCLEAR_FUNC_NAME(type)(type *_ptr) \
> + { \
> + (func)(_ptr); \
> + } \
> +
> +/**
> + * VIR_AUTOFREE:
> + * @type: type of the variables(s) to free automatically
> + *
> + * Macro to automatically free the memory allocated to
> + * the variable(s) declared with it by calling virFree
> + * when the variable goes out of scope.
> + */
> +# define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
> +
> +/**
> + * VIR_AUTOPTR:
> + * @type: type of the variables(s) to free automatically
> + *
> + * Macro to automatically free the memory allocated to
> + * the variable(s) declared with it by calling the function
> + * defined by VIR_DEFINE_AUTOPTR_FUNC when the variable
> + * goes out of scope.
> + */
> +# define VIR_AUTOPTR(type) \
> + __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) type
> +
> +/**
> + * VIR_AUTOCLEAR:
> + * @type: type of the variables(s) to free automatically
> + *
> + * Macro to automatically clear the variable(s) declared
> + * with it by calling the function defined by
> + * VIR_DEFINE_AUTOCLEAR_FUNC when the variabl* goes out
s/*/e
> + * of scope.
> + */
> +# define VIR_AUTOCLEAR(type) \
> + __attribute__((cleanup(VIR_AUTOCLEAR_FUNC_NAME(type)))) type
I don't see any functional problems here, I like the changes, however, if we go
ahead with merging any future patch sets, I'd probably like to see them to
follow the steps we discussed earlier (what the initial focus should be) since,
subjectively, it poses a better logical order:
1 patch series
1) Introduce VIR_AUTOFREE macro
2) use it at as many modules and places as possible (doesn't need to 100%,
since there probably be a few caveats)
another patch series
3) Introduce VIR_AUTOPTR and friends
4) use those in as many places as the simple straightforward replacement goes
another patch series
5) Introduce VIR_AUTOCLEAR and friends
6) use those
I don't share the same view, this would result in three huge patch
series and it would be annoying to review it.
IMHO introducing all of these in a single patch make sense and after
that we should follow with incremental switch to these macros ideally
having one patch per file. Some parts of the existing code would need
some modification before we can use these macros which would be done
separately from the one patch that implements the usage of these macros.
Switching majority of libvirt code into this new concept is dangerous
since it may introduce some issues or bugs and having small changes
makes it easier to figure out what cause the issue.
Pavel
8:37 a.m.
New subject: [libvirt] [RFC 1/4] add macros for implementing automatic cleanup functionality
On Thu, May 31, 2018 at 03:12:17PM +0200, Pavel Hrdina wrote:
On Thu, May 31, 2018 at 02:50:31PM +0200, Erik Skultety wrote:
> On Wed, May 30, 2018 at 02:30:04AM +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>
> > ---
> > src/util/viralloc.h | 69
+++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 69 insertions(+)
> >
> > diff --git a/src/util/viralloc.h b/src/util/viralloc.h
> > index 69d0f90..e1c31c3 100644
> > --- a/src/util/viralloc.h
> > +++ b/src/util/viralloc.h
> > @@ -596,4 +596,73 @@ void virAllocTestInit(void);
> > int virAllocTestCount(void);
> > void virAllocTestOOM(int n, int m);
> > void virAllocTestHook(void (*func)(int, void*), void *data);
> > +
> > +# define VIR_AUTOPTR_FUNC_NAME(type) virAutoPtr##type
> > +# define VIR_AUTOCLEAR_FUNC_NAME(type) virAutoClear##type
> > +
> > +/**
> > + * VIR_DEFINE_AUTOPTR_FUNC:
> > + * @type: type of the variables(s) to free automatically
> > + * @func: cleanup function to be automatically called
> > + *
> > + * Thos macro defines a function for automatically freeing
>
> s/Thos/This
> s/automatically freeing resources/automatic freeing of resources
>
> > + * resources allocated to a variable of type @type. The newly
> > + * defined function calls the corresponding pre-defined
> > + * function @func.
> > + */
> > +# define VIR_DEFINE_AUTOPTR_FUNC(type, func) \
> > + static inline void VIR_AUTOPTR_FUNC_NAME(type)(type *_ptr) \
> > + { \
> > + (func)(*_ptr); \
> > + } \
> > +
> > +/**
> > + * VIR_DEFINE_AUTOCLEAR_FUNC:
> > + * @type: type of the variables(s) to free automatically
> > + * @func: cleanup function to be automatically called
> > + *
> > + * This macro defines a function for automatically clearing
>
> same minor nit as above...
>
> > + * a variable of type @type. The newly deifined function calls
> > + * the corresponding pre-defined function @func.
> > + */
> > +# define VIR_DEFINE_AUTOCLEAR_FUNC(type, func) \
> > + static inline void VIR_AUTOCLEAR_FUNC_NAME(type)(type *_ptr) \
> > + { \
> > + (func)(_ptr); \
> > + } \
> > +
> > +/**
> > + * VIR_AUTOFREE:
> > + * @type: type of the variables(s) to free automatically
> > + *
> > + * Macro to automatically free the memory allocated to
> > + * the variable(s) declared with it by calling virFree
> > + * when the variable goes out of scope.
> > + */
> > +# define VIR_AUTOFREE(type) __attribute__((cleanup(virFree))) type
> > +
> > +/**
> > + * VIR_AUTOPTR:
> > + * @type: type of the variables(s) to free automatically
> > + *
> > + * Macro to automatically free the memory allocated to
> > + * the variable(s) declared with it by calling the function
> > + * defined by VIR_DEFINE_AUTOPTR_FUNC when the variable
> > + * goes out of scope.
> > + */
> > +# define VIR_AUTOPTR(type) \
> > + __attribute__((cleanup(VIR_AUTOPTR_FUNC_NAME(type)))) type
> > +
> > +/**
> > + * VIR_AUTOCLEAR:
> > + * @type: type of the variables(s) to free automatically
> > + *
> > + * Macro to automatically clear the variable(s) declared
> > + * with it by calling the function defined by
> > + * VIR_DEFINE_AUTOCLEAR_FUNC when the variabl* goes out
>
> s/*/e
>
> > + * of scope.
> > + */
> > +# define VIR_AUTOCLEAR(type) \
> > + __attribute__((cleanup(VIR_AUTOCLEAR_FUNC_NAME(type)))) type
>
> I don't see any functional problems here, I like the changes, however, if we go
> ahead with merging any future patch sets, I'd probably like to see them to
> follow the steps we discussed earlier (what the initial focus should be) since,
> subjectively, it poses a better logical order:
>
> 1 patch series
> 1) Introduce VIR_AUTOFREE macro
> 2) use it at as many modules and places as possible (doesn't need to 100%,
> since there probably be a few caveats)
>
> another patch series
> 3) Introduce VIR_AUTOPTR and friends
> 4) use those in as many places as the simple straightforward replacement goes
>
> another patch series
> 5) Introduce VIR_AUTOCLEAR and friends
> 6) use those
I don't share the same view, this would result in three huge patch
series and it would be annoying to review it.
Yeah, I elaborated on my vision more in the last patch, I understand that it's
not visible immediately from this response, my bad, this was just supposed to
be a general idea, we'll of course need some granularity, noone wants to review
such a beast.
IMHO introducing all of these in a single patch make sense and after
that we should follow with incremental switch to these macros ideally
having one patch per file. Some parts of the existing code would need
some modification before we can use these macros which would be done
separately from the one patch that implements the usage of these macros.
After a private discussion with Pavel, an alternative approach (again, using
some degree of granularity) would be to perform all the necessary changes for
each module of the series (e.g. a third of the the modules within src/util per
series - this is just an idea) in separate patches of the same series (1 patch
for VIR_AUTOFREE, 1 for AUTOPTR, 1 for AUTOCLEAR, possibly more in order to
prepare the module for such changes), thus knowing for sure which files we've
converted already and therefore not keeping track of what changes have been to
which modules. I have to give it to Pavel, that this seems like a better
approach given the timeline for GSoC.
Switching majority of libvirt code into this new concept is
dangerous
since it may introduce some issues or bugs and having small changes
makes it easier to figure out what cause the issue.
It sure is, without arguments...
Erik
4 p.m.
New subject: [libvirt] [RFC 2/4] add automatic cleanup support in src/util/virarptable.c
Modifiy code to use cleanup macros where required.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virarptable.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/src/util/virarptable.c b/src/util/virarptable.c
index c0e90dc..f53a479 100644
--- a/src/util/virarptable.c
+++ b/src/util/virarptable.c
@@ -71,9 +71,8 @@ virArpTableGet(void)
{
int num = 0;
int msglen;
- void *nlData = NULL;
+ VIR_AUTOFREE(void *) nlData = NULL;
virArpTablePtr table = NULL;
- char *ipstr = NULL;
struct nlmsghdr* nh;
struct rtattr * tb[NDA_MAX+1];
@@ -89,6 +88,7 @@ virArpTableGet(void)
VIR_WARNINGS_NO_CAST_ALIGN
for (; NLMSG_OK(nh, msglen); nh = NLMSG_NEXT(nh, msglen)) {
VIR_WARNINGS_RESET
+ VIR_AUTOFREE(char *) ipstr = NULL;
struct ndmsg *r = NLMSG_DATA(nh);
int len = nh->nlmsg_len;
void *addr;
@@ -134,8 +134,6 @@ virArpTableGet(void)
if (VIR_STRDUP(table->t[num].ipaddr, ipstr) < 0)
goto cleanup;
-
- VIR_FREE(ipstr);
}
if (tb[NDA_LLADDR]) {
@@ -155,13 +153,10 @@ virArpTableGet(void)
}
end_of_netlink_messages:
- VIR_FREE(nlData);
return table;
cleanup:
virArpTableFree(table);
- VIR_FREE(ipstr);
- VIR_FREE(nlData);
return NULL;
}
--
1.8.3.1
7:52 a.m.
New subject: [libvirt] [RFC 2/4] add automatic cleanup support in src/util/virarptable.c
On Wed, May 30, 2018 at 02:30:05AM +0530, Sukrit Bhatnagar wrote:
Modifiy code to use cleanup macros where required.
s/fiy/fy
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virarptable.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/src/util/virarptable.c b/src/util/virarptable.c
index c0e90dc..f53a479 100644
--- a/src/util/virarptable.c
+++ b/src/util/virarptable.c
@@ -71,9 +71,8 @@ virArpTableGet(void)
{
int num = 0;
int msglen;
- void *nlData = NULL;
+ VIR_AUTOFREE(void *) nlData = NULL;
virArpTablePtr table = NULL;
- char *ipstr = NULL;
struct nlmsghdr* nh;
struct rtattr * tb[NDA_MAX+1];
@@ -89,6 +88,7 @@ virArpTableGet(void)
VIR_WARNINGS_NO_CAST_ALIGN
for (; NLMSG_OK(nh, msglen); nh = NLMSG_NEXT(nh, msglen)) {
VIR_WARNINGS_RESET
+ VIR_AUTOFREE(char *) ipstr = NULL;
struct ndmsg *r = NLMSG_DATA(nh);
int len = nh->nlmsg_len;
void *addr;
@@ -134,8 +134,6 @@ virArpTableGet(void)
if (VIR_STRDUP(table->t[num].ipaddr, ipstr) < 0)
goto cleanup;
-
- VIR_FREE(ipstr);
}
if (tb[NDA_LLADDR]) {
@@ -155,13 +153,10 @@ virArpTableGet(void)
}
end_of_netlink_messages:
- VIR_FREE(nlData);
return table;
cleanup:
virArpTableFree(table);
- VIR_FREE(ipstr);
- VIR_FREE(nlData);
return NULL;
}
Looks good.
Erik
4 p.m.
New subject: [libvirt] [RFC 3/4] add automatic cleanup support in src/util/virauthconfig.c
Modifiy code to use cleanup macros where required.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virauthconfig.c | 34 ++++++++++++----------------------
1 file changed, 12 insertions(+), 22 deletions(-)
diff --git a/src/util/virauthconfig.c b/src/util/virauthconfig.c
index 91c9c0c..66f7f7e 100644
--- a/src/util/virauthconfig.c
+++ b/src/util/virauthconfig.c
@@ -106,10 +106,9 @@ int virAuthConfigLookup(virAuthConfigPtr auth,
const char *credname,
const char **value)
{
- char *authgroup = NULL;
- char *credgroup = NULL;
+ VIR_AUTOFREE(char *) authgroup = NULL;
+ VIR_AUTOFREE(char *) credgroup = NULL;
const char *authcred;
- int ret = -1;
*value = NULL;
@@ -119,47 +118,38 @@ int virAuthConfigLookup(virAuthConfigPtr auth,
hostname = "localhost";
if (virAsprintf(&authgroup, "auth-%s-%s", service, hostname) < 0)
- goto cleanup;
+ return -1;
if (!virKeyFileHasGroup(auth->keyfile, authgroup)) {
VIR_FREE(authgroup);
if (virAsprintf(&authgroup, "auth-%s-%s", service,
"default") < 0)
- goto cleanup;
+ return -1;
}
- if (!virKeyFileHasGroup(auth->keyfile, authgroup)) {
- ret = 0;
- goto cleanup;
- }
+ if (!virKeyFileHasGroup(auth->keyfile, authgroup))
+ return 0;
if (!(authcred = virKeyFileGetValueString(auth->keyfile, authgroup,
"credentials"))) {
virReportError(VIR_ERR_CONF_SYNTAX,
_("Missing item 'credentials' in group '%s'
in '%s'"),
authgroup, auth->path);
- goto cleanup;
+ return -1;
}
if (virAsprintf(&credgroup, "credentials-%s", authcred) < 0)
- goto cleanup;
+ return -1;
if (!virKeyFileHasGroup(auth->keyfile, credgroup)) {
virReportError(VIR_ERR_CONF_SYNTAX,
_("Missing group 'credentials-%s' referenced from
group '%s' in '%s'"),
authcred, authgroup, auth->path);
- goto cleanup;
+ return -1;
}
- if (!virKeyFileHasValue(auth->keyfile, credgroup, credname)) {
- ret = 0;
- goto cleanup;
- }
+ if (!virKeyFileHasValue(auth->keyfile, credgroup, credname))
+ return 0;
*value = virKeyFileGetValueString(auth->keyfile, credgroup, credname);
- ret = 0;
-
- cleanup:
- VIR_FREE(authgroup);
- VIR_FREE(credgroup);
- return ret;
+ return 0;
}
--
1.8.3.1
7:53 a.m.
New subject: [libvirt] [RFC 3/4] add automatic cleanup support in src/util/virauthconfig.c
On Wed, May 30, 2018 at 02:30:06AM +0530, Sukrit Bhatnagar wrote:
Modifiy code to use cleanup macros where required.
s/fiy/fy
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virauthconfig.c | 34 ++++++++++++----------------------
1 file changed, 12 insertions(+), 22 deletions(-)
diff --git a/src/util/virauthconfig.c b/src/util/virauthconfig.c
index 91c9c0c..66f7f7e 100644
--- a/src/util/virauthconfig.c
+++ b/src/util/virauthconfig.c
@@ -106,10 +106,9 @@ int virAuthConfigLookup(virAuthConfigPtr auth,
const char *credname,
const char **value)
{
- char *authgroup = NULL;
- char *credgroup = NULL;
+ VIR_AUTOFREE(char *) authgroup = NULL;
+ VIR_AUTOFREE(char *) credgroup = NULL;
const char *authcred;
- int ret = -1;
*value = NULL;
@@ -119,47 +118,38 @@ int virAuthConfigLookup(virAuthConfigPtr auth,
hostname = "localhost";
if (virAsprintf(&authgroup, "auth-%s-%s", service, hostname) < 0)
- goto cleanup;
+ return -1;
if (!virKeyFileHasGroup(auth->keyfile, authgroup)) {
VIR_FREE(authgroup);
if (virAsprintf(&authgroup, "auth-%s-%s", service,
"default") < 0)
- goto cleanup;
+ return -1;
}
- if (!virKeyFileHasGroup(auth->keyfile, authgroup)) {
- ret = 0;
- goto cleanup;
- }
+ if (!virKeyFileHasGroup(auth->keyfile, authgroup))
+ return 0;
if (!(authcred = virKeyFileGetValueString(auth->keyfile, authgroup,
"credentials"))) {
virReportError(VIR_ERR_CONF_SYNTAX,
_("Missing item 'credentials' in group '%s'
in '%s'"),
authgroup, auth->path);
- goto cleanup;
+ return -1;
}
if (virAsprintf(&credgroup, "credentials-%s", authcred) < 0)
- goto cleanup;
+ return -1;
if (!virKeyFileHasGroup(auth->keyfile, credgroup)) {
virReportError(VIR_ERR_CONF_SYNTAX,
_("Missing group 'credentials-%s' referenced from
group '%s' in '%s'"),
authcred, authgroup, auth->path);
- goto cleanup;
+ return -1;
}
- if (!virKeyFileHasValue(auth->keyfile, credgroup, credname)) {
- ret = 0;
- goto cleanup;
- }
+ if (!virKeyFileHasValue(auth->keyfile, credgroup, credname))
+ return 0;
*value = virKeyFileGetValueString(auth->keyfile, credgroup, credname);
- ret = 0;
-
- cleanup:
- VIR_FREE(authgroup);
- VIR_FREE(credgroup);
- return ret;
+ return 0;
}
Looks fine as well.
Erik
4 p.m.
New subject: [libvirt] [RFC 4/4] add automatic cleanup support in src/util/virauth.c
Define a new cleanup function for virAuthConfigPtr in
src/util/virauthconfig.h.
Modifiy code to use cleanup macros where required.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virauth.c | 66 ++++++++++++++++++------------------------------
src/util/virauthconfig.h | 3 +++
2 files changed, 27 insertions(+), 42 deletions(-)
diff --git a/src/util/virauth.c b/src/util/virauth.c
index adb093e..aa7593c 100644
--- a/src/util/virauth.c
+++ b/src/util/virauth.c
@@ -26,7 +26,6 @@
#include "virauth.h"
#include "virutil.h"
-#include "viralloc.h"
#include "virlog.h"
#include "datatypes.h"
#include "virerror.h"
@@ -42,10 +41,9 @@ int
virAuthGetConfigFilePathURI(virURIPtr uri,
char **path)
{
- int ret = -1;
size_t i;
const char *authenv = virGetEnvBlockSUID("LIBVIRT_AUTH_FILE");
- char *userdir = NULL;
+ VIR_AUTOFREE(char *) userdir = NULL;
*path = NULL;
@@ -54,7 +52,7 @@ virAuthGetConfigFilePathURI(virURIPtr uri,
if (authenv) {
VIR_DEBUG("Using path from env '%s'", authenv);
if (VIR_STRDUP(*path, authenv) < 0)
- goto cleanup;
+ return -1;
return 0;
}
@@ -64,41 +62,38 @@ virAuthGetConfigFilePathURI(virURIPtr uri,
uri->params[i].value) {
VIR_DEBUG("Using path from URI '%s'",
uri->params[i].value);
if (VIR_STRDUP(*path, uri->params[i].value) < 0)
- goto cleanup;
+ return -1;
return 0;
}
}
}
if (!(userdir = virGetUserConfigDirectory()))
- goto cleanup;
+ return -1;
if (virAsprintf(path, "%s/auth.conf", userdir) < 0)
- goto cleanup;
+ return -1;
VIR_DEBUG("Checking for readability of '%s'", *path);
- if (access(*path, R_OK) == 0)
- goto done;
+ if (access(*path, R_OK) == 0) {
+ VIR_DEBUG("Using auth file '%s'", NULLSTR(*path));
+ return 0;
+ }
VIR_FREE(*path);
if (VIR_STRDUP(*path, SYSCONFDIR "/libvirt/auth.conf") < 0)
- goto cleanup;
+ return -1;
VIR_DEBUG("Checking for readability of '%s'", *path);
- if (access(*path, R_OK) == 0)
- goto done;
+ if (access(*path, R_OK) == 0) {
+ VIR_DEBUG("Using auth file '%s'", NULLSTR(*path));
+ return 0;
+ }
VIR_FREE(*path);
- done:
- ret = 0;
-
- VIR_DEBUG("Using auth file '%s'", NULLSTR(*path));
- cleanup:
- VIR_FREE(userdir);
-
- return ret;
+ return 0;
}
@@ -117,8 +112,7 @@ virAuthGetCredential(const char *servicename,
const char *path,
char **value)
{
- int ret = -1;
- virAuthConfigPtr config = NULL;
+ VIR_AUTOPTR(virAuthConfigPtr) config = NULL;
const char *tmp;
*value = NULL;
@@ -127,23 +121,19 @@ virAuthGetCredential(const char *servicename,
return 0;
if (!(config = virAuthConfigNew(path)))
- goto cleanup;
+ return -1;
if (virAuthConfigLookup(config,
servicename,
hostname,
credname,
&tmp) < 0)
- goto cleanup;
+ return -1;
if (VIR_STRDUP(*value, tmp) < 0)
- goto cleanup;
+ return -1;
- ret = 0;
-
- cleanup:
- virAuthConfigFree(config);
- return ret;
+ return 0;
}
@@ -156,7 +146,7 @@ virAuthGetUsernamePath(const char *path,
{
unsigned int ncred;
virConnectCredential cred;
- char *prompt;
+ VIR_AUTOFREE(char *) prompt = NULL;
char *ret = NULL;
if (virAuthGetCredential(servicename, hostname, "username", path, &ret)
< 0)
@@ -193,8 +183,6 @@ virAuthGetUsernamePath(const char *path,
break;
}
- VIR_FREE(prompt);
-
return cred.result;
}
@@ -207,7 +195,7 @@ virAuthGetUsername(virConnectPtr conn,
const char *hostname)
{
char *ret;
- char *path;
+ VIR_AUTOFREE(char *) path = NULL;
if (virAuthGetConfigFilePath(conn, &path) < 0)
return NULL;
@@ -215,8 +203,6 @@ virAuthGetUsername(virConnectPtr conn,
ret = virAuthGetUsernamePath(path, auth, servicename,
defaultUsername, hostname);
- VIR_FREE(path);
-
return ret;
}
@@ -230,7 +216,7 @@ virAuthGetPasswordPath(const char *path,
{
unsigned int ncred;
virConnectCredential cred;
- char *prompt;
+ VIR_AUTOFREE(char *) prompt = NULL;
char *ret = NULL;
if (virAuthGetCredential(servicename, hostname, "password", path, &ret)
< 0)
@@ -264,8 +250,6 @@ virAuthGetPasswordPath(const char *path,
break;
}
- VIR_FREE(prompt);
-
return cred.result;
}
@@ -278,14 +262,12 @@ virAuthGetPassword(virConnectPtr conn,
const char *hostname)
{
char *ret;
- char *path;
+ VIR_AUTOFREE(char *) path = NULL;
if (virAuthGetConfigFilePath(conn, &path) < 0)
return NULL;
ret = virAuthGetPasswordPath(path, auth, servicename, username, hostname);
- VIR_FREE(path);
-
return ret;
}
diff --git a/src/util/virauthconfig.h b/src/util/virauthconfig.h
index ac0ceeb..375ccad 100644
--- a/src/util/virauthconfig.h
+++ b/src/util/virauthconfig.h
@@ -24,6 +24,7 @@
# define __VIR_AUTHCONFIG_H__
# include "internal.h"
+# include "viralloc.h"
typedef struct _virAuthConfig virAuthConfig;
typedef virAuthConfig *virAuthConfigPtr;
@@ -42,4 +43,6 @@ int virAuthConfigLookup(virAuthConfigPtr auth,
const char *credname,
const char **value);
+VIR_DEFINE_AUTOPTR_FUNC(virAuthConfigPtr, virAuthConfigFree)
+
#endif /* __VIR_AUTHCONFIG_H__ */
--
1.8.3.1
8:05 a.m.
New subject: [libvirt] [RFC 4/4] add automatic cleanup support in src/util/virauth.c
On Wed, May 30, 2018 at 02:30:07AM +0530, Sukrit Bhatnagar wrote:
Define a new cleanup function for virAuthConfigPtr in
src/util/virauthconfig.h.
Modifiy code to use cleanup macros where required.
s/fiy/fy
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr(a)gmail.com>
---
src/util/virauth.c | 66 ++++++++++++++++++------------------------------
src/util/virauthconfig.h | 3 +++
2 files changed, 27 insertions(+), 42 deletions(-)
diff --git a/src/util/virauth.c b/src/util/virauth.c
index adb093e..aa7593c 100644
--- a/src/util/virauth.c
+++ b/src/util/virauth.c
@@ -26,7 +26,6 @@
#include "virauth.h"
#include "virutil.h"
-#include "viralloc.h"
#include "virlog.h"
#include "datatypes.h"
#include "virerror.h"
@@ -42,10 +41,9 @@ int
virAuthGetConfigFilePathURI(virURIPtr uri,
char **path)
{
- int ret = -1;
size_t i;
const char *authenv = virGetEnvBlockSUID("LIBVIRT_AUTH_FILE");
- char *userdir = NULL;
+ VIR_AUTOFREE(char *) userdir = NULL;
*path = NULL;
@@ -54,7 +52,7 @@ virAuthGetConfigFilePathURI(virURIPtr uri,
if (authenv) {
VIR_DEBUG("Using path from env '%s'", authenv);
if (VIR_STRDUP(*path, authenv) < 0)
- goto cleanup;
+ return -1;
return 0;
}
@@ -64,41 +62,38 @@ virAuthGetConfigFilePathURI(virURIPtr uri,
uri->params[i].value) {
VIR_DEBUG("Using path from URI '%s'",
uri->params[i].value);
if (VIR_STRDUP(*path, uri->params[i].value) < 0)
- goto cleanup;
+ return -1;
return 0;
}
}
}
if (!(userdir = virGetUserConfigDirectory()))
- goto cleanup;
+ return -1;
if (virAsprintf(path, "%s/auth.conf", userdir) < 0)
- goto cleanup;
+ return -1;
VIR_DEBUG("Checking for readability of '%s'", *path);
- if (access(*path, R_OK) == 0)
- goto done;
+ if (access(*path, R_OK) == 0) {
+ VIR_DEBUG("Using auth file '%s'", NULLSTR(*path));
+ return 0;
+ }
VIR_FREE(*path);
if (VIR_STRDUP(*path, SYSCONFDIR "/libvirt/auth.conf") < 0)
- goto cleanup;
+ return -1;
VIR_DEBUG("Checking for readability of '%s'", *path);
- if (access(*path, R_OK) == 0)
- goto done;
+ if (access(*path, R_OK) == 0) {
+ VIR_DEBUG("Using auth file '%s'", NULLSTR(*path));
+ return 0;
+ }
VIR_FREE(*path);
- done:
- ret = 0;
-
- VIR_DEBUG("Using auth file '%s'", NULLSTR(*path));
- cleanup:
- VIR_FREE(userdir);
-
- return ret;
+ return 0;
}
@@ -117,8 +112,7 @@ virAuthGetCredential(const char *servicename,
const char *path,
char **value)
{
- int ret = -1;
- virAuthConfigPtr config = NULL;
+ VIR_AUTOPTR(virAuthConfigPtr) config = NULL;
As I said in patch 1, changes related to VIR_AUTOPTR should be a separate patch
series, IOW the patches shouldn't combine changing both VIR_AUTOFREE and
VIR_AUTOPTR.
Otherwise, the changes look good, looking forward to more files. As we
discussed privately, having a series addressing src/util then e.g. src/conf as
separate patch series which can be merged gradually is IMHO better than trying
to make all the VIR_FREE changes everywhere, then sending a massive series
consisting of tens of patches and then doing the same for VIR_AUTOPTR.
Erik
2368
days inactive
2370
days old
10 comments
3 participants
participants (3)
-
Erik Skultety -
Pavel Hrdina -
Sukrit Bhatnagar