[libvirt] [RFC 0/4] add automatic cleanup functionality in some files
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
New macros are added to src/util/viralloc.h which help in adding cleanup attribute to variable declarations. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@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
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@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
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@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
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@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
Modifiy code to use cleanup macros where required. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@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
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@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
Modifiy code to use cleanup macros where required. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@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
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@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
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@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
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@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
participants (3)
-
Erik Skultety -
Pavel Hrdina -
Sukrit Bhatnagar