[libvirt] [PATCH v1 00/18] use VIR_AUTOFREE in src/util
This series of patches modifies some files in src/util to use VIR_AUTOFREE for automatic freeing of memory and get rid of some VIR_FREE macro invocations. Sukrit Bhatnagar (18): add macros for implementing automatic cleanup functionality use VIR_AUTOFREE in src/util/virarptable.c use VIR_AUTOFREE in src/util/virauth.c use VIR_AUTOFREE in src/util/virauthconfig.c use VIR_AUTOFREE in src/util/iohelper.c use VIR_AUTOFREE in src/util/viraudit.c use VIR_AUTOFREE in src/util/virbuffer.c use VIR_AUTOFREE in src/util/vircgroup.c use VIR_AUTOFREE in src/util/virfcp.c use VIR_AUTOFREE in src/util/virdnsmasq.c use VIR_AUTOFREE in src/util/vireventpoll.c use VIR_AUTOFREE in src/util/virdbus.c use VIR_AUTOFREE in src/util/virfdstream.c use VIR_AUTOFREE in src/util/virfile.c use VIR_AUTOFREE in src/util/virconf.c use VIR_AUTOFREE in src/util/virfilecache.c use VIR_AUTOFREE in src/util/virfirewall.c use VIR_AUTOFREE in src/util/virhook.c src/util/iohelper.c | 4 +- src/util/viralloc.h | 69 +++++++ src/util/virarptable.c | 9 +- src/util/viraudit.c | 3 +- src/util/virauth.c | 60 ++---- src/util/virauthconfig.c | 34 ++- src/util/virbuffer.c | 33 ++- src/util/vircgroup.c | 526 ++++++++++++++++------------------------------- src/util/virconf.c | 42 ++-- src/util/virdbus.c | 28 +-- src/util/virdnsmasq.c | 116 ++++------- src/util/vireventpoll.c | 7 +- src/util/virfcp.c | 20 +- src/util/virfdstream.c | 3 +- src/util/virfile.c | 303 +++++++++------------------ src/util/virfilecache.c | 35 +--- src/util/virfirewall.c | 13 +- src/util/virhook.c | 16 +- 18 files changed, 487 insertions(+), 834 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..bb37c48 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 + * + * This macro defines a function for automatic freeing of + * 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 automatic clearing of + * 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 variable 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 Sun, Jun 03, 2018 at 01:41:59PM +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..bb37c48 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
- we are only going to use this with a single variable (or object if you will), so 'variables' doesn't sound right, how about: "type of the variable to be freed automatically"
+ * @func: cleanup function to be automatically called + * + * This macro defines a function for automatic freeing of + * resources allocated to a variable of type @type. The newly + * defined function calls the corresponding pre-defined
"This newly defined function works as a necessary wrapper around @func"
+ * 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 automatic clearing of + * a variable of type @type. The newly deifined function calls + * the corresponding pre-defined function @func.
Same comments as above...
+ */ +# 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
here too...
+ * + * Macro to automatically free the memory allocated to + * the variable(s) declared with it by calling virFree
s/(s)//
+ * 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
same here...
+ * 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
and here...
+ * with it by calling the function defined by + * VIR_DEFINE_AUTOCLEAR_FUNC when the variable goes out + * of scope. + */ +# define VIR_AUTOCLEAR(type) \ + __attribute__((cleanup(VIR_AUTOCLEAR_FUNC_NAME(type)))) type + #endif /* __VIR_MEMORY_H_ */ -- 1.8.3.1
Sorry, I missed these in the RFC, now that I read it again, I thought of a few wording improvements, functionally though, still fine and you have my R-b. Erik
On Sun, Jun 03, 2018 at 01:41:59PM +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>
If you recall the recent RFC thread [1], I agreed with Pavel that we should introduce complete changes to each file, so that we don't need to track was has been already done for what file and what is still missing, so the series would look something like this: 1) vir<some_file>.c preparations (especially VIR_{AUTOPTR,AUTOCLEAR} might need that) 2) Use VIR_AUTOFREE across vir<some_file>.c 3) Use VIR_AUTOPTR across vir<some_file>.c 4) Use VIR_AUTOCLEAR across vir<some_file>.c ...One more thing, the commit subject of every patch in the current series should probably look like this: util: <module>: Use VIR_AUTOFREE instead of VIR_FREE for scalar types and the corresponding commit message: By making use of the GCC's __attribute__((cleanup)) handled by VIR_AUTOFREE macro, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections. [1] https://www.redhat.com/archives/libvir-list/2018-May/msg02391.html Erik
--- src/util/viralloc.h | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+)
diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 69d0f90..bb37c48 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 + * + * This macro defines a function for automatic freeing of + * 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 automatic clearing of + * 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 variable goes out + * of scope. + */ +# define VIR_AUTOCLEAR(type) \ + __attribute__((cleanup(VIR_AUTOCLEAR_FUNC_NAME(type)))) type + #endif /* __VIR_MEMORY_H_ */ -- 1.8.3.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, 5 Jun 2018 at 21:00, Erik Skultety <eskultet@redhat.com> wrote:
On Sun, Jun 03, 2018 at 01:41:59PM +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>
If you recall the recent RFC thread [1], I agreed with Pavel that we should introduce complete changes to each file, so that we don't need to track was has been already done for what file and what is still missing, so the series would look something like this:
1) vir<some_file>.c preparations (especially VIR_{AUTOPTR,AUTOCLEAR} might need that) 2) Use VIR_AUTOFREE across vir<some_file>.c 3) Use VIR_AUTOPTR across vir<some_file>.c 4) Use VIR_AUTOCLEAR across vir<some_file>.c
...One more thing, the commit subject of every patch in the current series should probably look like this: util: <module>: Use VIR_AUTOFREE instead of VIR_FREE for scalar types
and the corresponding commit message: By making use of the GCC's __attribute__((cleanup)) handled by VIR_AUTOFREE macro, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections.
[1] https://www.redhat.com/archives/libvir-list/2018-May/msg02391.html
I have some rudimentary queries regarding this approach: - The <module> in commit subject should be like "vircgroup" or just "cgroup"? - Do I create a separate series of patch even for those files who do not require VIR_AUTOPTR and VIR_AUTOCLEAR but only VIR_AUTOFREE, like viraudit.c? - Do I use the same commit subject and message, with minor changes of course, for all patches in a series (AUTOFREE, AUTOPTR and AUTOCLEAR)? - In the case of files like virauth.c which require changes in virauthconfig.h instead of virauth.h, do I batch all the files virauth* together? I am assuming that the changes to header file vir<some_file>.h corresponding to vir<some_file>.c will be in the same series if needed. -- Thanks, Sukrit
On Wed, Jun 06, 2018 at 06:42:29PM +0530, Sukrit Bhatnagar wrote:
On Tue, 5 Jun 2018 at 21:00, Erik Skultety <eskultet@redhat.com> wrote:
On Sun, Jun 03, 2018 at 01:41:59PM +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>
If you recall the recent RFC thread [1], I agreed with Pavel that we should introduce complete changes to each file, so that we don't need to track was has been already done for what file and what is still missing, so the series would look something like this:
1) vir<some_file>.c preparations (especially VIR_{AUTOPTR,AUTOCLEAR} might need that) 2) Use VIR_AUTOFREE across vir<some_file>.c 3) Use VIR_AUTOPTR across vir<some_file>.c 4) Use VIR_AUTOCLEAR across vir<some_file>.c
...One more thing, the commit subject of every patch in the current series should probably look like this: util: <module>: Use VIR_AUTOFREE instead of VIR_FREE for scalar types
and the corresponding commit message: By making use of the GCC's __attribute__((cleanup)) handled by VIR_AUTOFREE macro, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections.
[1] https://www.redhat.com/archives/libvir-list/2018-May/msg02391.html
I have some rudimentary queries regarding this approach:
- The <module> in commit subject should be like "vircgroup" or just "cgroup"?
preferably "cgroup" (you'll save 3 chars :)), but doesn't really matter, I think I pushed a patch yesterday where I actually added the 'vir' prefix, so whatever...
- Do I create a separate series of patch even for those files who do not require VIR_AUTOPTR and VIR_AUTOCLEAR but only VIR_AUTOFREE, like viraudit.c?
I'm getting the impression we're not on the same page yet, I suppose I should have added something like "..." in the example above or simply be more clear in general. So, let's be more specific, this series was 18 patches long and you need to add 2 more patches per file, so that would be 54 patches in a single series, that's big, so how about taking 5-10 files instead of 18, that would produce roughly 15-30 patches in a single series (a bit more manageable and as I mentioned somewhere else, complete changes that are fine can be merged independently) - so it's not going to be a separate series per file, rather a single series, following the pattern I mentioned in my previous response above, performing changes to up to N selected files, does it make more sense now?
- Do I use the same commit subject and message, with minor changes of course, for all patches in a series (AUTOFREE, AUTOPTR and AUTOCLEAR)?
Yep.
- In the case of files like virauth.c which require changes in virauthconfig.h instead of virauth.h, do I batch all the files virauth* together?
Yes, well, it should be coupled based on the change you're making, but you just mentioned it - "required changes" - IOW the patch would not otherwise compile.
I am assuming that the changes to header file vir<some_file>.h corresponding to vir<some_file>.c will be in the same series if needed.
See above, but yes. Let me know if there's still something which is not clear. Erik
On Wed, 6 Jun 2018 at 21:25, Erik Skultety <eskultet@redhat.com> wrote:
On Wed, Jun 06, 2018 at 06:42:29PM +0530, Sukrit Bhatnagar wrote:
On Tue, 5 Jun 2018 at 21:00, Erik Skultety <eskultet@redhat.com> wrote:
On Sun, Jun 03, 2018 at 01:41:59PM +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>
If you recall the recent RFC thread [1], I agreed with Pavel that we should introduce complete changes to each file, so that we don't need to track was has been already done for what file and what is still missing, so the series would look something like this:
1) vir<some_file>.c preparations (especially VIR_{AUTOPTR,AUTOCLEAR} might need that) 2) Use VIR_AUTOFREE across vir<some_file>.c 3) Use VIR_AUTOPTR across vir<some_file>.c 4) Use VIR_AUTOCLEAR across vir<some_file>.c
...One more thing, the commit subject of every patch in the current series should probably look like this: util: <module>: Use VIR_AUTOFREE instead of VIR_FREE for scalar types
and the corresponding commit message: By making use of the GCC's __attribute__((cleanup)) handled by VIR_AUTOFREE macro, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections.
[1] https://www.redhat.com/archives/libvir-list/2018-May/msg02391.html
I have some rudimentary queries regarding this approach:
- The <module> in commit subject should be like "vircgroup" or just "cgroup"?
preferably "cgroup" (you'll save 3 chars :)), but doesn't really matter, I think I pushed a patch yesterday where I actually added the 'vir' prefix, so whatever...
- Do I create a separate series of patch even for those files who do not require VIR_AUTOPTR and VIR_AUTOCLEAR but only VIR_AUTOFREE, like viraudit.c?
I'm getting the impression we're not on the same page yet, I suppose I should have added something like "..." in the example above or simply be more clear in general.
So, let's be more specific, this series was 18 patches long and you need to add 2 more patches per file, so that would be 54 patches in a single series, that's big, so how about taking 5-10 files instead of 18, that would produce roughly 15-30 patches in a single series (a bit more manageable and as I mentioned somewhere else, complete changes that are fine can be merged independently) - so it's not going to be a separate series per file, rather a single series, following the pattern I mentioned in my previous response above, performing changes to up to N selected files, does it make more sense now?
Yes, it does now.
- Do I use the same commit subject and message, with minor changes of course, for all patches in a series (AUTOFREE, AUTOPTR and AUTOCLEAR)?
Yep.
- In the case of files like virauth.c which require changes in virauthconfig.h instead of virauth.h, do I batch all the files virauth* together?
Yes, well, it should be coupled based on the change you're making, but you just mentioned it - "required changes" - IOW the patch would not otherwise compile.
I am assuming that the changes to header file vir<some_file>.h corresponding to vir<some_file>.c will be in the same series if needed.
See above, but yes.
Let me know if there's still something which is not clear.
No other queries for now, thanks!
Modify code to use VIR_AUTOFREE macro wherever 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
Modify code to use VIR_AUTOFREE macro wherever required. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virauth.c | 60 +++++++++++++++++++----------------------------------- 1 file changed, 21 insertions(+), 39 deletions(-) diff --git a/src/util/virauth.c b/src/util/virauth.c index adb093e..089a820 100644 --- a/src/util/virauth.c +++ b/src/util/virauth.c @@ -42,10 +42,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 +53,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 +63,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; } @@ -156,7 +152,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 +189,6 @@ virAuthGetUsernamePath(const char *path, break; } - VIR_FREE(prompt); - return cred.result; } @@ -206,18 +200,13 @@ virAuthGetUsername(virConnectPtr conn, const char *defaultUsername, const char *hostname) { - char *ret; - char *path; + VIR_AUTOFREE(char *) path = NULL; if (virAuthGetConfigFilePath(conn, &path) < 0) return NULL; - ret = virAuthGetUsernamePath(path, auth, servicename, + return virAuthGetUsernamePath(path, auth, servicename, defaultUsername, hostname); - - VIR_FREE(path); - - return ret; } @@ -230,7 +219,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 +253,6 @@ virAuthGetPasswordPath(const char *path, break; } - VIR_FREE(prompt); - return cred.result; } @@ -277,15 +264,10 @@ virAuthGetPassword(virConnectPtr conn, const char *username, 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; + return virAuthGetPasswordPath(path, auth, servicename, username, hostname); } -- 1.8.3.1
On Sun, Jun 03, 2018 at 01:42:01PM +0530, Sukrit Bhatnagar wrote:
Modify code to use VIR_AUTOFREE macro wherever required.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virauth.c | 60 +++++++++++++++++++----------------------------------- 1 file changed, 21 insertions(+), 39 deletions(-)
diff --git a/src/util/virauth.c b/src/util/virauth.c index adb093e..089a820 100644 --- a/src/util/virauth.c +++ b/src/util/virauth.c @@ -42,10 +42,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 +53,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 +63,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; + }
This should be left untouched (minus the @ret variable), it's actually better not to copy the VIR_DEBUG message on multiple places. The same goes for the hunk below... Erik
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; }
@@ -156,7 +152,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 +189,6 @@ virAuthGetUsernamePath(const char *path, break; }
- VIR_FREE(prompt); - return cred.result; }
@@ -206,18 +200,13 @@ virAuthGetUsername(virConnectPtr conn, const char *defaultUsername, const char *hostname) { - char *ret; - char *path; + VIR_AUTOFREE(char *) path = NULL;
if (virAuthGetConfigFilePath(conn, &path) < 0) return NULL;
- ret = virAuthGetUsernamePath(path, auth, servicename, + return virAuthGetUsernamePath(path, auth, servicename, defaultUsername, hostname); - - VIR_FREE(path); - - return ret; }
@@ -230,7 +219,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 +253,6 @@ virAuthGetPasswordPath(const char *path, break; }
- VIR_FREE(prompt); - return cred.result; }
@@ -277,15 +264,10 @@ virAuthGetPassword(virConnectPtr conn, const char *username, 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; + return virAuthGetPasswordPath(path, auth, servicename, username, hostname); } -- 1.8.3.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Modify code to use VIR_AUTOFREE macro wherever 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
Modify code to use VIR_AUTOFREE macro wherever required. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/iohelper.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/util/iohelper.c b/src/util/iohelper.c index bb8a8dd..f7794dc 100644 --- a/src/util/iohelper.c +++ b/src/util/iohelper.c @@ -46,7 +46,7 @@ static int runIO(const char *path, int fd, int oflags) { - void *base = NULL; /* Location to be freed */ + VIR_AUTOFREE(void *) base = NULL; /* Location to be freed */ char *buf = NULL; /* Aligned location within base */ size_t buflen = 1024*1024; intptr_t alignMask = 64*1024 - 1; @@ -174,8 +174,6 @@ runIO(const char *path, int fd, int oflags) virReportSystemError(errno, _("Unable to close %s"), path); ret = -1; } - - VIR_FREE(base); return ret; } -- 1.8.3.1
Modify code to use VIR_AUTOFREE macro wherever required. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/viraudit.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/viraudit.c b/src/util/viraudit.c index 0085dc3..a49d458 100644 --- a/src/util/viraudit.c +++ b/src/util/viraudit.c @@ -97,7 +97,7 @@ void virAuditSend(virLogSourcePtr source, virAuditRecordType type ATTRIBUTE_UNUSED, bool success, const char *fmt, ...) { - char *str = NULL; + VIR_AUTOFREE(char *) str = NULL; va_list args; /* Duplicate later checks, to short circuit & avoid printf overhead @@ -144,7 +144,6 @@ void virAuditSend(virLogSourcePtr source, } } #endif - VIR_FREE(str); } void virAuditClose(void) -- 1.8.3.1
Modify code to use VIR_AUTOFREE macro wherever required. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virbuffer.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index 3d6defb..5152f73 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -456,7 +456,8 @@ void virBufferEscapeString(virBufferPtr buf, const char *format, const char *str) { int len; - char *escaped, *out; + VIR_AUTOFREE(char *) escaped = NULL; + char *out; const char *cur; const char forbidden_characters[] = { 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, @@ -533,7 +534,6 @@ virBufferEscapeString(virBufferPtr buf, const char *format, const char *str) *out = 0; virBufferAsprintf(buf, format, escaped); - VIR_FREE(escaped); } /** @@ -612,7 +612,8 @@ virBufferEscape(virBufferPtr buf, char escape, const char *toescape, const char *format, const char *str) { int len; - char *escaped, *out; + VIR_AUTOFREE(char *) escaped = NULL; + char *out; const char *cur; if ((format == NULL) || (buf == NULL) || (str == NULL)) @@ -644,7 +645,6 @@ virBufferEscape(virBufferPtr buf, char escape, const char *toescape, *out = 0; virBufferAsprintf(buf, format, escaped); - VIR_FREE(escaped); } @@ -675,11 +675,11 @@ virBufferEscapeN(virBufferPtr buf, { int len; size_t i; - char *escaped = NULL; + VIR_AUTOFREE(char *) escaped = NULL; char *out; const char *cur; struct _virBufferEscapePair escapeItem; - struct _virBufferEscapePair *escapeList = NULL; + VIR_AUTOFREE(struct _virBufferEscapePair *) escapeList = NULL; size_t nescapeList = 0; va_list ap; @@ -696,7 +696,8 @@ virBufferEscapeN(virBufferPtr buf, while ((escapeItem.escape = va_arg(ap, int))) { if (!(escapeItem.toescape = va_arg(ap, char *))) { virBufferSetError(buf, errno); - goto cleanup; + va_end(ap); + return; } if (strcspn(str, escapeItem.toescape) == len) @@ -704,19 +705,22 @@ virBufferEscapeN(virBufferPtr buf, if (VIR_APPEND_ELEMENT_QUIET(escapeList, nescapeList, escapeItem) < 0) { virBufferSetError(buf, errno); - goto cleanup; + va_end(ap); + return; } } if (nescapeList == 0) { virBufferAsprintf(buf, format, str); - goto cleanup; + va_end(ap); + return; } if (xalloc_oversized(2, len) || VIR_ALLOC_N_QUIET(escaped, 2 * len + 1) < 0) { virBufferSetError(buf, errno); - goto cleanup; + va_end(ap); + return; } cur = str; @@ -734,11 +738,6 @@ virBufferEscapeN(virBufferPtr buf, *out = 0; virBufferAsprintf(buf, format, escaped); - - cleanup: - va_end(ap); - VIR_FREE(escapeList); - VIR_FREE(escaped); } @@ -803,7 +802,8 @@ void virBufferEscapeShell(virBufferPtr buf, const char *str) { int len; - char *escaped, *out; + VIR_AUTOFREE(char *) escaped = NULL; + char *out; const char *cur; if ((buf == NULL) || (str == NULL)) @@ -847,7 +847,6 @@ virBufferEscapeShell(virBufferPtr buf, const char *str) *out = 0; virBufferAdd(buf, escaped, -1); - VIR_FREE(escaped); } /** -- 1.8.3.1
On Sun, Jun 03, 2018 at 01:42:05PM +0530, Sukrit Bhatnagar wrote:
Modify code to use VIR_AUTOFREE macro wherever required.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virbuffer.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-)
diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index 3d6defb..5152f73 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -456,7 +456,8 @@ void virBufferEscapeString(virBufferPtr buf, const char *format, const char *str) { int len; - char *escaped, *out; + VIR_AUTOFREE(char *) escaped = NULL; + char *out; const char *cur; const char forbidden_characters[] = { 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, @@ -533,7 +534,6 @@ virBufferEscapeString(virBufferPtr buf, const char *format, const char *str) *out = 0;
virBufferAsprintf(buf, format, escaped); - VIR_FREE(escaped); }
/** @@ -612,7 +612,8 @@ virBufferEscape(virBufferPtr buf, char escape, const char *toescape, const char *format, const char *str) { int len; - char *escaped, *out; + VIR_AUTOFREE(char *) escaped = NULL; + char *out; const char *cur;
if ((format == NULL) || (buf == NULL) || (str == NULL)) @@ -644,7 +645,6 @@ virBufferEscape(virBufferPtr buf, char escape, const char *toescape, *out = 0;
virBufferAsprintf(buf, format, escaped); - VIR_FREE(escaped); }
@@ -675,11 +675,11 @@ virBufferEscapeN(virBufferPtr buf, { int len; size_t i; - char *escaped = NULL; + VIR_AUTOFREE(char *) escaped = NULL; char *out; const char *cur; struct _virBufferEscapePair escapeItem; - struct _virBufferEscapePair *escapeList = NULL; + VIR_AUTOFREE(struct _virBufferEscapePair *) escapeList = NULL;
During the design discussion, I said I was fine with ^this usage and I still kinda am, but at the same time Andrea argued in the same thread [1] that we should stay consistent here in terms that every virSomething should have a corresponding virSomethingFree method even if it ends up just calling VIR_FREE. And he's right in that it could easily turn against us, simply because it can look a bit fishy that we're calling plain VIR_FREE on the structure pointer rather than having a dedicated cleanup for the compound type at first glance, so you have to double check whether it's really okay to use it this way. So unless someone else comes and says that they're absolutely fine with making exceptions like this (although consistency is key to a readable and maintainable code), I'd say let's shoot for consistency, create a static virBufferEscapePairFree helper and declare VIR_AUTOFREE as a macro taking care of automatic freeing of *scalar* resources. [1] https://www.redhat.com/archives/libvir-list/2018-May/msg01910.html
size_t nescapeList = 0; va_list ap;
@@ -696,7 +696,8 @@ virBufferEscapeN(virBufferPtr buf, while ((escapeItem.escape = va_arg(ap, int))) { if (!(escapeItem.toescape = va_arg(ap, char *))) { virBufferSetError(buf, errno); - goto cleanup; + va_end(ap); + return; }
if (strcspn(str, escapeItem.toescape) == len) @@ -704,19 +705,22 @@ virBufferEscapeN(virBufferPtr buf,
if (VIR_APPEND_ELEMENT_QUIET(escapeList, nescapeList, escapeItem) < 0) { virBufferSetError(buf, errno); - goto cleanup; + va_end(ap); + return;
so we basically replaced one goto statement for 2 statements where the va_end statement is very easily to be forgotten... the cleanup label should stay in place here, calling va_end and return since the frees will be taken care of. Erik
} }
if (nescapeList == 0) { virBufferAsprintf(buf, format, str); - goto cleanup; + va_end(ap); + return; }
if (xalloc_oversized(2, len) || VIR_ALLOC_N_QUIET(escaped, 2 * len + 1) < 0) { virBufferSetError(buf, errno); - goto cleanup; + va_end(ap); + return; }
cur = str; @@ -734,11 +738,6 @@ virBufferEscapeN(virBufferPtr buf, *out = 0;
virBufferAsprintf(buf, format, escaped); - - cleanup: - va_end(ap); - VIR_FREE(escapeList); - VIR_FREE(escaped); }
@@ -803,7 +802,8 @@ void virBufferEscapeShell(virBufferPtr buf, const char *str) { int len; - char *escaped, *out; + VIR_AUTOFREE(char *) escaped = NULL; + char *out; const char *cur;
if ((buf == NULL) || (str == NULL)) @@ -847,7 +847,6 @@ virBufferEscapeShell(virBufferPtr buf, const char *str) *out = 0;
virBufferAdd(buf, escaped, -1); - VIR_FREE(escaped); }
/** -- 1.8.3.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Modify code to use VIR_AUTOFREE macro wherever required. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/vircgroup.c | 526 ++++++++++++++++++--------------------------------- 1 file changed, 179 insertions(+), 347 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 0a31947..ed86d31 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -163,7 +163,7 @@ virCgroupPartitionNeedsEscaping(const char *path) { FILE *fp = NULL; int ret = 0; - char *line = NULL; + VIR_AUTOFREE(char *) line = NULL; size_t buflen; /* If it starts with 'cgroup.' or a '_' of any @@ -223,7 +223,6 @@ virCgroupPartitionNeedsEscaping(const char *path) } cleanup: - VIR_FREE(line); VIR_FORCE_FCLOSE(fp); return ret; } @@ -256,41 +255,40 @@ virCgroupValidateMachineGroup(virCgroupPtr group, const char *machinename) { size_t i; - bool valid = false; - char *partname = NULL; - char *scopename_old = NULL; - char *scopename_new = NULL; - char *partmachinename = NULL; + VIR_AUTOFREE(char *) partname = NULL; + VIR_AUTOFREE(char *) scopename_old = NULL; + VIR_AUTOFREE(char *) scopename_new = NULL; + VIR_AUTOFREE(char *) partmachinename = NULL; if (virAsprintf(&partname, "%s.libvirt-%s", name, drivername) < 0) - goto cleanup; + return false; if (virCgroupPartitionEscape(&partname) < 0) - goto cleanup; + return false; if (machinename && (virAsprintf(&partmachinename, "%s.libvirt-%s", machinename, drivername) < 0 || virCgroupPartitionEscape(&partmachinename) < 0)) - goto cleanup; + return false; if (!(scopename_old = virSystemdMakeScopeName(name, drivername, true))) - goto cleanup; + return false; /* We should keep trying even if this failed */ if (!machinename) virResetLastError(); else if (!(scopename_new = virSystemdMakeScopeName(machinename, drivername, false))) - goto cleanup; + return false; if (virCgroupPartitionEscape(&scopename_old) < 0) - goto cleanup; + return false; if (scopename_new && virCgroupPartitionEscape(&scopename_new) < 0) - goto cleanup; + return false; for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { char *tmp; @@ -303,7 +301,7 @@ virCgroupValidateMachineGroup(virCgroupPtr group, tmp = strrchr(group->controllers[i].placement, '/'); if (!tmp) - goto cleanup; + return false; if (stripEmulatorSuffix && (i == VIR_CGROUP_CONTROLLER_CPU || @@ -313,7 +311,7 @@ virCgroupValidateMachineGroup(virCgroupPtr group, *tmp = '\0'; tmp = strrchr(group->controllers[i].placement, '/'); if (!tmp) - goto cleanup; + return false; } tmp++; @@ -329,18 +327,11 @@ virCgroupValidateMachineGroup(virCgroupPtr group, tmp, virCgroupControllerTypeToString(i), name, NULLSTR(machinename), partname, scopename_old, NULLSTR(scopename_new)); - goto cleanup; + return false; } } - valid = true; - - cleanup: - VIR_FREE(partmachinename); - VIR_FREE(partname); - VIR_FREE(scopename_old); - VIR_FREE(scopename_new); - return valid; + return true; } @@ -378,7 +369,7 @@ virCgroupDetectMountsFromFile(virCgroupPtr group, FILE *mounts = NULL; struct mntent entry; char buf[CGROUP_MAX_VAL]; - char *linksrc = NULL; + VIR_AUTOFREE(char *) linksrc = NULL; int ret = -1; mounts = fopen(path, "r"); @@ -468,7 +459,6 @@ virCgroupDetectMountsFromFile(virCgroupPtr group, ret = 0; cleanup: - VIR_FREE(linksrc); VIR_FORCE_FCLOSE(mounts); return ret; } @@ -547,7 +537,7 @@ virCgroupDetectPlacement(virCgroupPtr group, FILE *mapping = NULL; char line[1024]; int ret = -1; - char *procfile; + VIR_AUTOFREE(char *) procfile = NULL; VIR_DEBUG("Detecting placement for pid %lld path %s", (long long) pid, path); @@ -628,9 +618,7 @@ virCgroupDetectPlacement(virCgroupPtr group, ret = 0; cleanup: - VIR_FREE(procfile); VIR_FORCE_FCLOSE(mapping); - return ret; } @@ -786,8 +774,7 @@ virCgroupSetValueStr(virCgroupPtr group, const char *key, const char *value) { - int ret = -1; - char *keypath = NULL; + VIR_AUTOFREE(char *) keypath = NULL; char *tmp = NULL; if (virCgroupPathOfController(group, controller, key, &keypath) < 0) @@ -800,18 +787,14 @@ virCgroupSetValueStr(virCgroupPtr group, virReportSystemError(errno, _("Invalid value '%s' for '%s'"), value, tmp + 1); - goto cleanup; + return -1; } virReportSystemError(errno, _("Unable to write to '%s'"), keypath); - goto cleanup; + return -1; } - ret = 0; - - cleanup: - VIR_FREE(keypath); - return ret; + return 0; } @@ -821,8 +804,8 @@ virCgroupGetValueStr(virCgroupPtr group, const char *key, char **value) { - char *keypath = NULL; - int ret = -1, rc; + VIR_AUTOFREE(char *) keypath = NULL; + int rc; *value = NULL; @@ -834,18 +817,14 @@ virCgroupGetValueStr(virCgroupPtr group, if ((rc = virFileReadAll(keypath, 1024*1024, value)) < 0) { virReportSystemError(errno, _("Unable to read from '%s'"), keypath); - goto cleanup; + return -1; } /* Terminated with '\n' has sometimes harmful effects to the caller */ if (rc > 0 && (*value)[rc - 1] == '\n') (*value)[rc - 1] = '\0'; - ret = 0; - - cleanup: - VIR_FREE(keypath); - return ret; + return 0; } @@ -856,8 +835,8 @@ virCgroupGetValueForBlkDev(virCgroupPtr group, const char *path, char **value) { - char *prefix = NULL; - char *str = NULL; + VIR_AUTOFREE(char *) prefix = NULL; + VIR_AUTOFREE(char *) str = NULL; char **lines = NULL; int ret = -1; @@ -875,8 +854,6 @@ virCgroupGetValueForBlkDev(virCgroupPtr group, ret = 0; error: - VIR_FREE(str); - VIR_FREE(prefix); virStringListFree(lines); return ret; } @@ -888,17 +865,12 @@ virCgroupSetValueU64(virCgroupPtr group, const char *key, unsigned long long int value) { - char *strval = NULL; - int ret; + VIR_AUTOFREE(char *) strval = NULL; if (virAsprintf(&strval, "%llu", value) < 0) return -1; - ret = virCgroupSetValueStr(group, controller, key, strval); - - VIR_FREE(strval); - - return ret; + return virCgroupSetValueStr(group, controller, key, strval); } @@ -908,17 +880,12 @@ virCgroupSetValueI64(virCgroupPtr group, const char *key, long long int value) { - char *strval = NULL; - int ret; + VIR_AUTOFREE(char *) strval = NULL; if (virAsprintf(&strval, "%lld", value) < 0) return -1; - ret = virCgroupSetValueStr(group, controller, key, strval); - - VIR_FREE(strval); - - return ret; + return virCgroupSetValueStr(group, controller, key, strval); } @@ -928,24 +895,19 @@ virCgroupGetValueI64(virCgroupPtr group, const char *key, long long int *value) { - char *strval = NULL; - int ret = -1; + VIR_AUTOFREE(char *) strval = NULL; if (virCgroupGetValueStr(group, controller, key, &strval) < 0) - goto cleanup; + return -1; if (virStrToLong_ll(strval, NULL, 10, value) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse '%s' as an integer"), strval); - goto cleanup; + return -1; } - ret = 0; - - cleanup: - VIR_FREE(strval); - return ret; + return 0; } @@ -955,24 +917,19 @@ virCgroupGetValueU64(virCgroupPtr group, const char *key, unsigned long long int *value) { - char *strval = NULL; - int ret = -1; + VIR_AUTOFREE(char *) strval = NULL; if (virCgroupGetValueStr(group, controller, key, &strval) < 0) - goto cleanup; + return -1; if (virStrToLong_ull(strval, NULL, 10, value) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse '%s' as an integer"), strval); - goto cleanup; + return -1; } - ret = 0; - - cleanup: - VIR_FREE(strval); - return ret; + return 0; } @@ -988,7 +945,7 @@ virCgroupCpuSetInherit(virCgroupPtr parent, virCgroupPtr group) VIR_DEBUG("Setting up inheritance %s -> %s", parent->path, group->path); for (i = 0; i < ARRAY_CARDINALITY(inherit_values); i++) { - char *value; + VIR_AUTOFREE(char *) value = NULL; if (virCgroupGetValueStr(parent, VIR_CGROUP_CONTROLLER_CPUSET, @@ -1001,11 +958,8 @@ virCgroupCpuSetInherit(virCgroupPtr parent, virCgroupPtr group) if (virCgroupSetValueStr(group, VIR_CGROUP_CONTROLLER_CPUSET, inherit_values[i], - value) < 0) { - VIR_FREE(value); + value) < 0) return -1; - } - VIR_FREE(value); } return 0; @@ -1044,11 +998,10 @@ virCgroupMakeGroup(virCgroupPtr parent, unsigned int flags) { size_t i; - int ret = -1; VIR_DEBUG("Make group %s", group->path); for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { - char *path = NULL; + VIR_AUTOFREE(char *) path = NULL; /* We must never mkdir() in systemd's hierarchy */ if (i == VIR_CGROUP_CONTROLLER_SYSTEMD) { @@ -1074,10 +1027,8 @@ virCgroupMakeGroup(virCgroupPtr parent, if (!virFileExists(path)) { if (!create || mkdir(path, 0755) < 0) { - if (errno == EEXIST) { - VIR_FREE(path); + if (errno == EEXIST) continue; - } /* With a kernel that doesn't support multi-level directory * for blkio controller, libvirt will fail and disable all * other controllers even though they are available. So @@ -1085,24 +1036,20 @@ virCgroupMakeGroup(virCgroupPtr parent, if (i == VIR_CGROUP_CONTROLLER_BLKIO) { VIR_DEBUG("Ignoring mkdir failure with blkio controller. Kernel probably too old"); VIR_FREE(group->controllers[i].mountPoint); - VIR_FREE(path); continue; } else { virReportSystemError(errno, _("Failed to create controller %s for group"), virCgroupControllerTypeToString(i)); - VIR_FREE(path); - goto cleanup; + return -1; } } if (group->controllers[VIR_CGROUP_CONTROLLER_CPUSET].mountPoint != NULL && (i == VIR_CGROUP_CONTROLLER_CPUSET || STREQ(group->controllers[i].mountPoint, group->controllers[VIR_CGROUP_CONTROLLER_CPUSET].mountPoint))) { - if (virCgroupCpuSetInherit(parent, group) < 0) { - VIR_FREE(path); - goto cleanup; - } + if (virCgroupCpuSetInherit(parent, group) < 0) + return -1; } /* * Note that virCgroupSetMemoryUseHierarchy should always be @@ -1113,21 +1060,14 @@ virCgroupMakeGroup(virCgroupPtr parent, (i == VIR_CGROUP_CONTROLLER_MEMORY || STREQ(group->controllers[i].mountPoint, group->controllers[VIR_CGROUP_CONTROLLER_MEMORY].mountPoint))) { - if (virCgroupSetMemoryUseHierarchy(group) < 0) { - VIR_FREE(path); - goto cleanup; - } + if (virCgroupSetMemoryUseHierarchy(group) < 0) + return -1; } } - - VIR_FREE(path); } VIR_DEBUG("Done making controllers for group"); - ret = 0; - - cleanup: - return ret; + return 0; } @@ -1339,9 +1279,9 @@ virCgroupNewPartition(const char *path, virCgroupPtr *group) { int ret = -1; - char *parentPath = NULL; + VIR_AUTOFREE(char *) parentPath = NULL; virCgroupPtr parent = NULL; - char *newPath = NULL; + VIR_AUTOFREE(char *) newPath = NULL; VIR_DEBUG("path=%s create=%d controllers=%x", path, create, controllers); @@ -1381,8 +1321,6 @@ virCgroupNewPartition(const char *path, if (ret != 0) virCgroupFree(group); virCgroupFree(&parent); - VIR_FREE(parentPath); - VIR_FREE(newPath); return ret; } @@ -1421,18 +1359,17 @@ virCgroupNewDomainPartition(virCgroupPtr partition, bool create, virCgroupPtr *group) { - int ret = -1; - char *grpname = NULL; + VIR_AUTOFREE(char *)grpname = NULL; if (virAsprintf(&grpname, "%s.libvirt-%s", name, driver) < 0) - goto cleanup; + return -1; if (virCgroupPartitionEscape(&grpname) < 0) - goto cleanup; + return -1; if (virCgroupNew(-1, grpname, partition, -1, group) < 0) - goto cleanup; + return -1; /* * Create a cgroup with memory.use_hierarchy enabled to @@ -1448,14 +1385,10 @@ virCgroupNewDomainPartition(virCgroupPtr partition, VIR_CGROUP_MEM_HIERACHY) < 0) { virCgroupRemove(*group); virCgroupFree(group); - goto cleanup; + return -1; } - ret = 0; - - cleanup: - VIR_FREE(grpname); - return ret; + return 0; } @@ -1477,27 +1410,26 @@ virCgroupNewThread(virCgroupPtr domain, bool create, virCgroupPtr *group) { - int ret = -1; - char *name = NULL; + VIR_AUTOFREE(char *) name = NULL; int controllers; switch (nameval) { case VIR_CGROUP_THREAD_VCPU: if (virAsprintf(&name, "vcpu%d", id) < 0) - goto cleanup; + return -1; break; case VIR_CGROUP_THREAD_EMULATOR: if (VIR_STRDUP(name, "emulator") < 0) - goto cleanup; + return -1; break; case VIR_CGROUP_THREAD_IOTHREAD: if (virAsprintf(&name, "iothread%d", id) < 0) - goto cleanup; + return -1; break; case VIR_CGROUP_THREAD_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected name value %d"), nameval); - goto cleanup; + return -1; } controllers = ((1 << VIR_CGROUP_CONTROLLER_CPU) | @@ -1505,18 +1437,15 @@ virCgroupNewThread(virCgroupPtr domain, (1 << VIR_CGROUP_CONTROLLER_CPUSET)); if (virCgroupNew(-1, name, domain, controllers, group) < 0) - goto cleanup; + return -1; if (virCgroupMakeGroup(domain, *group, create, VIR_CGROUP_NONE) < 0) { virCgroupRemove(*group); virCgroupFree(group); - goto cleanup; + return -1; } - ret = 0; - cleanup: - VIR_FREE(name); - return ret; + return 0; } @@ -1577,7 +1506,7 @@ virCgroupNewMachineSystemd(const char *name, int ret = -1; int rv; virCgroupPtr init, parent = NULL; - char *path = NULL; + VIR_AUTOFREE(char *) path = NULL; char *offset; VIR_DEBUG("Trying to setup machine '%s' via systemd", name); @@ -1662,7 +1591,6 @@ virCgroupNewMachineSystemd(const char *name, ret = 0; cleanup: virCgroupFree(&parent); - VIR_FREE(path); return ret; } @@ -1894,9 +1822,10 @@ virCgroupGetBlkioIoServiced(virCgroupPtr group, long long *requests_write) { long long stats_val; - char *str1 = NULL, *str2 = NULL, *p1, *p2; + VIR_AUTOFREE(char *) str1 = NULL; + VIR_AUTOFREE(char *) str2 = NULL; + char *p1, *p2; size_t i; - int ret = -1; const char *value_names[] = { "Read ", @@ -1919,12 +1848,12 @@ virCgroupGetBlkioIoServiced(virCgroupPtr group, if (virCgroupGetValueStr(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.io_service_bytes", &str1) < 0) - goto cleanup; + return -1; if (virCgroupGetValueStr(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.io_serviced", &str2) < 0) - goto cleanup; + return -1; /* sum up all entries of the same kind, from all devices */ for (i = 0; i < ARRAY_CARDINALITY(value_names); i++) { @@ -1938,7 +1867,7 @@ virCgroupGetBlkioIoServiced(virCgroupPtr group, _("Cannot parse byte %sstat '%s'"), value_names[i], p1); - goto cleanup; + return -1; } if (stats_val < 0 || @@ -1947,7 +1876,7 @@ virCgroupGetBlkioIoServiced(virCgroupPtr group, virReportError(VIR_ERR_OVERFLOW, _("Sum of byte %sstat overflows"), value_names[i]); - goto cleanup; + return -1; } *bytes_ptrs[i] += stats_val; } @@ -1959,7 +1888,7 @@ virCgroupGetBlkioIoServiced(virCgroupPtr group, _("Cannot parse %srequest stat '%s'"), value_names[i], p2); - goto cleanup; + return -1; } if (stats_val < 0 || @@ -1968,18 +1897,13 @@ virCgroupGetBlkioIoServiced(virCgroupPtr group, virReportError(VIR_ERR_OVERFLOW, _("Sum of %srequest stat overflows"), value_names[i]); - goto cleanup; + return -1; } *requests_ptrs[i] += stats_val; } } - ret = 0; - - cleanup: - VIR_FREE(str2); - VIR_FREE(str1); - return ret; + return 0; } @@ -2003,9 +1927,11 @@ virCgroupGetBlkioIoDeviceServiced(virCgroupPtr group, long long *requests_read, long long *requests_write) { - char *str1 = NULL, *str2 = NULL, *str3 = NULL, *p1, *p2; + VIR_AUTOFREE(char *) str1 = NULL; + VIR_AUTOFREE(char *) str2 = NULL; + VIR_AUTOFREE(char *) str3 = NULL; + char *p1, *p2; size_t i; - int ret = -1; const char *value_names[] = { "Read ", @@ -2023,28 +1949,28 @@ virCgroupGetBlkioIoDeviceServiced(virCgroupPtr group, if (virCgroupGetValueStr(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.io_service_bytes", &str1) < 0) - goto cleanup; + return -1; if (virCgroupGetValueStr(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.io_serviced", &str2) < 0) - goto cleanup; + return -1; if (!(str3 = virCgroupGetBlockDevString(path))) - goto cleanup; + return -1; if (!(p1 = strstr(str1, str3))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot find byte stats for block device '%s'"), str3); - goto cleanup; + return -1; } if (!(p2 = strstr(str2, str3))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot find request stats for block device '%s'"), str3); - goto cleanup; + return -1; } for (i = 0; i < ARRAY_CARDINALITY(value_names); i++) { @@ -2052,38 +1978,32 @@ virCgroupGetBlkioIoDeviceServiced(virCgroupPtr group, virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot find byte %sstats for block device '%s'"), value_names[i], str3); - goto cleanup; + return -1; } if (virStrToLong_ll(p1 + strlen(value_names[i]), &p1, 10, bytes_ptrs[i]) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot parse %sstat '%s'"), value_names[i], p1 + strlen(value_names[i])); - goto cleanup; + return -1; } if (!(p2 = strstr(p2, value_names[i]))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot find request %sstats for block device '%s'"), value_names[i], str3); - goto cleanup; + return -1; } if (virStrToLong_ll(p2 + strlen(value_names[i]), &p2, 10, requests_ptrs[i]) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot parse %sstat '%s'"), value_names[i], p2 + strlen(value_names[i])); - goto cleanup; + return -1; } } - ret = 0; - - cleanup: - VIR_FREE(str3); - VIR_FREE(str2); - VIR_FREE(str1); - return ret; + return 0; } @@ -2139,24 +2059,19 @@ virCgroupSetBlkioDeviceReadIops(virCgroupPtr group, const char *path, unsigned int riops) { - char *str = NULL; - char *blkstr = NULL; - int ret = -1; + VIR_AUTOFREE(char *) str = NULL; + VIR_AUTOFREE(char *) blkstr = NULL; if (!(blkstr = virCgroupGetBlockDevString(path))) return -1; if (virAsprintf(&str, "%s%u", blkstr, riops) < 0) - goto error; + return -1; - ret = virCgroupSetValueStr(group, + return virCgroupSetValueStr(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.read_iops_device", str); - error: - VIR_FREE(blkstr); - VIR_FREE(str); - return ret; } @@ -2173,24 +2088,19 @@ virCgroupSetBlkioDeviceWriteIops(virCgroupPtr group, const char *path, unsigned int wiops) { - char *str = NULL; - char *blkstr = NULL; - int ret = -1; + VIR_AUTOFREE(char *) str = NULL; + VIR_AUTOFREE(char *) blkstr = NULL; if (!(blkstr = virCgroupGetBlockDevString(path))) return -1; if (virAsprintf(&str, "%s%u", blkstr, wiops) < 0) - goto error; + return -1; - ret = virCgroupSetValueStr(group, + return virCgroupSetValueStr(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.write_iops_device", str); - error: - VIR_FREE(blkstr); - VIR_FREE(str); - return ret; } @@ -2207,24 +2117,19 @@ virCgroupSetBlkioDeviceReadBps(virCgroupPtr group, const char *path, unsigned long long rbps) { - char *str = NULL; - char *blkstr = NULL; - int ret = -1; + VIR_AUTOFREE(char *) str = NULL; + VIR_AUTOFREE(char *) blkstr = NULL; if (!(blkstr = virCgroupGetBlockDevString(path))) return -1; if (virAsprintf(&str, "%s%llu", blkstr, rbps) < 0) - goto error; + return -1; - ret = virCgroupSetValueStr(group, + return virCgroupSetValueStr(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.read_bps_device", str); - error: - VIR_FREE(blkstr); - VIR_FREE(str); - return ret; } /** @@ -2240,24 +2145,19 @@ virCgroupSetBlkioDeviceWriteBps(virCgroupPtr group, const char *path, unsigned long long wbps) { - char *str = NULL; - char *blkstr = NULL; - int ret = -1; + VIR_AUTOFREE(char *) str = NULL; + VIR_AUTOFREE(char *) blkstr = NULL; if (!(blkstr = virCgroupGetBlockDevString(path))) return -1; if (virAsprintf(&str, "%s%llu", blkstr, wbps) < 0) - goto error; + return -1; - ret = virCgroupSetValueStr(group, + return virCgroupSetValueStr(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.write_bps_device", str); - error: - VIR_FREE(blkstr); - VIR_FREE(str); - return ret; } @@ -2275,24 +2175,19 @@ virCgroupSetBlkioDeviceWeight(virCgroupPtr group, const char *path, unsigned int weight) { - char *str = NULL; - char *blkstr = NULL; - int ret = -1; + VIR_AUTOFREE(char *) str = NULL; + VIR_AUTOFREE(char *) blkstr = NULL; if (!(blkstr = virCgroupGetBlockDevString(path))) return -1; if (virAsprintf(&str, "%s%d", blkstr, weight) < 0) - goto error; + return -1; - ret = virCgroupSetValueStr(group, + return virCgroupSetValueStr(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.weight_device", str); - error: - VIR_FREE(blkstr); - VIR_FREE(str); - return ret; } /** @@ -2308,15 +2203,14 @@ virCgroupGetBlkioDeviceReadIops(virCgroupPtr group, const char *path, unsigned int *riops) { - char *str = NULL; - int ret = -1; + VIR_AUTOFREE(char *) str = NULL; if (virCgroupGetValueForBlkDev(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.read_iops_device", path, &str) < 0) - goto error; + return -1; if (!str) { *riops = 0; @@ -2324,13 +2218,10 @@ virCgroupGetBlkioDeviceReadIops(virCgroupPtr group, virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse '%s' as an integer"), str); - goto error; + return -1; } - ret = 0; - error: - VIR_FREE(str); - return ret; + return 0; } /** @@ -2346,15 +2237,14 @@ virCgroupGetBlkioDeviceWriteIops(virCgroupPtr group, const char *path, unsigned int *wiops) { - char *str = NULL; - int ret = -1; + VIR_AUTOFREE(char *) str = NULL; if (virCgroupGetValueForBlkDev(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.write_iops_device", path, &str) < 0) - goto error; + return -1; if (!str) { *wiops = 0; @@ -2362,13 +2252,10 @@ virCgroupGetBlkioDeviceWriteIops(virCgroupPtr group, virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse '%s' as an integer"), str); - goto error; + return -1; } - ret = 0; - error: - VIR_FREE(str); - return ret; + return 0; } /** @@ -2384,15 +2271,14 @@ virCgroupGetBlkioDeviceReadBps(virCgroupPtr group, const char *path, unsigned long long *rbps) { - char *str = NULL; - int ret = -1; + VIR_AUTOFREE(char *) str = NULL; if (virCgroupGetValueForBlkDev(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.read_bps_device", path, &str) < 0) - goto error; + return -1; if (!str) { *rbps = 0; @@ -2400,13 +2286,10 @@ virCgroupGetBlkioDeviceReadBps(virCgroupPtr group, virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse '%s' as an integer"), str); - goto error; + return -1; } - ret = 0; - error: - VIR_FREE(str); - return ret; + return 0; } /** @@ -2422,15 +2305,14 @@ virCgroupGetBlkioDeviceWriteBps(virCgroupPtr group, const char *path, unsigned long long *wbps) { - char *str = NULL; - int ret = -1; + VIR_AUTOFREE(char *) str = NULL; if (virCgroupGetValueForBlkDev(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.write_bps_device", path, &str) < 0) - goto error; + return -1; if (!str) { *wbps = 0; @@ -2438,13 +2320,10 @@ virCgroupGetBlkioDeviceWriteBps(virCgroupPtr group, virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse '%s' as an integer"), str); - goto error; + return -1; } - ret = 0; - error: - VIR_FREE(str); - return ret; + return 0; } /** @@ -2460,15 +2339,14 @@ virCgroupGetBlkioDeviceWeight(virCgroupPtr group, const char *path, unsigned int *weight) { - char *str = NULL; - int ret = -1; + VIR_AUTOFREE(char *) str = NULL; if (virCgroupGetValueForBlkDev(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.weight_device", path, &str) < 0) - goto error; + return -1; if (!str) { *weight = 0; @@ -2476,13 +2354,10 @@ virCgroupGetBlkioDeviceWeight(virCgroupPtr group, virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse '%s' as an integer"), str); - goto error; + return -1; } - ret = 0; - error: - VIR_FREE(str); - return ret; + return 0; } @@ -2941,36 +2816,29 @@ int virCgroupAllowDevice(virCgroupPtr group, char type, int major, int minor, int perms) { - int ret = -1; - char *devstr = NULL; - char *majorstr = NULL; - char *minorstr = NULL; + VIR_AUTOFREE(char *) devstr = NULL; + VIR_AUTOFREE(char *) majorstr = NULL; + VIR_AUTOFREE(char *) minorstr = NULL; if ((major < 0 && VIR_STRDUP(majorstr, "*") < 0) || (major >= 0 && virAsprintf(&majorstr, "%i", major) < 0)) - goto cleanup; + return -1; if ((minor < 0 && VIR_STRDUP(minorstr, "*") < 0) || (minor >= 0 && virAsprintf(&minorstr, "%i", minor) < 0)) - goto cleanup; + return -1; if (virAsprintf(&devstr, "%c %s:%s %s", type, majorstr, minorstr, virCgroupGetDevicePermsString(perms)) < 0) - goto cleanup; + return -1; if (virCgroupSetValueStr(group, VIR_CGROUP_CONTROLLER_DEVICES, "devices.allow", devstr) < 0) - goto cleanup; - - ret = 0; + return -1; - cleanup: - VIR_FREE(devstr); - VIR_FREE(majorstr); - VIR_FREE(minorstr); - return ret; + return 0; } @@ -3032,36 +2900,29 @@ int virCgroupDenyDevice(virCgroupPtr group, char type, int major, int minor, int perms) { - int ret = -1; - char *devstr = NULL; - char *majorstr = NULL; - char *minorstr = NULL; + VIR_AUTOFREE(char *) devstr = NULL; + VIR_AUTOFREE(char *) majorstr = NULL; + VIR_AUTOFREE(char *) minorstr = NULL; if ((major < 0 && VIR_STRDUP(majorstr, "*") < 0) || (major >= 0 && virAsprintf(&majorstr, "%i", major) < 0)) - goto cleanup; + return -1; if ((minor < 0 && VIR_STRDUP(minorstr, "*") < 0) || (minor >= 0 && virAsprintf(&minorstr, "%i", minor) < 0)) - goto cleanup; + return -1; if (virAsprintf(&devstr, "%c %s:%s %s", type, majorstr, minorstr, virCgroupGetDevicePermsString(perms)) < 0) - goto cleanup; + return -1; if (virCgroupSetValueStr(group, VIR_CGROUP_CONTROLLER_DEVICES, "devices.deny", devstr) < 0) - goto cleanup; - - ret = 0; + return -1; - cleanup: - VIR_FREE(devstr); - VIR_FREE(majorstr); - VIR_FREE(minorstr); - return ret; + return 0; } @@ -3131,10 +2992,10 @@ virCgroupGetPercpuVcpuSum(virCgroupPtr group, { int ret = -1; ssize_t i = -1; - char *buf = NULL; virCgroupPtr group_vcpu = NULL; while ((i = virBitmapNextSetBit(guestvcpus, i)) >= 0) { + VIR_AUTOFREE(char *) buf = NULL; char *pos; unsigned long long tmp; ssize_t j; @@ -3159,13 +3020,11 @@ virCgroupGetPercpuVcpuSum(virCgroupPtr group, } virCgroupFree(&group_vcpu); - VIR_FREE(buf); } ret = 0; cleanup: virCgroupFree(&group_vcpu); - VIR_FREE(buf); return ret; } @@ -3202,8 +3061,8 @@ virCgroupGetPercpuStats(virCgroupPtr group, size_t i; int need_cpus, total_cpus; char *pos; - char *buf = NULL; - unsigned long long *sum_cpu_time = NULL; + VIR_AUTOFREE(char *) buf = NULL; + VIR_AUTOFREE(unsigned long long *) sum_cpu_time = NULL; virTypedParameterPtr ent; int param_idx; unsigned long long cpu_time; @@ -3289,8 +3148,6 @@ virCgroupGetPercpuStats(virCgroupPtr group, cleanup: virBitmapFree(cpumap); - VIR_FREE(sum_cpu_time); - VIR_FREE(buf); return ret; } @@ -3461,7 +3318,7 @@ virCgroupRemoveRecursively(char *grppath) /* This is best-effort cleanup: we want to log failures with just * VIR_ERROR instead of normal virReportError */ while ((direrr = virDirRead(grpdir, &ent, NULL)) > 0) { - char *path; + VIR_AUTOFREE(char *) path = NULL; if (ent->d_type != DT_DIR) continue; @@ -3470,7 +3327,6 @@ virCgroupRemoveRecursively(char *grppath) break; } rc = virCgroupRemoveRecursively(path); - VIR_FREE(path); if (rc != 0) break; } @@ -3508,10 +3364,11 @@ virCgroupRemove(virCgroupPtr group) { int rc = 0; size_t i; - char *grppath = NULL; VIR_DEBUG("Removing cgroup %s", group->path); for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { + VIR_AUTOFREE(char *) grppath = NULL; + /* Skip over controllers not mounted */ if (!group->controllers[i].mountPoint) continue; @@ -3533,7 +3390,6 @@ virCgroupRemove(virCgroupPtr group) VIR_DEBUG("Removing cgroup %s and all child cgroups", grppath); rc = virCgroupRemoveRecursively(grppath); - VIR_FREE(grppath); } VIR_DEBUG("Done removing cgroup %s", group->path); @@ -3549,7 +3405,7 @@ virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr pids) { int ret = -1; bool killedAny = false; - char *keypath = NULL; + VIR_AUTOFREE(char *) keypath = NULL; bool done = false; FILE *fp = NULL; VIR_DEBUG("group=%p path=%s signum=%d pids=%p", @@ -3613,7 +3469,6 @@ virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr pids) ret = killedAny ? 1 : 0; cleanup: - VIR_FREE(keypath); VIR_FORCE_FCLOSE(fp); return ret; @@ -3678,7 +3533,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, int ret = -1; int rc; bool killedAny = false; - char *keypath = NULL; + VIR_AUTOFREE(char *) keypath = NULL; DIR *dp = NULL; virCgroupPtr subgroup = NULL; struct dirent *ent; @@ -3732,7 +3587,6 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, cleanup: virCgroupFree(&subgroup); - VIR_FREE(keypath); VIR_DIR_CLOSE(dp); return ret; } @@ -3846,9 +3700,8 @@ int virCgroupGetCpuacctStat(virCgroupPtr group, unsigned long long *user, unsigned long long *sys) { - char *str; + VIR_AUTOFREE(char *) str = NULL; char *p; - int ret = -1; static double scale = -1.0; if (virCgroupGetValueStr(group, VIR_CGROUP_CONTROLLER_CPUACCT, @@ -3860,14 +3713,14 @@ virCgroupGetCpuacctStat(virCgroupPtr group, unsigned long long *user, virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot parse user stat '%s'"), p); - goto cleanup; + return -1; } if (!(p = STRSKIP(p, "\nsystem ")) || virStrToLong_ull(p, NULL, 10, sys) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot parse sys stat '%s'"), p); - goto cleanup; + return -1; } /* times reported are in system ticks (generally 100 Hz), but that * rate can theoretically vary between machines. Scale things @@ -3877,17 +3730,14 @@ virCgroupGetCpuacctStat(virCgroupPtr group, unsigned long long *user, if (ticks_per_sec == -1) { virReportSystemError(errno, "%s", _("Cannot determine system clock HZ")); - goto cleanup; + return -1; } scale = 1000000000.0 / ticks_per_sec; } *user *= scale; *sys *= scale; - ret = 0; - cleanup: - VIR_FREE(str); - return ret; + return 0; } @@ -3913,10 +3763,9 @@ int virCgroupBindMount(virCgroupPtr group, const char *oldroot, const char *mountopts) { - int ret = -1; size_t i; - char *opts = NULL; - char *root = NULL; + VIR_AUTOFREE(char *) opts = NULL; + VIR_AUTOFREE(char *) root = NULL; if (!(root = virCgroupIdentifyRoot(group))) return -1; @@ -3927,18 +3776,18 @@ virCgroupBindMount(virCgroupPtr group, const char *oldroot, virReportSystemError(errno, _("Unable to create directory %s"), root); - goto cleanup; + return -1; } if (virAsprintf(&opts, "mode=755,size=65536%s", mountopts) < 0) - goto cleanup; + return -1; if (mount("tmpfs", root, "tmpfs", MS_NOSUID|MS_NODEV|MS_NOEXEC, opts) < 0) { virReportSystemError(errno, _("Failed to mount %s on %s type %s"), "tmpfs", root, "tmpfs"); - goto cleanup; + return -1; } for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { @@ -3946,11 +3795,11 @@ virCgroupBindMount(virCgroupPtr group, const char *oldroot, continue; if (!virFileExists(group->controllers[i].mountPoint)) { - char *src; + VIR_AUTOFREE(char *) src = NULL; if (virAsprintf(&src, "%s%s", oldroot, group->controllers[i].mountPoint) < 0) - goto cleanup; + return -1; VIR_DEBUG("Create mount point '%s'", group->controllers[i].mountPoint); @@ -3958,8 +3807,7 @@ virCgroupBindMount(virCgroupPtr group, const char *oldroot, virReportSystemError(errno, _("Unable to create directory %s"), group->controllers[i].mountPoint); - VIR_FREE(src); - goto cleanup; + return -1; } if (mount(src, group->controllers[i].mountPoint, NULL, MS_BIND, @@ -3967,11 +3815,8 @@ virCgroupBindMount(virCgroupPtr group, const char *oldroot, virReportSystemError(errno, _("Failed to bind cgroup '%s' on '%s'"), src, group->controllers[i].mountPoint); - VIR_FREE(src); - goto cleanup; + return -1; } - - VIR_FREE(src); } if (group->controllers[i].linkPoint) { @@ -3984,16 +3829,12 @@ virCgroupBindMount(virCgroupPtr group, const char *oldroot, _("Unable to symlink directory %s to %s"), group->controllers[i].mountPoint, group->controllers[i].linkPoint); - goto cleanup; + return -1; } } } - ret = 0; - cleanup: - VIR_FREE(root); - VIR_FREE(opts); - return ret; + return 0; } @@ -4004,11 +3845,11 @@ int virCgroupSetOwner(virCgroupPtr cgroup, { int ret = -1; size_t i; - char *base = NULL, *entry = NULL; DIR *dh = NULL; int direrr; for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { + VIR_AUTOFREE(char *) base = NULL; struct dirent *de; if (!((1 << i) & controllers)) @@ -4025,6 +3866,8 @@ int virCgroupSetOwner(virCgroupPtr cgroup, goto cleanup; while ((direrr = virDirRead(dh, &de, base)) > 0) { + VIR_AUTOFREE(char *) entry = NULL; + if (virAsprintf(&entry, "%s/%s", base, de->d_name) < 0) goto cleanup; @@ -4034,8 +3877,6 @@ int virCgroupSetOwner(virCgroupPtr cgroup, entry, uid, gid); goto cleanup; } - - VIR_FREE(entry); } if (direrr < 0) goto cleanup; @@ -4047,7 +3888,6 @@ int virCgroupSetOwner(virCgroupPtr cgroup, goto cleanup; } - VIR_FREE(base); VIR_DIR_CLOSE(dh); } @@ -4055,8 +3895,6 @@ int virCgroupSetOwner(virCgroupPtr cgroup, cleanup: VIR_DIR_CLOSE(dh); - VIR_FREE(entry); - VIR_FREE(base); return ret; } @@ -4071,8 +3909,7 @@ int virCgroupSetOwner(virCgroupPtr cgroup, bool virCgroupSupportsCpuBW(virCgroupPtr cgroup) { - char *path = NULL; - bool ret = false; + VIR_AUTOFREE(char *) path = NULL; if (!cgroup) return false; @@ -4080,21 +3917,17 @@ virCgroupSupportsCpuBW(virCgroupPtr cgroup) if (virCgroupPathOfController(cgroup, VIR_CGROUP_CONTROLLER_CPU, "cpu.cfs_period_us", &path) < 0) { virResetLastError(); - goto cleanup; + return false; } - ret = virFileExists(path); - - cleanup: - VIR_FREE(path); - return ret; + return virFileExists(path); } int virCgroupHasEmptyTasks(virCgroupPtr cgroup, int controller) { - int ret = -1; - char *content = NULL; + int ret; + VIR_AUTOFREE(char *) content = NULL; if (!cgroup) return -1; @@ -4104,7 +3937,6 @@ virCgroupHasEmptyTasks(virCgroupPtr cgroup, int controller) if (ret == 0 && content[0] == '\0') ret = 1; - VIR_FREE(content); return ret; } -- 1.8.3.1
On Sun, Jun 03, 2018 at 01:42:06PM +0530, Sukrit Bhatnagar wrote:
Modify code to use VIR_AUTOFREE macro wherever required.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/vircgroup.c | 526 ++++++++++++++++++--------------------------------- 1 file changed, 179 insertions(+), 347 deletions(-)
...
@@ -378,7 +369,7 @@ virCgroupDetectMountsFromFile(virCgroupPtr group, FILE *mounts = NULL; struct mntent entry; char buf[CGROUP_MAX_VAL]; - char *linksrc = NULL; + VIR_AUTOFREE(char *) linksrc = NULL;
@linksrc is being used only in an 'if' block further in the function body (the context of which is not seen in the patch), so I'd move the declaration to that block which will allow us to ditch the VIR_FREE(linksrc) in the same block. ...
@@ -1339,9 +1279,9 @@ virCgroupNewPartition(const char *path, virCgroupPtr *group) { int ret = -1; - char *parentPath = NULL; + VIR_AUTOFREE(char *) parentPath = NULL; virCgroupPtr parent = NULL; - char *newPath = NULL; + VIR_AUTOFREE(char *) newPath = NULL;
move ^this one line up, so the AUTOFREEs are nicely packed...
VIR_DEBUG("path=%s create=%d controllers=%x", path, create, controllers);
@@ -1381,8 +1321,6 @@ virCgroupNewPartition(const char *path, if (ret != 0) virCgroupFree(group); virCgroupFree(&parent); - VIR_FREE(parentPath); - VIR_FREE(newPath); return ret; }
...
@@ -1577,7 +1506,7 @@ virCgroupNewMachineSystemd(const char *name, int ret = -1; int rv; virCgroupPtr init, parent = NULL; - char *path = NULL; + VIR_AUTOFREE(char *) path = NULL; char *offset;
VIR_DEBUG("Trying to setup machine '%s' via systemd", name); @@ -1662,7 +1591,6 @@ virCgroupNewMachineSystemd(const char *name, ret = 0; cleanup: virCgroupFree(&parent); - VIR_FREE(path); return ret; }
@@ -1894,9 +1822,10 @@ virCgroupGetBlkioIoServiced(virCgroupPtr group, long long *requests_write) { long long stats_val; - char *str1 = NULL, *str2 = NULL, *p1, *p2; + VIR_AUTOFREE(char *) str1 = NULL; + VIR_AUTOFREE(char *) str2 = NULL; + char *p1, *p2;
since you're changing it already, p1 and p2 should be on separate lines...
size_t i; - int ret = -1;
const char *value_names[] = { "Read ", @@ -1919,12 +1848,12 @@ virCgroupGetBlkioIoServiced(virCgroupPtr group, if (virCgroupGetValueStr(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.io_service_bytes", &str1) < 0) - goto cleanup; + return -1;
if (virCgroupGetValueStr(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.io_serviced", &str2) < 0) - goto cleanup; + return -1;
/* sum up all entries of the same kind, from all devices */ for (i = 0; i < ARRAY_CARDINALITY(value_names); i++) { @@ -1938,7 +1867,7 @@ virCgroupGetBlkioIoServiced(virCgroupPtr group, _("Cannot parse byte %sstat '%s'"), value_names[i], p1); - goto cleanup; + return -1; }
if (stats_val < 0 || @@ -1947,7 +1876,7 @@ virCgroupGetBlkioIoServiced(virCgroupPtr group, virReportError(VIR_ERR_OVERFLOW, _("Sum of byte %sstat overflows"), value_names[i]); - goto cleanup; + return -1; } *bytes_ptrs[i] += stats_val; } @@ -1959,7 +1888,7 @@ virCgroupGetBlkioIoServiced(virCgroupPtr group, _("Cannot parse %srequest stat '%s'"), value_names[i], p2); - goto cleanup; + return -1; }
if (stats_val < 0 || @@ -1968,18 +1897,13 @@ virCgroupGetBlkioIoServiced(virCgroupPtr group, virReportError(VIR_ERR_OVERFLOW, _("Sum of %srequest stat overflows"), value_names[i]); - goto cleanup; + return -1; } *requests_ptrs[i] += stats_val; } }
- ret = 0; - - cleanup: - VIR_FREE(str2); - VIR_FREE(str1); - return ret; + return 0; }
@@ -2003,9 +1927,11 @@ virCgroupGetBlkioIoDeviceServiced(virCgroupPtr group, long long *requests_read, long long *requests_write) { - char *str1 = NULL, *str2 = NULL, *str3 = NULL, *p1, *p2; + VIR_AUTOFREE(char *) str1 = NULL; + VIR_AUTOFREE(char *) str2 = NULL; + VIR_AUTOFREE(char *) str3 = NULL; + char *p1, *p2;
here too..separate lines... ...
@@ -3131,10 +2992,10 @@ virCgroupGetPercpuVcpuSum(virCgroupPtr group, { int ret = -1; ssize_t i = -1; - char *buf = NULL; virCgroupPtr group_vcpu = NULL;
while ((i = virBitmapNextSetBit(guestvcpus, i)) >= 0) { + VIR_AUTOFREE(char *) buf = NULL; char *pos;
more cleanup possible here, since there's no point in having @pos at all, on the contrary, it's a bit confusing what it's being used for...so @pos should be dropped in a separate patch and @buf should be used directly, beware though: virStrToLong_ull(buf, &buf, 10, &tmp) would leak memory, so virStrToLong_ull(buf, NULL, 10, &tmp) should be used instead... ...
int virCgroupHasEmptyTasks(virCgroupPtr cgroup, int controller) { - int ret = -1;
^This is okay, so don't change it...
- char *content = NULL; + int ret; + VIR_AUTOFREE(char *) content = NULL;
if (!cgroup) return -1; @@ -4104,7 +3937,6 @@ virCgroupHasEmptyTasks(virCgroupPtr cgroup, int controller) if (ret == 0 && content[0] == '\0') ret = 1;
- VIR_FREE(content); return ret; }
Erik
Modify code to use VIR_AUTOFREE macro wherever required. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virfcp.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/util/virfcp.c b/src/util/virfcp.c index 7660ba7..b703744 100644 --- a/src/util/virfcp.c +++ b/src/util/virfcp.c @@ -40,16 +40,12 @@ bool virFCIsCapableRport(const char *rport) { - bool ret = false; - char *path = NULL; + VIR_AUTOFREE(char *) path = NULL; if (virBuildPath(&path, SYSFS_FC_RPORT_PATH, rport) < 0) return false; - ret = virFileExists(path); - VIR_FREE(path); - - return ret; + return virFileExists(path); } int @@ -57,8 +53,8 @@ virFCReadRportValue(const char *rport, const char *entry, char **result) { - int ret = -1; - char *buf = NULL, *p = NULL; + VIR_AUTOFREE(char *) buf = NULL; + char *p = NULL; if (virFileReadValueString(&buf, "%s/%s/%s", SYSFS_FC_RPORT_PATH, rport, entry) < 0) { @@ -69,13 +65,9 @@ virFCReadRportValue(const char *rport, *p = '\0'; if (VIR_STRDUP(*result, buf) < 0) - goto cleanup; - - ret = 0; + return -1; - cleanup: - VIR_FREE(buf); - return ret; + return 0; } #else -- 1.8.3.1
Modify code to use VIR_AUTOFREE macro wherever required. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virdnsmasq.c | 116 +++++++++++++++++--------------------------------- 1 file changed, 39 insertions(+), 77 deletions(-) diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 492dcad..c806cd2 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -95,7 +95,7 @@ addnhostsAdd(dnsmasqAddnHostsfile *addnhostsfile, virSocketAddr *ip, const char *name) { - char *ipstr = NULL; + VIR_AUTOFREE(char *) ipstr = NULL; int idx = -1; size_t i; @@ -111,35 +111,29 @@ addnhostsAdd(dnsmasqAddnHostsfile *addnhostsfile, if (idx < 0) { if (VIR_REALLOC_N(addnhostsfile->hosts, addnhostsfile->nhosts + 1) < 0) - goto error; + return -1; idx = addnhostsfile->nhosts; if (VIR_ALLOC(addnhostsfile->hosts[idx].hostnames) < 0) - goto error; + return -1; if (VIR_STRDUP(addnhostsfile->hosts[idx].ip, ipstr) < 0) - goto error; + return -1; addnhostsfile->hosts[idx].nhostnames = 0; addnhostsfile->nhosts++; } if (VIR_REALLOC_N(addnhostsfile->hosts[idx].hostnames, addnhostsfile->hosts[idx].nhostnames + 1) < 0) - goto error; + return -1; if (VIR_STRDUP(addnhostsfile->hosts[idx].hostnames[addnhostsfile->hosts[idx].nhostnames], name) < 0) - goto error; - - VIR_FREE(ipstr); + return -1; addnhostsfile->hosts[idx].nhostnames++; return 0; - - error: - VIR_FREE(ipstr); - return -1; } static dnsmasqAddnHostsfile * @@ -178,11 +172,10 @@ addnhostsWrite(const char *path, dnsmasqAddnHost *hosts, unsigned int nhosts) { - char *tmp; + VIR_AUTOFREE(char *) tmp = NULL; FILE *f; bool istmp = true; size_t i, j; - int rc = 0; /* even if there are 0 hosts, create a 0 length file, to allow * for runtime addition. @@ -193,61 +186,50 @@ addnhostsWrite(const char *path, if (!(f = fopen(tmp, "w"))) { istmp = false; - if (!(f = fopen(path, "w"))) { - rc = -errno; - goto cleanup; - } + if (!(f = fopen(path, "w"))) + return -errno; } for (i = 0; i < nhosts; i++) { if (fputs(hosts[i].ip, f) == EOF || fputc('\t', f) == EOF) { - rc = -errno; VIR_FORCE_FCLOSE(f); if (istmp) unlink(tmp); - goto cleanup; + return -errno; } for (j = 0; j < hosts[i].nhostnames; j++) { if (fputs(hosts[i].hostnames[j], f) == EOF || fputc('\t', f) == EOF) { - rc = -errno; VIR_FORCE_FCLOSE(f); if (istmp) unlink(tmp); - goto cleanup; + return -errno; } } if (fputc('\n', f) == EOF) { - rc = -errno; VIR_FORCE_FCLOSE(f); if (istmp) unlink(tmp); - goto cleanup; + return -errno; } } - if (VIR_FCLOSE(f) == EOF) { - rc = -errno; - goto cleanup; - } + if (VIR_FCLOSE(f) == EOF) + return -errno; if (istmp && rename(tmp, path) < 0) { - rc = -errno; unlink(tmp); - goto cleanup; + return -errno; } - cleanup: - VIR_FREE(tmp); - - return rc; + return 0; } static int @@ -310,9 +292,10 @@ hostsfileAdd(dnsmasqHostsfile *hostsfile, const char *id, bool ipv6) { - char *ipstr = NULL; + VIR_AUTOFREE(char *) ipstr = NULL; + if (VIR_REALLOC_N(hostsfile->hosts, hostsfile->nhosts + 1) < 0) - goto error; + return -1; if (!(ipstr = virSocketAddrFormat(ip))) return -1; @@ -322,38 +305,33 @@ hostsfileAdd(dnsmasqHostsfile *hostsfile, if (name && id) { if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, "id:%s,%s,[%s]", id, name, ipstr) < 0) - goto error; + return -1; } else if (name && !id) { if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, "%s,[%s]", name, ipstr) < 0) - goto error; + return -1; } else if (!name && id) { if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, "id:%s,[%s]", id, ipstr) < 0) - goto error; + return -1; } } else if (name && mac) { if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, "%s,%s,%s", mac, ipstr, name) < 0) - goto error; + return -1; } else if (name && !mac) { if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, "%s,%s", name, ipstr) < 0) - goto error; + return -1; } else { if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, "%s,%s", mac, ipstr) < 0) - goto error; + return -1; } - VIR_FREE(ipstr); hostsfile->nhosts++; return 0; - - error: - VIR_FREE(ipstr); - return -1; } static dnsmasqHostsfile * @@ -391,11 +369,10 @@ hostsfileWrite(const char *path, dnsmasqDhcpHost *hosts, unsigned int nhosts) { - char *tmp; + VIR_AUTOFREE(char *) tmp = NULL; FILE *f; bool istmp = true; size_t i; - int rc = 0; /* even if there are 0 hosts, create a 0 length file, to allow * for runtime addition. @@ -406,39 +383,30 @@ hostsfileWrite(const char *path, if (!(f = fopen(tmp, "w"))) { istmp = false; - if (!(f = fopen(path, "w"))) { - rc = -errno; - goto cleanup; - } + if (!(f = fopen(path, "w"))) + return -errno; } for (i = 0; i < nhosts; i++) { if (fputs(hosts[i].host, f) == EOF || fputc('\n', f) == EOF) { - rc = -errno; VIR_FORCE_FCLOSE(f); if (istmp) unlink(tmp); - goto cleanup; + return -errno; } } - if (VIR_FCLOSE(f) == EOF) { - rc = -errno; - goto cleanup; - } + if (VIR_FCLOSE(f) == EOF) + return -errno; if (istmp && rename(tmp, path) < 0) { - rc = -errno; unlink(tmp); - goto cleanup; + return -errno; } - cleanup: - VIR_FREE(tmp); - - return rc; + return 0; } static int @@ -708,17 +676,12 @@ dnsmasqCapsSetFromBuffer(dnsmasqCapsPtr caps, const char *buf) static int dnsmasqCapsSetFromFile(dnsmasqCapsPtr caps, const char *path) { - int ret = -1; - char *buf = NULL; + VIR_AUTOFREE(char *) buf = NULL; if (virFileReadAll(path, 1024 * 1024, &buf) < 0) - goto cleanup; - - ret = dnsmasqCapsSetFromBuffer(caps, buf); + return -1; - cleanup: - VIR_FREE(buf); - return ret; + return dnsmasqCapsSetFromBuffer(caps, buf); } static int @@ -727,7 +690,9 @@ dnsmasqCapsRefreshInternal(dnsmasqCapsPtr caps, bool force) int ret = -1; struct stat sb; virCommandPtr cmd = NULL; - char *help = NULL, *version = NULL, *complete = NULL; + VIR_AUTOFREE(char *) help = NULL; + VIR_AUTOFREE(char *) version = NULL; + VIR_AUTOFREE(char *) complete = NULL; if (!caps || caps->noRefresh) return 0; @@ -779,9 +744,6 @@ dnsmasqCapsRefreshInternal(dnsmasqCapsPtr caps, bool force) cleanup: virCommandFree(cmd); - VIR_FREE(help); - VIR_FREE(version); - VIR_FREE(complete); return ret; } -- 1.8.3.1
Modify code to use VIR_AUTOFREE macro wherever required. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/vireventpoll.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/util/vireventpoll.c b/src/util/vireventpoll.c index 81ecab4..13d278d 100644 --- a/src/util/vireventpoll.c +++ b/src/util/vireventpoll.c @@ -618,7 +618,7 @@ static void virEventPollCleanupHandles(void) */ int virEventPollRunOnce(void) { - struct pollfd *fds = NULL; + VIR_AUTOFREE(struct pollfd *) fds = NULL; int ret, timeout, nfds; virMutexLock(&eventLoop.lock); @@ -645,7 +645,7 @@ int virEventPollRunOnce(void) goto retry; virReportSystemError(errno, "%s", _("Unable to poll on file handles")); - goto error_unlocked; + return -1; } EVENT_DEBUG("Poll got %d event(s)", ret); @@ -662,13 +662,10 @@ int virEventPollRunOnce(void) eventLoop.running = 0; virMutexUnlock(&eventLoop.lock); - VIR_FREE(fds); return 0; error: virMutexUnlock(&eventLoop.lock); - error_unlocked: - VIR_FREE(fds); return -1; } -- 1.8.3.1
Modify code to use VIR_AUTOFREE macro wherever required. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virdbus.c | 28 +++++++--------------------- 1 file changed, 7 insertions(+), 21 deletions(-) diff --git a/src/util/virdbus.c b/src/util/virdbus.c index ba8b684..66dbe41 100644 --- a/src/util/virdbus.c +++ b/src/util/virdbus.c @@ -614,9 +614,8 @@ virDBusMessageIterEncode(DBusMessageIter *rootiter, size_t nstack = 0; size_t siglen; size_t skiplen; - char *contsig = NULL; const char *vsig; - DBusMessageIter *newiter = NULL; + VIR_AUTOFREE(DBusMessageIter *) newiter = NULL; DBusMessageIter *iter = rootiter; VIR_DEBUG("rootiter=%p types=%s", rootiter, types); @@ -628,6 +627,7 @@ virDBusMessageIterEncode(DBusMessageIter *rootiter, nstruct = strlen(types); for (;;) { + VIR_AUTOFREE(char *) contsig = NULL; const char *t; VIR_DEBUG("Loop nstack=%zu narray=%zd nstruct=%zu types='%s'", @@ -752,11 +752,8 @@ virDBusMessageIterEncode(DBusMessageIter *rootiter, goto cleanup; if (virDBusTypeStackPush(&stack, &nstack, iter, types, - nstruct, narray) < 0) { - VIR_FREE(newiter); + nstruct, narray) < 0) goto cleanup; - } - VIR_FREE(contsig); iter = newiter; newiter = NULL; types = t + 1; @@ -780,10 +777,8 @@ virDBusMessageIterEncode(DBusMessageIter *rootiter, goto cleanup; if (virDBusTypeStackPush(&stack, &nstack, iter, types, - nstruct, narray) < 0) { - VIR_FREE(newiter); + nstruct, narray) < 0) goto cleanup; - } iter = newiter; newiter = NULL; types = vsig; @@ -811,11 +806,8 @@ virDBusMessageIterEncode(DBusMessageIter *rootiter, if (virDBusTypeStackPush(&stack, &nstack, iter, types, - nstruct, narray) < 0) { - VIR_FREE(newiter); + nstruct, narray) < 0) goto cleanup; - } - VIR_FREE(contsig); iter = newiter; newiter = NULL; types = t + 1; @@ -847,8 +839,6 @@ virDBusMessageIterEncode(DBusMessageIter *rootiter, } virDBusTypeStackFree(&stack, &nstack); - VIR_FREE(contsig); - VIR_FREE(newiter); return ret; } # undef SET_NEXT_VAL @@ -891,9 +881,8 @@ virDBusMessageIterDecode(DBusMessageIter *rootiter, size_t nstack = 0; size_t skiplen; size_t siglen; - char *contsig = NULL; const char *vsig; - DBusMessageIter *newiter = NULL; + VIR_AUTOFREE(DBusMessageIter *) newiter = NULL; DBusMessageIter *iter = rootiter; VIR_DEBUG("rootiter=%p types=%s", rootiter, types); @@ -905,6 +894,7 @@ virDBusMessageIterDecode(DBusMessageIter *rootiter, nstruct = strlen(types); for (;;) { + VIR_AUTOFREE(char *) contsig = NULL; const char *t; bool advanceiter = true; @@ -1053,7 +1043,6 @@ virDBusMessageIterDecode(DBusMessageIter *rootiter, iter, types, nstruct, narray) < 0) goto cleanup; - VIR_FREE(contsig); iter = newiter; newiter = NULL; types = t + 1; @@ -1112,7 +1101,6 @@ virDBusMessageIterDecode(DBusMessageIter *rootiter, iter, types, nstruct, narray) < 0) goto cleanup; - VIR_FREE(contsig); iter = newiter; newiter = NULL; types = t + 1; @@ -1167,8 +1155,6 @@ virDBusMessageIterDecode(DBusMessageIter *rootiter, cleanup: virDBusTypeStackFree(&stack, &nstack); - VIR_FREE(contsig); - VIR_FREE(newiter); return ret; } # undef GET_NEXT_VAL -- 1.8.3.1
On Sun, Jun 03, 2018 at 01:42:10PM +0530, Sukrit Bhatnagar wrote:
Modify code to use VIR_AUTOFREE macro wherever required.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virdbus.c | 28 +++++++--------------------- 1 file changed, 7 insertions(+), 21 deletions(-)
diff --git a/src/util/virdbus.c b/src/util/virdbus.c index ba8b684..66dbe41 100644 --- a/src/util/virdbus.c +++ b/src/util/virdbus.c @@ -614,9 +614,8 @@ virDBusMessageIterEncode(DBusMessageIter *rootiter, size_t nstack = 0; size_t siglen; size_t skiplen; - char *contsig = NULL; const char *vsig; - DBusMessageIter *newiter = NULL; + VIR_AUTOFREE(DBusMessageIter *) newiter = NULL;
Doh!...and here we are facing a potential exception to the rule I mentioned in one of the previous patches about compound types and VIR_AUTOFREE...Sigh, although possible, I really don't think we want to introduce *Free helpers for external types like ^this...that's honestly my only argument to support my earlier claim about the VIR_AUTOFREE usage with scalar types only. It's not a "libvirt-native" type, so let's close our eyes and forget that we're doing this. Erik
Modify code to use VIR_AUTOFREE macro wherever required. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virfdstream.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/virfdstream.c b/src/util/virfdstream.c index e4973a2..e7befbc 100644 --- a/src/util/virfdstream.c +++ b/src/util/virfdstream.c @@ -431,7 +431,7 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst, virFDStreamMsgPtr msg = NULL; int inData = 0; long long sectionLen = 0; - char *buf = NULL; + VIR_AUTOFREE(char *) buf = NULL; ssize_t got; if (sparse && *dataLen == 0) { @@ -496,7 +496,6 @@ virFDStreamThreadDoRead(virFDStreamDataPtr fdst, return got; error: - VIR_FREE(buf); virFDStreamMsgFree(msg); return -1; } -- 1.8.3.1
Modify code to use VIR_AUTOFREE macro wherever required. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virfile.c | 303 +++++++++++++++++------------------------------------ 1 file changed, 99 insertions(+), 204 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 523241f..52b601d 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -211,7 +211,7 @@ virFileWrapperFdNew(int *fd, const char *name, unsigned int flags) bool output = false; int pipefd[2] = { -1, -1 }; int mode = -1; - char *iohelper_path = NULL; + VIR_AUTOFREE(char *) iohelper_path = NULL; if (!flags) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -262,8 +262,6 @@ virFileWrapperFdNew(int *fd, const char *name, unsigned int flags) ret->cmd = virCommandNewArgList(iohelper_path, name, NULL); - VIR_FREE(iohelper_path); - if (output) { virCommandSetInputFD(ret->cmd, pipefd[0]); virCommandSetOutputFD(ret->cmd, fd); @@ -294,7 +292,6 @@ virFileWrapperFdNew(int *fd, const char *name, unsigned int flags) return ret; error: - VIR_FREE(iohelper_path); VIR_FORCE_CLOSE(pipefd[0]); VIR_FORCE_CLOSE(pipefd[1]); virFileWrapperFdFree(ret); @@ -453,7 +450,7 @@ virFileRewrite(const char *path, virFileRewriteFunc rewrite, const void *opaque) { - char *newfile = NULL; + VIR_AUTOFREE(char *) newfile = NULL; int fd = -1; int ret = -1; @@ -494,10 +491,8 @@ virFileRewrite(const char *path, cleanup: VIR_FORCE_CLOSE(fd); - if (newfile) { + if (newfile) unlink(newfile); - VIR_FREE(newfile); - } return ret; } @@ -724,7 +719,7 @@ int virFileLoopDeviceAssociate(const char *file, int lofd = -1; int fsfd = -1; struct loop_info64 lo; - char *loname = NULL; + VIR_AUTOFREE(char *) loname = NULL; int ret = -1; if ((lofd = virFileLoopDeviceOpen(&loname)) < 0) @@ -762,7 +757,6 @@ int virFileLoopDeviceAssociate(const char *file, ret = 0; cleanup: - VIR_FREE(loname); VIR_FORCE_CLOSE(fsfd); if (ret == -1) VIR_FORCE_CLOSE(lofd); @@ -777,8 +771,7 @@ int virFileLoopDeviceAssociate(const char *file, static int virFileNBDDeviceIsBusy(const char *dev_name) { - char *path; - int ret = -1; + VIR_AUTOFREE(char *) path = NULL; if (virAsprintf(&path, SYSFS_BLOCK_DIR "/%s/pid", dev_name) < 0) @@ -786,18 +779,15 @@ virFileNBDDeviceIsBusy(const char *dev_name) if (!virFileExists(path)) { if (errno == ENOENT) - ret = 0; + return 0; else virReportSystemError(errno, _("Cannot check NBD device %s pid"), dev_name); - goto cleanup; + return -1; } - ret = 1; - cleanup: - VIR_FREE(path); - return ret; + return 1; } @@ -842,15 +832,13 @@ virFileNBDLoadDriver(void) "administratively prohibited")); return false; } else { - char *errbuf = NULL; + VIR_AUTOFREE(char *) errbuf = NULL; if ((errbuf = virKModLoad(NBD_DRIVER, true))) { - VIR_FREE(errbuf); virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to load nbd module")); return false; } - VIR_FREE(errbuf); } return true; } @@ -860,8 +848,8 @@ int virFileNBDDeviceAssociate(const char *file, bool readonly, char **dev) { - char *nbddev = NULL; - char *qemunbd = NULL; + VIR_AUTOFREE(char *) nbddev = NULL; + VIR_AUTOFREE(char *) qemunbd = NULL; virCommandPtr cmd = NULL; int ret = -1; const char *fmtstr = NULL; @@ -909,8 +897,6 @@ int virFileNBDDeviceAssociate(const char *file, ret = 0; cleanup: - VIR_FREE(nbddev); - VIR_FREE(qemunbd); virCommandFree(cmd); return ret; } @@ -957,7 +943,6 @@ int virFileDeleteTree(const char *dir) { DIR *dh; struct dirent *de; - char *filepath = NULL; int ret = -1; int direrr; @@ -969,6 +954,7 @@ int virFileDeleteTree(const char *dir) return -1; while ((direrr = virDirRead(dh, &de, dir)) > 0) { + VIR_AUTOFREE(char *) filepath = NULL; struct stat sb; if (virAsprintf(&filepath, "%s/%s", @@ -992,8 +978,6 @@ int virFileDeleteTree(const char *dir) goto cleanup; } } - - VIR_FREE(filepath); } if (direrr < 0) goto cleanup; @@ -1008,7 +992,6 @@ int virFileDeleteTree(const char *dir) ret = 0; cleanup: - VIR_FREE(filepath); VIR_DIR_CLOSE(dh); return ret; } @@ -1166,7 +1149,7 @@ static int safezero_slow(int fd, off_t offset, off_t len) { int r; - char *buf; + VIR_AUTOFREE(char *) buf = NULL; unsigned long long remain, bytes; if (lseek(fd, offset, SEEK_SET) < 0) @@ -1187,15 +1170,12 @@ safezero_slow(int fd, off_t offset, off_t len) bytes = remain; r = safewrite(fd, buf, bytes); - if (r < 0) { - VIR_FREE(buf); + if (r < 0) return -1; - } /* safewrite() guarantees all data will be written */ remain -= bytes; } - VIR_FREE(buf); return 0; } @@ -1558,8 +1538,7 @@ virFileRelLinkPointsTo(const char *directory, const char *checkLink, const char *checkDest) { - char *candidate; - int ret; + VIR_AUTOFREE(char *) candidate = NULL; if (*checkLink == '/') return virFileLinkPointsTo(checkLink, checkDest); @@ -1571,9 +1550,8 @@ virFileRelLinkPointsTo(const char *directory, } if (virAsprintf(&candidate, "%s/%s", directory, checkLink) < 0) return -1; - ret = virFileLinkPointsTo(candidate, checkDest); - VIR_FREE(candidate); - return ret; + + return virFileLinkPointsTo(candidate, checkDest); } @@ -1897,44 +1875,39 @@ virFileIsExecutable(const char *file) */ int virFileIsMountPoint(const char *file) { - char *parent = NULL; - int ret = -1; + VIR_AUTOFREE(char *) parent = NULL; + int ret; struct stat sb1, sb2; if (!(parent = mdir_name(file))) { virReportOOMError(); - goto cleanup; + return -1; } VIR_DEBUG("Comparing '%s' to '%s'", file, parent); if (stat(file, &sb1) < 0) { if (errno == ENOENT) - ret = 0; + return 0; else virReportSystemError(errno, _("Cannot stat '%s'"), file); - goto cleanup; + return -1; } if (stat(parent, &sb2) < 0) { virReportSystemError(errno, _("Cannot stat '%s'"), parent); - goto cleanup; + return -1; } - if (!S_ISDIR(sb1.st_mode)) { - ret = 0; - goto cleanup; - } + if (!S_ISDIR(sb1.st_mode)) + return 0; ret = sb1.st_dev != sb2.st_dev; VIR_DEBUG("Is mount %d", ret); - - cleanup: - VIR_FREE(parent); return ret; } @@ -2118,7 +2091,7 @@ virFileAccessibleAs(const char *path, int mode, pid_t pid = 0; int status, ret = 0; int forkRet = 0; - gid_t *groups; + VIR_AUTOFREE(gid_t *) groups = NULL; int ngroups; if (uid == geteuid() && @@ -2131,13 +2104,10 @@ virFileAccessibleAs(const char *path, int mode, pid = virFork(); - if (pid < 0) { - VIR_FREE(groups); + if (pid < 0) return -1; - } if (pid) { /* parent */ - VIR_FREE(groups); if (virProcessWait(pid, &status, false) < 0) { /* virProcessWait() already reported error */ errno = EINTR; @@ -2242,7 +2212,7 @@ virFileOpenForked(const char *path, int openflags, mode_t mode, int recvfd_errno = 0; int fd = -1; int pair[2] = { -1, -1 }; - gid_t *groups; + VIR_AUTOFREE(gid_t *) groups = NULL; int ngroups; bool created = false; @@ -2260,16 +2230,12 @@ virFileOpenForked(const char *path, int openflags, mode_t mode, virReportSystemError(errno, _("failed to create socket needed for '%s'"), path); - VIR_FREE(groups); return ret; } pid = virFork(); - if (pid < 0) { - ret = -errno; - VIR_FREE(groups); - return ret; - } + if (pid < 0) + return -errno; if (pid == 0) { @@ -2334,7 +2300,6 @@ virFileOpenForked(const char *path, int openflags, mode_t mode, /* parent */ - VIR_FREE(groups); VIR_FORCE_CLOSE(pair[1]); do { @@ -2535,7 +2500,7 @@ virFileRemove(const char *path, { pid_t pid; int status = 0, ret = 0; - gid_t *groups; + VIR_AUTOFREE(gid_t *) groups = NULL; int ngroups; if (!virFileRemoveNeedsSetuid(path, uid, gid)) { @@ -2560,15 +2525,11 @@ virFileRemove(const char *path, pid = virFork(); - if (pid < 0) { - ret = -errno; - VIR_FREE(groups); - return ret; - } + if (pid < 0) + return -errno; if (pid) { /* parent */ /* wait for child to complete, and retrieve its exit code */ - VIR_FREE(groups); if (virProcessWait(pid, &status, 0) < 0) { /* virProcessWait() reports errno on waitpid failure, so we'll just @@ -2700,7 +2661,7 @@ virDirCreate(const char *path, struct stat st; pid_t pid; int status = 0, ret = 0; - gid_t *groups; + VIR_AUTOFREE(gid_t *) groups = NULL; int ngroups; bool created = false; @@ -2736,15 +2697,11 @@ virDirCreate(const char *path, pid = virFork(); - if (pid < 0) { - ret = -errno; - VIR_FREE(groups); - return ret; - } + if (pid < 0) + return -errno; if (pid) { /* parent */ /* wait for child to complete, and retrieve its exit code */ - VIR_FREE(groups); if (virProcessWait(pid, &status, 0) < 0) { /* virProcessWait() reports errno on waitpid failure, so we'll just @@ -3045,19 +3002,14 @@ int virFileMakePathWithMode(const char *path, mode_t mode) { - int ret = -1; - char *tmp; + VIR_AUTOFREE(char *) tmp = NULL; if (VIR_STRDUP(tmp, path) < 0) { errno = ENOMEM; - goto cleanup; + return -1; } - ret = virFileMakePathHelper(tmp, mode); - - cleanup: - VIR_FREE(tmp); - return ret; + return virFileMakePathHelper(tmp, mode); } @@ -3065,8 +3017,7 @@ int virFileMakeParentPath(const char *path) { char *p; - char *tmp; - int ret = -1; + VIR_AUTOFREE(char *) tmp = NULL; VIR_DEBUG("path=%s", path); @@ -3077,15 +3028,11 @@ virFileMakeParentPath(const char *path) if ((p = strrchr(tmp, '/')) == NULL) { errno = EINVAL; - goto cleanup; + return -1; } *p = '\0'; - ret = virFileMakePathHelper(tmp, 0777); - - cleanup: - VIR_FREE(tmp); - return ret; + return virFileMakePathHelper(tmp, 0777); } @@ -3119,7 +3066,7 @@ virFileOpenTty(int *ttymaster, char **ttyName, int rawmode) * doesn't have to worry about that mess? */ int ret = -1; int slave = -1; - char *name = NULL; + VIR_AUTOFREE(char *) name = NULL; /* Unfortunately, we can't use the name argument of openpty, since * there is no guarantee on how large the buffer has to be. @@ -3180,7 +3127,6 @@ virFileOpenTty(int *ttymaster, char **ttyName, int rawmode) if (ret != 0) VIR_FORCE_CLOSE(*ttymaster); VIR_FORCE_CLOSE(slave); - VIR_FREE(name); return ret; } @@ -3280,7 +3226,7 @@ virFileSkipRoot(const char *path) int virFileAbsPath(const char *path, char **abspath) { - char *buf; + VIR_AUTOFREE(char *) buf = NULL; if (path[0] == '/') { if (VIR_STRDUP(*abspath, path) < 0) @@ -3290,11 +3236,8 @@ virFileAbsPath(const char *path, char **abspath) if (buf == NULL) return -1; - if (virAsprintf(abspath, "%s/%s", buf, path) < 0) { - VIR_FREE(buf); + if (virAsprintf(abspath, "%s/%s", buf, path) < 0) return -1; - } - VIR_FREE(buf); } return 0; @@ -3394,7 +3337,7 @@ virFileRemoveLastComponent(char *path) int virFilePrintf(FILE *fp, const char *msg, ...) { va_list vargs; - char *str; + VIR_AUTOFREE(char *) str = NULL; int ret; va_start(vargs, msg); @@ -3408,8 +3351,6 @@ int virFilePrintf(FILE *fp, const char *msg, ...) ret = -1; } - VIR_FREE(str); - cleanup: va_end(vargs); @@ -3445,7 +3386,8 @@ int virFileIsSharedFSType(const char *path, int fstypes) { - char *dirpath, *p; + VIR_AUTOFREE(char *) dirpath = NULL; + char *p; struct statfs sb; int statfs_ret; @@ -3465,7 +3407,6 @@ virFileIsSharedFSType(const char *path, if ((p = strrchr(dirpath, '/')) == NULL) { virReportSystemError(EINVAL, _("Invalid relative path '%s'"), path); - VIR_FREE(dirpath); return -1; } @@ -3478,8 +3419,6 @@ virFileIsSharedFSType(const char *path, } while ((statfs_ret < 0) && (p != dirpath)); - VIR_FREE(dirpath); - if (statfs_ret < 0) { virReportSystemError(errno, _("cannot determine filesystem for '%s'"), @@ -3546,18 +3485,18 @@ virFileGetHugepageSize(const char *path, static int virFileGetDefaultHugepageSize(unsigned long long *size) { - int ret = -1; - char *meminfo, *c, *n, *unit; + VIR_AUTOFREE(char *) meminfo = NULL; + char *c, *n, *unit; if (virFileReadAll(PROC_MEMINFO, 4096, &meminfo) < 0) - goto cleanup; + return -1; if (!(c = strstr(meminfo, HUGEPAGESIZE_STR))) { virReportError(VIR_ERR_NO_SUPPORT, _("%s not found in %s"), HUGEPAGESIZE_STR, PROC_MEMINFO); - goto cleanup; + return -1; } c += strlen(HUGEPAGESIZE_STR); @@ -3570,13 +3509,10 @@ virFileGetDefaultHugepageSize(unsigned long long *size) virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse %s %s"), HUGEPAGESIZE_STR, c); - goto cleanup; + return -1; } - ret = 0; - cleanup: - VIR_FREE(meminfo); - return ret; + return 0; } # define PROC_MOUNTS "/proc/mounts" @@ -3589,7 +3525,7 @@ virFileFindHugeTLBFS(virHugeTLBFSPtr *ret_fs, FILE *f = NULL; struct mntent mb; char mntbuf[1024]; - virHugeTLBFSPtr fs = NULL; + VIR_AUTOFREE(virHugeTLBFSPtr) fs = NULL; size_t nfs = 0; unsigned long long default_hugepagesz = 0; @@ -3634,7 +3570,6 @@ virFileFindHugeTLBFS(virHugeTLBFSPtr *ret_fs, endmntent(f); while (nfs) VIR_FREE(fs[--nfs].mnt_dir); - VIR_FREE(fs); return ret; } @@ -3870,10 +3805,8 @@ virFileCopyACLs(const char *src, int virFileComparePaths(const char *p1, const char *p2) { - int ret = -1; - char *res1, *res2; - - res1 = res2 = NULL; + VIR_AUTOFREE(char *) res1 = NULL; + VIR_AUTOFREE(char *) res2 = NULL; /* Assume p1 and p2 are symlinks, so try to resolve and canonicalize them. * Canonicalization fails for example on file systems names like 'proc' or @@ -3882,19 +3815,13 @@ virFileComparePaths(const char *p1, const char *p2) */ ignore_value(virFileResolveLink(p1, &res1)); if (!res1 && VIR_STRDUP(res1, p1) < 0) - goto cleanup; + return -1; ignore_value(virFileResolveLink(p2, &res2)); if (!res2 && VIR_STRDUP(res2, p2) < 0) - goto cleanup; - - ret = STREQ_NULLABLE(res1, res2); - - cleanup: - VIR_FREE(res1); - VIR_FREE(res2); + return -1; - return ret; + return STREQ_NULLABLE(res1, res2); } @@ -4038,25 +3965,22 @@ virFileInData(int fd ATTRIBUTE_UNUSED, int virFileReadValueInt(int *value, const char *format, ...) { - int ret = -1; - char *str = NULL; - char *path = NULL; + VIR_AUTOFREE(char *) str = NULL; + VIR_AUTOFREE(char *) path = NULL; va_list ap; va_start(ap, format); if (virVasprintf(&path, format, ap) < 0) { va_end(ap); - goto cleanup; + return -1; } va_end(ap); - if (!virFileExists(path)) { - ret = -2; - goto cleanup; - } + if (!virFileExists(path)) + return -2; if (virFileReadAll(path, INT_BUFSIZE_BOUND(*value), &str) < 0) - goto cleanup; + return -1; virStringTrimOptionalNewline(str); @@ -4064,14 +3988,10 @@ virFileReadValueInt(int *value, const char *format, ...) virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid integer value '%s' in file '%s'"), str, path); - goto cleanup; + return -1; } - ret = 0; - cleanup: - VIR_FREE(path); - VIR_FREE(str); - return ret; + return 0; } @@ -4088,25 +4008,22 @@ virFileReadValueInt(int *value, const char *format, ...) int virFileReadValueUint(unsigned int *value, const char *format, ...) { - int ret = -1; - char *str = NULL; - char *path = NULL; + VIR_AUTOFREE(char *) str = NULL; + VIR_AUTOFREE(char *) path = NULL; va_list ap; va_start(ap, format); if (virVasprintf(&path, format, ap) < 0) { va_end(ap); - goto cleanup; + return -1; } va_end(ap); - if (!virFileExists(path)) { - ret = -2; - goto cleanup; - } + if (!virFileExists(path)) + return -2; if (virFileReadAll(path, INT_BUFSIZE_BOUND(*value), &str) < 0) - goto cleanup; + return -1; virStringTrimOptionalNewline(str); @@ -4114,14 +4031,10 @@ virFileReadValueUint(unsigned int *value, const char *format, ...) virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid unsigned integer value '%s' in file '%s'"), str, path); - goto cleanup; + return -1; } - ret = 0; - cleanup: - VIR_FREE(path); - VIR_FREE(str); - return ret; + return 0; } @@ -4138,26 +4051,23 @@ virFileReadValueUint(unsigned int *value, const char *format, ...) int virFileReadValueScaledInt(unsigned long long *value, const char *format, ...) { - int ret = -1; - char *str = NULL; + VIR_AUTOFREE(char *) str = NULL; + VIR_AUTOFREE(char *) path = NULL; char *endp = NULL; - char *path = NULL; va_list ap; va_start(ap, format); if (virVasprintf(&path, format, ap) < 0) { va_end(ap); - goto cleanup; + return -1; } va_end(ap); - if (!virFileExists(path)) { - ret = -2; - goto cleanup; - } + if (!virFileExists(path)) + return -2; if (virFileReadAll(path, INT_BUFSIZE_BOUND(*value), &str) < 0) - goto cleanup; + return -1; virStringTrimOptionalNewline(str); @@ -4165,14 +4075,10 @@ virFileReadValueScaledInt(unsigned long long *value, const char *format, ...) virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid unsigned scaled integer value '%s' in file '%s'"), str, path); - goto cleanup; + return -1; } - ret = virScaleInteger(value, endp, 1024, ULLONG_MAX); - cleanup: - VIR_FREE(path); - VIR_FREE(str); - return ret; + return virScaleInteger(value, endp, 1024, ULLONG_MAX); } /* Arbitrarily sized number, feel free to change, but the function should be @@ -4192,37 +4098,30 @@ virFileReadValueScaledInt(unsigned long long *value, const char *format, ...) int virFileReadValueBitmap(virBitmapPtr *value, const char *format, ...) { - int ret = -1; - char *str = NULL; - char *path = NULL; + VIR_AUTOFREE(char *) str = NULL; + VIR_AUTOFREE(char *) path = NULL; va_list ap; va_start(ap, format); if (virVasprintf(&path, format, ap) < 0) { va_end(ap); - goto cleanup; + return -1; } va_end(ap); - if (!virFileExists(path)) { - ret = -2; - goto cleanup; - } + if (!virFileExists(path)) + return -2; if (virFileReadAll(path, VIR_FILE_READ_VALUE_STRING_MAX, &str) < 0) - goto cleanup; + return -1; virStringTrimOptionalNewline(str); *value = virBitmapParseUnlimited(str); if (!*value) - goto cleanup; + return -1; - ret = 0; - cleanup: - VIR_FREE(path); - VIR_FREE(str); - return ret; + return 0; } /** @@ -4240,30 +4139,26 @@ virFileReadValueBitmap(virBitmapPtr *value, const char *format, ...) int virFileReadValueString(char **value, const char *format, ...) { - int ret = -1; - char *str = NULL; - char *path = NULL; + int ret; + VIR_AUTOFREE(char *) str = NULL; + VIR_AUTOFREE(char *) path = NULL; va_list ap; va_start(ap, format); if (virVasprintf(&path, format, ap) < 0) { va_end(ap); - goto cleanup; + return -1; } va_end(ap); - if (!virFileExists(path)) { - ret = -2; - goto cleanup; - } + if (!virFileExists(path)) + return -2; ret = virFileReadAll(path, VIR_FILE_READ_VALUE_STRING_MAX, value); if (*value) virStringTrimOptionalNewline(*value); - cleanup: - VIR_FREE(path); - VIR_FREE(str); + return ret; } -- 1.8.3.1
On Sun, Jun 03, 2018 at 01:42:12PM +0530, Sukrit Bhatnagar wrote:
Modify code to use VIR_AUTOFREE macro wherever required.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virfile.c | 303 +++++++++++++++++------------------------------------ 1 file changed, 99 insertions(+), 204 deletions(-)
...
@@ -3546,18 +3485,18 @@ virFileGetHugepageSize(const char *path, static int virFileGetDefaultHugepageSize(unsigned long long *size) { - int ret = -1; - char *meminfo, *c, *n, *unit; + VIR_AUTOFREE(char *) meminfo = NULL; + char *c, *n, *unit;
one variable per line...
if (virFileReadAll(PROC_MEMINFO, 4096, &meminfo) < 0) - goto cleanup; + return -1;
if (!(c = strstr(meminfo, HUGEPAGESIZE_STR))) { virReportError(VIR_ERR_NO_SUPPORT, _("%s not found in %s"), HUGEPAGESIZE_STR, PROC_MEMINFO); - goto cleanup; + return -1; } c += strlen(HUGEPAGESIZE_STR);
@@ -3570,13 +3509,10 @@ virFileGetDefaultHugepageSize(unsigned long long *size) virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse %s %s"), HUGEPAGESIZE_STR, c); - goto cleanup; + return -1; }
- ret = 0; - cleanup: - VIR_FREE(meminfo); - return ret; + return 0; }
# define PROC_MOUNTS "/proc/mounts" @@ -3589,7 +3525,7 @@ virFileFindHugeTLBFS(virHugeTLBFSPtr *ret_fs, FILE *f = NULL; struct mntent mb; char mntbuf[1024]; - virHugeTLBFSPtr fs = NULL; + VIR_AUTOFREE(virHugeTLBFSPtr) fs = NULL;
compound type (internal), needs a dedicated free helper...
size_t nfs = 0; unsigned long long default_hugepagesz = 0;
@@ -3634,7 +3570,6 @@ virFileFindHugeTLBFS(virHugeTLBFSPtr *ret_fs, endmntent(f); while (nfs) VIR_FREE(fs[--nfs].mnt_dir);
...especially because of ^this...
- VIR_FREE(fs); return ret; }
Erik
Modify code to use VIR_AUTOFREE macro wherever required. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virconf.c | 42 ++++++++++++------------------------------ 1 file changed, 12 insertions(+), 30 deletions(-) diff --git a/src/util/virconf.c b/src/util/virconf.c index e0a3fd1..7dd8820 100644 --- a/src/util/virconf.c +++ b/src/util/virconf.c @@ -745,9 +745,8 @@ virConfParse(const char *filename, const char *content, int len, virConfPtr virConfReadFile(const char *filename, unsigned int flags) { - char *content; + VIR_AUTOFREE(char *) content = NULL; int len; - virConfPtr conf; VIR_DEBUG("filename=%s", NULLSTR(filename)); @@ -759,11 +758,7 @@ virConfReadFile(const char *filename, unsigned int flags) if ((len = virFileReadAll(filename, MAX_CONFIG_FILE_SIZE, &content)) < 0) return NULL; - conf = virConfParse(filename, content, len, flags); - - VIR_FREE(content); - - return conf; + return virConfParse(filename, content, len, flags); } /** @@ -1451,7 +1446,7 @@ virConfWriteFile(const char *filename, virConfPtr conf) virConfEntryPtr cur; int ret; int fd; - char *content; + VIR_AUTOFREE(char *) content = NULL; unsigned int use; if (conf == NULL) @@ -1476,7 +1471,6 @@ virConfWriteFile(const char *filename, virConfPtr conf) use = virBufferUse(&buf); content = virBufferContentAndReset(&buf); ret = safewrite(fd, content, use); - VIR_FREE(content); VIR_FORCE_CLOSE(fd); if (ret != (int)use) { virConfError(NULL, VIR_ERR_WRITE_FAILED, _("failed to save content")); @@ -1504,7 +1498,7 @@ virConfWriteMem(char *memory, int *len, virConfPtr conf) { virBuffer buf = VIR_BUFFER_INITIALIZER; virConfEntryPtr cur; - char *content; + VIR_AUTOFREE(char *) content = NULL; unsigned int use; if ((memory == NULL) || (len == NULL) || (*len <= 0) || (conf == NULL)) @@ -1524,11 +1518,9 @@ virConfWriteMem(char *memory, int *len, virConfPtr conf) if ((int)use >= *len) { *len = (int)use; - VIR_FREE(content); return -1; } memcpy(memory, content, use); - VIR_FREE(content); *len = use; return use; } @@ -1542,16 +1534,13 @@ virConfLoadConfigPath(const char *name) SYSCONFDIR, name) < 0) return NULL; } else { - char *userdir = virGetUserConfigDirectory(); + VIR_AUTOFREE(char *) userdir = virGetUserConfigDirectory(); if (!userdir) return NULL; if (virAsprintf(&path, "%s/%s", - userdir, name) < 0) { - VIR_FREE(userdir); + userdir, name) < 0) return NULL; - } - VIR_FREE(userdir); } return path; @@ -1560,26 +1549,19 @@ virConfLoadConfigPath(const char *name) int virConfLoadConfig(virConfPtr *conf, const char *name) { - char *path = NULL; - int ret = -1; + VIR_AUTOFREE(char *) path = NULL; *conf = NULL; if (!(path = virConfLoadConfigPath(name))) - goto cleanup; + return -1; - if (!virFileExists(path)) { - ret = 0; - goto cleanup; - } + if (!virFileExists(path)) + return 0; VIR_DEBUG("Loading config file '%s'", path); if (!(*conf = virConfReadFile(path, 0))) - goto cleanup; - - ret = 0; + return -1; - cleanup: - VIR_FREE(path); - return ret; + return 0; } -- 1.8.3.1
Modify code to use VIR_AUTOFREE macro wherever required. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virfilecache.c | 35 +++++++++++------------------------ 1 file changed, 11 insertions(+), 24 deletions(-) diff --git a/src/util/virfilecache.c b/src/util/virfilecache.c index dab7216..49049b7 100644 --- a/src/util/virfilecache.c +++ b/src/util/virfilecache.c @@ -100,18 +100,17 @@ static char * virFileCacheGetFileName(virFileCachePtr cache, const char *name) { - char *file = NULL; - char *namehash = NULL; + VIR_AUTOFREE(char *) namehash = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, name, &namehash) < 0) - goto cleanup; + return NULL; if (virFileMakePath(cache->dir) < 0) { virReportSystemError(errno, _("Unable to create directory '%s'"), cache->dir); - goto cleanup; + return NULL; } virBufferAsprintf(&buf, "%s/%s", cache->dir, namehash); @@ -120,13 +119,9 @@ virFileCacheGetFileName(virFileCachePtr cache, virBufferAsprintf(&buf, ".%s", cache->suffix); if (virBufferCheckError(&buf) < 0) - goto cleanup; - - file = virBufferContentAndReset(&buf); + return NULL; - cleanup: - VIR_FREE(namehash); - return file; + return virBufferContentAndReset(&buf); } @@ -135,7 +130,7 @@ virFileCacheLoad(virFileCachePtr cache, const char *name, void **data) { - char *file = NULL; + VIR_AUTOFREE(char *) file = NULL; int ret = -1; void *loadData = NULL; @@ -179,7 +174,6 @@ virFileCacheLoad(virFileCachePtr cache, cleanup: virObjectUnref(loadData); - VIR_FREE(file); return ret; } @@ -189,20 +183,15 @@ virFileCacheSave(virFileCachePtr cache, const char *name, void *data) { - char *file = NULL; - int ret = -1; + VIR_AUTOFREE(char *) file = NULL; if (!(file = virFileCacheGetFileName(cache, name))) - return ret; + return -1; if (cache->handlers.saveFile(data, file, cache->priv) < 0) - goto cleanup; - - ret = 0; + return -1; - cleanup: - VIR_FREE(file); - return ret; + return 0; } @@ -347,7 +336,7 @@ virFileCacheLookupByFunc(virFileCachePtr cache, const void *iterData) { void *data = NULL; - char *name = NULL; + VIR_AUTOFREE(char *) name = NULL; virObjectLock(cache); @@ -357,8 +346,6 @@ virFileCacheLookupByFunc(virFileCachePtr cache, virObjectRef(data); virObjectUnlock(cache); - VIR_FREE(name); - return data; } -- 1.8.3.1
Modify code to use VIR_AUTOFREE macro wherever required. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virfirewall.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index 10c370a..568612c 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -512,7 +512,7 @@ void virFirewallRuleAddArgFormat(virFirewallPtr firewall, virFirewallRulePtr rule, const char *fmt, ...) { - char *arg; + VIR_AUTOFREE(char *) arg = NULL; va_list list; VIR_FIREWALL_RULE_RETURN_IF_ERROR(firewall, rule); @@ -526,13 +526,11 @@ void virFirewallRuleAddArgFormat(virFirewallPtr firewall, va_end(list); - VIR_FREE(arg); return; no_memory: firewall->err = ENOMEM; va_end(list); - VIR_FREE(arg); } @@ -679,7 +677,7 @@ virFirewallApplyRuleDirect(virFirewallRulePtr rule, virCommandPtr cmd = NULL; int status; int ret = -1; - char *error = NULL; + VIR_AUTOFREE(char *) error = NULL; if (!bin) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -715,7 +713,6 @@ virFirewallApplyRuleDirect(virFirewallRulePtr rule, ret = 0; cleanup: - VIR_FREE(error); virCommandFree(cmd); return ret; } @@ -808,12 +805,11 @@ virFirewallApplyRule(virFirewallPtr firewall, virFirewallRulePtr rule, bool ignoreErrors) { - char *output = NULL; + VIR_AUTOFREE(char *) output = NULL; char **lines = NULL; int ret = -1; - char *str = virFirewallRuleToString(rule); + VIR_AUTOFREE(char *) str = virFirewallRuleToString(rule); VIR_INFO("Applying rule '%s'", NULLSTR(str)); - VIR_FREE(str); if (rule->ignoreErrors) ignoreErrors = rule->ignoreErrors; @@ -858,7 +854,6 @@ virFirewallApplyRule(virFirewallPtr firewall, ret = 0; cleanup: virStringListFree(lines); - VIR_FREE(output); return ret; } -- 1.8.3.1
On Sun, Jun 03, 2018 at 01:42:15PM +0530, Sukrit Bhatnagar wrote:
Modify code to use VIR_AUTOFREE macro wherever required.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virfirewall.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)
...
@@ -808,12 +805,11 @@ virFirewallApplyRule(virFirewallPtr firewall, virFirewallRulePtr rule, bool ignoreErrors) { - char *output = NULL; + VIR_AUTOFREE(char *) output = NULL; char **lines = NULL; int ret = -1; - char *str = virFirewallRuleToString(rule); + VIR_AUTOFREE(char *) str = virFirewallRuleToString(rule);
let's couple ^this VIR_AUTOFREE with the one above... Erik
Modify code to use VIR_AUTOFREE macro wherever required. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virhook.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/util/virhook.c b/src/util/virhook.c index facd74a..51f0eb5 100644 --- a/src/util/virhook.c +++ b/src/util/virhook.c @@ -122,8 +122,7 @@ static int virHooksFound = -1; static int virHookCheck(int no, const char *driver) { - char *path; - int ret; + VIR_AUTOFREE(char *) path = NULL; if (driver == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -139,18 +138,15 @@ virHookCheck(int no, const char *driver) } if (!virFileExists(path)) { - ret = 0; VIR_DEBUG("No hook script %s", path); + return 0; } else if (!virFileIsExecutable(path)) { - ret = 0; VIR_WARN("Non-executable hook script %s", path); + return 0; } else { - ret = 1; VIR_DEBUG("Found hook script %s", path); + return 1; } - - VIR_FREE(path); - return ret; } /* @@ -233,7 +229,7 @@ virHookCall(int driver, char **output) { int ret; - char *path; + VIR_AUTOFREE(char *) path = NULL; virCommandPtr cmd; const char *drvstr; const char *opstr; @@ -318,7 +314,5 @@ virHookCall(int driver, virCommandFree(cmd); - VIR_FREE(path); - return ret; } -- 1.8.3.1
participants (2)
-
Erik Skultety -
Sukrit Bhatnagar