[libvirt] [PATCH 0/6] Introduce VIR_AUTOCLOSE macro for closing fd automatically
This series introduce VIR_AUTOCLOSE macro which can force close fd of the file automatically when the fd goes out of scope. It's used to eliminate VIR_FORCE_CLOSE in cleanup sections. The new macro consults VIR_AUTOFREE and VIR_AUTOPTR. Shi Lei (6): util: file: introduce VIR_AUTOCLOSE macro to close fd of the file automatically util: file: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup sections util: netdevbridge: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup sections util: netdev: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup sections phyp: driver: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup sections uml: conf: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup sections src/phyp/phyp_driver.c | 29 ++--- src/uml/uml_conf.c | 13 +- src/util/virfile.c | 21 +-- src/util/virfile.h | 20 ++- src/util/virnetdev.c | 253 +++++++++++++------------------------ src/util/virnetdevbridge.c | 120 ++++++------------ 6 files changed, 163 insertions(+), 293 deletions(-) -- 2.17.1
By making use of GNU C's cleanup attribute handled by the VIR_AUTOCLOSE macro, many of the VIR_FORCE_CLOSE calls can be dropped, which in turn leads to getting rid of many of our cleanup sections. Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/util/virfile.h | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/src/util/virfile.h b/src/util/virfile.h index b30a1d3..70e7203 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -54,6 +54,11 @@ int virFileClose(int *fdptr, virFileCloseFlags flags) int virFileFclose(FILE **file, bool preserve_errno) ATTRIBUTE_RETURN_CHECK; FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK; +static inline void virForceCloseHelper(int *_fd) +{ + ignore_value(virFileClose(_fd, VIR_FILE_CLOSE_PRESERVE_ERRNO)); +} + /* For use on normal paths; caller must check return value, and failure sets errno per close. */ # define VIR_CLOSE(FD) virFileClose(&(FD), 0) @@ -64,8 +69,7 @@ FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK; /* For use on cleanup paths; errno is unaffected by close, and no return value to worry about. */ -# define VIR_FORCE_CLOSE(FD) \ - ignore_value(virFileClose(&(FD), VIR_FILE_CLOSE_PRESERVE_ERRNO)) +# define VIR_FORCE_CLOSE(FD) virForceCloseHelper(&(FD)) # define VIR_FORCE_FCLOSE(FILE) ignore_value(virFileFclose(&(FILE), true)) /* Similar VIR_FORCE_CLOSE() but ignores EBADF errors since they are expected @@ -80,6 +84,18 @@ FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK; VIR_FILE_CLOSE_PRESERVE_ERRNO | \ VIR_FILE_CLOSE_DONT_LOG)) +/** + * VIR_AUTOCLOSE: + * @fd: fd of the file to be closed automatically + * + * Macro to automatically force close the fd by calling virForceCloseHelper + * when the fd goes out of scope. It's used to eliminate VIR_FORCE_CLOSE + * in cleanup sections. + */ +# define VIR_AUTOCLOSE(fd) \ + __attribute__((cleanup(virForceCloseHelper))) int fd = -1 + + /* Opaque type for managing a wrapper around a fd. */ struct _virFileWrapperFd; -- 2.17.1
On 09/10/2018 05:47 AM, Shi Lei wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOCLOSE macro, many of the VIR_FORCE_CLOSE calls can be dropped, which in turn leads to getting rid of many of our cleanup sections.
Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/util/virfile.h | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/util/virfile.h b/src/util/virfile.h index b30a1d3..70e7203 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -54,6 +54,11 @@ int virFileClose(int *fdptr, virFileCloseFlags flags) int virFileFclose(FILE **file, bool preserve_errno) ATTRIBUTE_RETURN_CHECK; FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK;
+static inline void virForceCloseHelper(int *_fd)
No need for this argument to have underscore in its name.
+{ + ignore_value(virFileClose(_fd, VIR_FILE_CLOSE_PRESERVE_ERRNO)); +} + /* For use on normal paths; caller must check return value, and failure sets errno per close. */ # define VIR_CLOSE(FD) virFileClose(&(FD), 0) @@ -64,8 +69,7 @@ FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK;
/* For use on cleanup paths; errno is unaffected by close, and no return value to worry about. */ -# define VIR_FORCE_CLOSE(FD) \ - ignore_value(virFileClose(&(FD), VIR_FILE_CLOSE_PRESERVE_ERRNO)) +# define VIR_FORCE_CLOSE(FD) virForceCloseHelper(&(FD)) # define VIR_FORCE_FCLOSE(FILE) ignore_value(virFileFclose(&(FILE), true))
/* Similar VIR_FORCE_CLOSE() but ignores EBADF errors since they are expected @@ -80,6 +84,18 @@ FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK; VIR_FILE_CLOSE_PRESERVE_ERRNO | \ VIR_FILE_CLOSE_DONT_LOG))
+/** + * VIR_AUTOCLOSE: + * @fd: fd of the file to be closed automatically + * + * Macro to automatically force close the fd by calling virForceCloseHelper + * when the fd goes out of scope. It's used to eliminate VIR_FORCE_CLOSE + * in cleanup sections. + */ +# define VIR_AUTOCLOSE(fd) \ + __attribute__((cleanup(virForceCloseHelper))) int fd = -1
While this may helps us to initialize variables correctly, I think we should do that explicitly. Not only it follows what VIR_AUTOFREE is doing, it also is more visible when used. For instance, in 2/6 when the macro is used for the first time, it's not visible what is @fd initialized to.
+ + /* Opaque type for managing a wrapper around a fd. */ struct _virFileWrapperFd;
Otherwise the rest of the series looks good. Michal
On 2018-09-11 at 20:22, Michal Privoznik wrote:
On 09/10/2018 05:47 AM, Shi Lei wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOCLOSE macro, many of the VIR_FORCE_CLOSE calls can be dropped, which in turn leads to getting rid of many of our cleanup sections. Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/util/virfile.h | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/util/virfile.h b/src/util/virfile.h index b30a1d3..70e7203 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -54,6 +54,11 @@ int virFileClose(int *fdptr, virFileCloseFlags flags) int virFileFclose(FILE **file, bool preserve_errno) ATTRIBUTE_RETURN_CHECK; FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK; +static inline void virForceCloseHelper(int *_fd)
No need for this argument to have underscore in its name.
Okay.
+{ + ignore_value(virFileClose(_fd, VIR_FILE_CLOSE_PRESERVE_ERRNO)); +} + /* For use on normal paths; caller must check return value, and failure sets errno per close. */ # define VIR_CLOSE(FD) virFileClose(&(FD), 0) @@ -64,8 +69,7 @@ FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK; /* For use on cleanup paths; errno is unaffected by close, and no return value to worry about. */ -# define VIR_FORCE_CLOSE(FD) \ - ignore_value(virFileClose(&(FD), VIR_FILE_CLOSE_PRESERVE_ERRNO)) +# define VIR_FORCE_CLOSE(FD) virForceCloseHelper(&(FD)) # define VIR_FORCE_FCLOSE(FILE) ignore_value(virFileFclose(&(FILE), true)) /* Similar VIR_FORCE_CLOSE() but ignores EBADF errors since they are expected @@ -80,6 +84,18 @@ FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK; VIR_FILE_CLOSE_PRESERVE_ERRNO | \ VIR_FILE_CLOSE_DONT_LOG)) +/** + * VIR_AUTOCLOSE: + * @fd: fd of the file to be closed automatically + * + * Macro to automatically force close the fd by calling virForceCloseHelper + * when the fd goes out of scope. It's used to eliminate VIR_FORCE_CLOSE + * in cleanup sections. + */ +# define VIR_AUTOCLOSE(fd) \ + __attribute__((cleanup(virForceCloseHelper))) int fd = -1
While this may helps us to initialize variables correctly, I think we should do that explicitly. Not only it follows what VIR_AUTOFREE is doing, it also is more visible when used. For instance, in 2/6 when the macro is used for the first time, it's not visible what is @fd initialized to.
Okay. So I think the macro could be like: # define VIR_AUTOCLOSE \ __attribute__((cleanup(virForceCloseHelper))) int And the statement is like: VIR_AUTOCLOSE fd = -1; VIR_AUTOCLOSE sock_fd = socket(VIR_NETDEV_FAMILY, SOCK_DGRAM, 0); Also, I think I should add a new syntax-check rule to ensure the initialization of the variable. Just like sc_require_attribute_cleanup_initialization for VIR_AUTO(FREE|PTR).
+ + /* Opaque type for managing a wrapper around a fd. */ struct _virFileWrapperFd;
Otherwise the rest of the series looks good.
Michal
Thanks for your comments. Shi Lei
On Tue, Sep 11, 2018 at 10:31:18PM +0800, Shi Lei wrote:
On 2018-09-11 at 20:22, Michal Privoznik wrote:
On 09/10/2018 05:47 AM, Shi Lei wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOCLOSE macro, many of the VIR_FORCE_CLOSE calls can be dropped, which in turn leads to getting rid of many of our cleanup sections.
Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/util/virfile.h | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/util/virfile.h b/src/util/virfile.h index b30a1d3..70e7203 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -54,6 +54,11 @@ int virFileClose(int *fdptr, virFileCloseFlags flags) int virFileFclose(FILE **file, bool preserve_errno) ATTRIBUTE_RETURN_CHECK; FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK;
+static inline void virForceCloseHelper(int *_fd)
No need for this argument to have underscore in its name.
Okay.
+{ + ignore_value(virFileClose(_fd, VIR_FILE_CLOSE_PRESERVE_ERRNO)); +} + /* For use on normal paths; caller must check return value, and failure sets errno per close. */ # define VIR_CLOSE(FD) virFileClose(&(FD), 0) @@ -64,8 +69,7 @@ FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK;
/* For use on cleanup paths; errno is unaffected by close, and no return value to worry about. */ -# define VIR_FORCE_CLOSE(FD) \ - ignore_value(virFileClose(&(FD), VIR_FILE_CLOSE_PRESERVE_ERRNO)) +# define VIR_FORCE_CLOSE(FD) virForceCloseHelper(&(FD)) # define VIR_FORCE_FCLOSE(FILE) ignore_value(virFileFclose(&(FILE), true))
/* Similar VIR_FORCE_CLOSE() but ignores EBADF errors since they are expected @@ -80,6 +84,18 @@ FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK; VIR_FILE_CLOSE_PRESERVE_ERRNO | \ VIR_FILE_CLOSE_DONT_LOG))
+/** + * VIR_AUTOCLOSE: + * @fd: fd of the file to be closed automatically + * + * Macro to automatically force close the fd by calling virForceCloseHelper + * when the fd goes out of scope. It's used to eliminate VIR_FORCE_CLOSE + * in cleanup sections. + */ +# define VIR_AUTOCLOSE(fd) \ + __attribute__((cleanup(virForceCloseHelper))) int fd = -1
While this may helps us to initialize variables correctly, I think we should do that explicitly. Not only it follows what VIR_AUTOFREE is doing, it also is more visible when used. For instance, in 2/6 when the macro is used for the first time, it's not visible what is @fd initialized to.
Okay. So I think the macro could be like:
# define VIR_AUTOCLOSE \ __attribute__((cleanup(virForceCloseHelper))) int
Actually, I'd prefer if we stayed consistent with the existing AUTO_ macros, IOW, the form of VIR_AUTOCLOSE(fd) = <rvalue> is IMHO desired.
And the statement is like:
VIR_AUTOCLOSE fd = -1; VIR_AUTOCLOSE sock_fd = socket(VIR_NETDEV_FAMILY, SOCK_DGRAM, 0);
Also, I think I should add a new syntax-check rule to ensure the initialization of the variable. Just like sc_require_attribute_cleanup_initialization for VIR_AUTO(FREE|PTR).
Yep, I agree with adding a similar syntax-check rule. Erik
On 2018-09-12 at 15:18, Erik Skultety wrote:
On Tue, Sep 11, 2018 at 10:31:18PM +0800, Shi Lei wrote:
On 2018-09-11 at 20:22, Michal Privoznik wrote:
On 09/10/2018 05:47 AM, Shi Lei wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOCLOSE macro, many of the VIR_FORCE_CLOSE calls can be dropped, which in turn leads to getting rid of many of our cleanup sections.
Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/util/virfile.h | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/util/virfile.h b/src/util/virfile.h index b30a1d3..70e7203 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -54,6 +54,11 @@ int virFileClose(int *fdptr, virFileCloseFlags flags) int virFileFclose(FILE **file, bool preserve_errno) ATTRIBUTE_RETURN_CHECK; FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK;
+static inline void virForceCloseHelper(int *_fd)
No need for this argument to have underscore in its name.
Okay.
+{ + ignore_value(virFileClose(_fd, VIR_FILE_CLOSE_PRESERVE_ERRNO)); +} + /* For use on normal paths; caller must check return value, and failure sets errno per close. */ # define VIR_CLOSE(FD) virFileClose(&(FD), 0) @@ -64,8 +69,7 @@ FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK;
/* For use on cleanup paths; errno is unaffected by close, and no return value to worry about. */ -# define VIR_FORCE_CLOSE(FD) \ - ignore_value(virFileClose(&(FD), VIR_FILE_CLOSE_PRESERVE_ERRNO)) +# define VIR_FORCE_CLOSE(FD) virForceCloseHelper(&(FD)) # define VIR_FORCE_FCLOSE(FILE) ignore_value(virFileFclose(&(FILE), true))
/* Similar VIR_FORCE_CLOSE() but ignores EBADF errors since they are expected @@ -80,6 +84,18 @@ FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK; VIR_FILE_CLOSE_PRESERVE_ERRNO | \ VIR_FILE_CLOSE_DONT_LOG))
+/** + * VIR_AUTOCLOSE: + * @fd: fd of the file to be closed automatically + * + * Macro to automatically force close the fd by calling virForceCloseHelper + * when the fd goes out of scope. It's used to eliminate VIR_FORCE_CLOSE + * in cleanup sections. + */ +# define VIR_AUTOCLOSE(fd) \ + __attribute__((cleanup(virForceCloseHelper))) int fd = -1
While this may helps us to initialize variables correctly, I think we should do that explicitly. Not only it follows what VIR_AUTOFREE is doing, it also is more visible when used. For instance, in 2/6 when the macro is used for the first time, it's not visible what is @fd initialized to.
Okay. So I think the macro could be like:
# define VIR_AUTOCLOSE \ __attribute__((cleanup(virForceCloseHelper))) int
Actually, I'd prefer if we stayed consistent with the existing AUTO_ macros, IOW, the form of VIR_AUTOCLOSE(fd) = <rvalue> is IMHO desired.
Okay. Then the syntax-check rule would be simpler.
And the statement is like:
VIR_AUTOCLOSE fd = -1; VIR_AUTOCLOSE sock_fd = socket(VIR_NETDEV_FAMILY, SOCK_DGRAM, 0);
Also, I think I should add a new syntax-check rule to ensure the initialization of the variable. Just like sc_require_attribute_cleanup_initialization for VIR_AUTO(FREE|PTR).
Yep, I agree with adding a similar syntax-check rule.
Erik
Okay. Just now, I find that we do not need to add a new rule. A minor change on sc_require_attribute_cleanup_initialization can work. Shi Lei
On Wed, Sep 12, 2018 at 04:04:04PM +0800, Shi Lei wrote:
On 2018-09-12 at 15:18, Erik Skultety wrote:
On Tue, Sep 11, 2018 at 10:31:18PM +0800, Shi Lei wrote:
On 2018-09-11 at 20:22, Michal Privoznik wrote:
On 09/10/2018 05:47 AM, Shi Lei wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOCLOSE macro, many of the VIR_FORCE_CLOSE calls can be dropped, which in turn leads to getting rid of many of our cleanup sections.
Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/util/virfile.h | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/util/virfile.h b/src/util/virfile.h index b30a1d3..70e7203 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -54,6 +54,11 @@ int virFileClose(int *fdptr, virFileCloseFlags flags) int virFileFclose(FILE **file, bool preserve_errno) ATTRIBUTE_RETURN_CHECK; FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK;
+static inline void virForceCloseHelper(int *_fd)
No need for this argument to have underscore in its name.
Okay.
+{ + ignore_value(virFileClose(_fd, VIR_FILE_CLOSE_PRESERVE_ERRNO)); +} + /* For use on normal paths; caller must check return value, and failure sets errno per close. */ # define VIR_CLOSE(FD) virFileClose(&(FD), 0) @@ -64,8 +69,7 @@ FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK;
/* For use on cleanup paths; errno is unaffected by close, and no return value to worry about. */ -# define VIR_FORCE_CLOSE(FD) \ - ignore_value(virFileClose(&(FD), VIR_FILE_CLOSE_PRESERVE_ERRNO)) +# define VIR_FORCE_CLOSE(FD) virForceCloseHelper(&(FD)) # define VIR_FORCE_FCLOSE(FILE) ignore_value(virFileFclose(&(FILE), true))
/* Similar VIR_FORCE_CLOSE() but ignores EBADF errors since they are expected @@ -80,6 +84,18 @@ FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK; VIR_FILE_CLOSE_PRESERVE_ERRNO | \ VIR_FILE_CLOSE_DONT_LOG))
+/** + * VIR_AUTOCLOSE: + * @fd: fd of the file to be closed automatically + * + * Macro to automatically force close the fd by calling virForceCloseHelper + * when the fd goes out of scope. It's used to eliminate VIR_FORCE_CLOSE + * in cleanup sections. + */ +# define VIR_AUTOCLOSE(fd) \ + __attribute__((cleanup(virForceCloseHelper))) int fd = -1
While this may helps us to initialize variables correctly, I think we should do that explicitly. Not only it follows what VIR_AUTOFREE is doing, it also is more visible when used. For instance, in 2/6 when the macro is used for the first time, it's not visible what is @fd initialized to.
Okay. So I think the macro could be like:
# define VIR_AUTOCLOSE \ __attribute__((cleanup(virForceCloseHelper))) int
Actually, I'd prefer if we stayed consistent with the existing AUTO_ macros, IOW, the form of VIR_AUTOCLOSE(fd) = <rvalue> is IMHO desired.
Okay. Then the syntax-check rule would be simpler.
And the statement is like:
VIR_AUTOCLOSE fd = -1; VIR_AUTOCLOSE sock_fd = socket(VIR_NETDEV_FAMILY, SOCK_DGRAM, 0);
Also, I think I should add a new syntax-check rule to ensure the initialization of the variable. Just like sc_require_attribute_cleanup_initialization for VIR_AUTO(FREE|PTR).
Yep, I agree with adding a similar syntax-check rule.
Erik
Okay. Just now, I find that we do not need to add a new rule. A minor change on sc_require_attribute_cleanup_initialization can work.
Even better. Regards, Erik
On 09/12/2018 09:18 AM, Erik Skultety wrote:
On Tue, Sep 11, 2018 at 10:31:18PM +0800, Shi Lei wrote:
On 2018-09-11 at 20:22, Michal Privoznik wrote:
On 09/10/2018 05:47 AM, Shi Lei wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOCLOSE macro, many of the VIR_FORCE_CLOSE calls can be dropped, which in turn leads to getting rid of many of our cleanup sections.
Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/util/virfile.h | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/util/virfile.h b/src/util/virfile.h index b30a1d3..70e7203 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -54,6 +54,11 @@ int virFileClose(int *fdptr, virFileCloseFlags flags) int virFileFclose(FILE **file, bool preserve_errno) ATTRIBUTE_RETURN_CHECK; FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK;
+static inline void virForceCloseHelper(int *_fd)
No need for this argument to have underscore in its name.
Okay.
+{ + ignore_value(virFileClose(_fd, VIR_FILE_CLOSE_PRESERVE_ERRNO)); +} + /* For use on normal paths; caller must check return value, and failure sets errno per close. */ # define VIR_CLOSE(FD) virFileClose(&(FD), 0) @@ -64,8 +69,7 @@ FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK;
/* For use on cleanup paths; errno is unaffected by close, and no return value to worry about. */ -# define VIR_FORCE_CLOSE(FD) \ - ignore_value(virFileClose(&(FD), VIR_FILE_CLOSE_PRESERVE_ERRNO)) +# define VIR_FORCE_CLOSE(FD) virForceCloseHelper(&(FD)) # define VIR_FORCE_FCLOSE(FILE) ignore_value(virFileFclose(&(FILE), true))
/* Similar VIR_FORCE_CLOSE() but ignores EBADF errors since they are expected @@ -80,6 +84,18 @@ FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK; VIR_FILE_CLOSE_PRESERVE_ERRNO | \ VIR_FILE_CLOSE_DONT_LOG))
+/** + * VIR_AUTOCLOSE: + * @fd: fd of the file to be closed automatically + * + * Macro to automatically force close the fd by calling virForceCloseHelper + * when the fd goes out of scope. It's used to eliminate VIR_FORCE_CLOSE + * in cleanup sections. + */ +# define VIR_AUTOCLOSE(fd) \ + __attribute__((cleanup(virForceCloseHelper))) int fd = -1
While this may helps us to initialize variables correctly, I think we should do that explicitly. Not only it follows what VIR_AUTOFREE is doing, it also is more visible when used. For instance, in 2/6 when the macro is used for the first time, it's not visible what is @fd initialized to.
Okay. So I think the macro could be like:
# define VIR_AUTOCLOSE \ __attribute__((cleanup(virForceCloseHelper))) int
Actually, I'd prefer if we stayed consistent with the existing AUTO_ macros, IOW, the form of VIR_AUTOCLOSE(fd) = <rvalue> is IMHO desired.
I don't think this is correct. Sure, we do have parentheses in VIR_AUTOFREE but only for type, not variable itself: VIR_AUTOFREE(char *) str = NULL; Therefore, in order to be consistent the autoclose macro should look like I'm suggesting actually: VIR_AUTOCLOSE fd = -1; To extend this idea further, we can then have VIR_AUTOFCLOSE and VIR_AUTODIRCLOSE macros to call fclose() and dirclose() respectively (or our wrappers around those functions). Michal
On Wed, Sep 12, 2018 at 10:32:04AM +0200, Michal Privoznik wrote:
On 09/12/2018 09:18 AM, Erik Skultety wrote:
On Tue, Sep 11, 2018 at 10:31:18PM +0800, Shi Lei wrote:
On 2018-09-11 at 20:22, Michal Privoznik wrote:
On 09/10/2018 05:47 AM, Shi Lei wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOCLOSE macro, many of the VIR_FORCE_CLOSE calls can be dropped, which in turn leads to getting rid of many of our cleanup sections.
Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/util/virfile.h | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/util/virfile.h b/src/util/virfile.h index b30a1d3..70e7203 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -54,6 +54,11 @@ int virFileClose(int *fdptr, virFileCloseFlags flags) int virFileFclose(FILE **file, bool preserve_errno) ATTRIBUTE_RETURN_CHECK; FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK;
+static inline void virForceCloseHelper(int *_fd)
No need for this argument to have underscore in its name.
Okay.
+{ + ignore_value(virFileClose(_fd, VIR_FILE_CLOSE_PRESERVE_ERRNO)); +} + /* For use on normal paths; caller must check return value, and failure sets errno per close. */ # define VIR_CLOSE(FD) virFileClose(&(FD), 0) @@ -64,8 +69,7 @@ FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK;
/* For use on cleanup paths; errno is unaffected by close, and no return value to worry about. */ -# define VIR_FORCE_CLOSE(FD) \ - ignore_value(virFileClose(&(FD), VIR_FILE_CLOSE_PRESERVE_ERRNO)) +# define VIR_FORCE_CLOSE(FD) virForceCloseHelper(&(FD)) # define VIR_FORCE_FCLOSE(FILE) ignore_value(virFileFclose(&(FILE), true))
/* Similar VIR_FORCE_CLOSE() but ignores EBADF errors since they are expected @@ -80,6 +84,18 @@ FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK; VIR_FILE_CLOSE_PRESERVE_ERRNO | \ VIR_FILE_CLOSE_DONT_LOG))
+/** + * VIR_AUTOCLOSE: + * @fd: fd of the file to be closed automatically + * + * Macro to automatically force close the fd by calling virForceCloseHelper + * when the fd goes out of scope. It's used to eliminate VIR_FORCE_CLOSE + * in cleanup sections. + */ +# define VIR_AUTOCLOSE(fd) \ + __attribute__((cleanup(virForceCloseHelper))) int fd = -1
While this may helps us to initialize variables correctly, I think we should do that explicitly. Not only it follows what VIR_AUTOFREE is doing, it also is more visible when used. For instance, in 2/6 when the macro is used for the first time, it's not visible what is @fd initialized to.
Okay. So I think the macro could be like:
# define VIR_AUTOCLOSE \ __attribute__((cleanup(virForceCloseHelper))) int
Actually, I'd prefer if we stayed consistent with the existing AUTO_ macros, IOW, the form of VIR_AUTOCLOSE(fd) = <rvalue> is IMHO desired.
I don't think this is correct. Sure, we do have parentheses in VIR_AUTOFREE but only for type, not variable itself:
Oh, right, I missed that fact, sorry for the noise.
VIR_AUTOFREE(char *) str = NULL;
Therefore, in order to be consistent the autoclose macro should look like I'm suggesting actually:
VIR_AUTOCLOSE fd = -1;
To extend this idea further, we can then have VIR_AUTOFCLOSE and VIR_AUTODIRCLOSE macros to call fclose() and dirclose() respectively (or our wrappers around those functions).
I believe the general consensus is to always use our wrappers. I'm thinking whether this could be made more generic, but that would just introduce more unnecessary black magic, so I'm fine with your suggestion for the time being, we might come up with something better eventually. Erik
On 2018-09-12 at 16:37, Erik Skultety wrote:
On Wed, Sep 12, 2018 at 10:32:04AM +0200, Michal Privoznik wrote:
On 09/12/2018 09:18 AM, Erik Skultety wrote:
On Tue, Sep 11, 2018 at 10:31:18PM +0800, Shi Lei wrote:
On 2018-09-11 at 20:22, Michal Privoznik wrote:
On 09/10/2018 05:47 AM, Shi Lei wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOCLOSE macro, many of the VIR_FORCE_CLOSE calls can be dropped, which in turn leads to getting rid of many of our cleanup sections.
Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/util/virfile.h | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/src/util/virfile.h b/src/util/virfile.h index b30a1d3..70e7203 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -54,6 +54,11 @@ int virFileClose(int *fdptr, virFileCloseFlags flags) int virFileFclose(FILE **file, bool preserve_errno) ATTRIBUTE_RETURN_CHECK; FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK;
+static inline void virForceCloseHelper(int *_fd)
No need for this argument to have underscore in its name.
Okay.
+{ + ignore_value(virFileClose(_fd, VIR_FILE_CLOSE_PRESERVE_ERRNO)); +} + /* For use on normal paths; caller must check return value, and failure sets errno per close. */ # define VIR_CLOSE(FD) virFileClose(&(FD), 0) @@ -64,8 +69,7 @@ FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK;
/* For use on cleanup paths; errno is unaffected by close, and no return value to worry about. */ -# define VIR_FORCE_CLOSE(FD) \ - ignore_value(virFileClose(&(FD), VIR_FILE_CLOSE_PRESERVE_ERRNO)) +# define VIR_FORCE_CLOSE(FD) virForceCloseHelper(&(FD)) # define VIR_FORCE_FCLOSE(FILE) ignore_value(virFileFclose(&(FILE), true))
/* Similar VIR_FORCE_CLOSE() but ignores EBADF errors since they are expected @@ -80,6 +84,18 @@ FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK; VIR_FILE_CLOSE_PRESERVE_ERRNO | \ VIR_FILE_CLOSE_DONT_LOG))
+/** + * VIR_AUTOCLOSE: + * @fd: fd of the file to be closed automatically + * + * Macro to automatically force close the fd by calling virForceCloseHelper + * when the fd goes out of scope. It's used to eliminate VIR_FORCE_CLOSE + * in cleanup sections. + */ +# define VIR_AUTOCLOSE(fd) \ + __attribute__((cleanup(virForceCloseHelper))) int fd = -1
While this may helps us to initialize variables correctly, I think we should do that explicitly. Not only it follows what VIR_AUTOFREE is doing, it also is more visible when used. For instance, in 2/6 when the macro is used for the first time, it's not visible what is @fd initialized to.
Okay. So I think the macro could be like:
# define VIR_AUTOCLOSE \ __attribute__((cleanup(virForceCloseHelper))) int
Actually, I'd prefer if we stayed consistent with the existing AUTO_ macros, IOW, the form of VIR_AUTOCLOSE(fd) = <rvalue> is IMHO desired.
I don't think this is correct. Sure, we do have parentheses in VIR_AUTOFREE but only for type, not variable itself:
Oh, right, I missed that fact, sorry for the noise.
VIR_AUTOFREE(char *) str = NULL;
Therefore, in order to be consistent the autoclose macro should look like I'm suggesting actually:
VIR_AUTOCLOSE fd = -1;
To extend this idea further, we can then have VIR_AUTOFCLOSE and VIR_AUTODIRCLOSE macros to call fclose() and dirclose() respectively (or our wrappers around those functions).
I believe the general consensus is to always use our wrappers. I'm thinking whether this could be made more generic, but that would just introduce more unnecessary black magic, so I'm fine with your suggestion for the time being, we might come up with something better eventually.
Erik
Okay. So, I would follow the suggestion from both of you and post the v2 series. Shi Lei
By making use of GNU C's cleanup attribute handled by the VIR_AUTOCLOSE macro, many of the VIR_FORCE_CLOSE calls can be dropped, which in turn leads to getting rid of many of our cleanup sections. Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/util/virfile.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 01ebdb6..e63fada 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1969,29 +1969,22 @@ int virFileIsCDROM(const char *path) { struct stat st; - int fd; - int ret = -1; + VIR_AUTOCLOSE(fd); if ((fd = open(path, O_RDONLY | O_NONBLOCK)) < 0) - goto cleanup; + return -1; if (fstat(fd, &st) < 0) - goto cleanup; + return -1; - if (!S_ISBLK(st.st_mode)) { - ret = 0; - goto cleanup; - } + if (!S_ISBLK(st.st_mode)) + return 0; /* Attempt to detect via a CDROM specific ioctl */ if (ioctl(fd, CDROM_DRIVE_STATUS, CDSL_CURRENT) >= 0) - ret = 1; - else - ret = 0; + return 1; - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return 0; } #else -- 2.17.1
By making use of GNU C's cleanup attribute handled by the VIR_AUTOCLOSE macro, many of the VIR_FORCE_CLOSE calls can be dropped, which in turn leads to getting rid of many of our cleanup sections. Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/util/virnetdevbridge.c | 120 ++++++++++++------------------------- 1 file changed, 37 insertions(+), 83 deletions(-) diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c index bc377b5..d8528e1 100644 --- a/src/util/virnetdevbridge.c +++ b/src/util/virnetdevbridge.c @@ -81,9 +81,8 @@ static int virNetDevBridgeCmd(const char *brname, void *arg, size_t argsize) { - int s; - int ret = -1; struct ifdrv ifd; + VIR_AUTOCLOSE(s); memset(&ifd, 0, sizeof(ifd)); @@ -97,19 +96,14 @@ static int virNetDevBridgeCmd(const char *brname, virReportSystemError(ERANGE, _("Network interface name '%s' is too long"), brname); - goto cleanup; + return -1; } ifd.ifd_cmd = op; ifd.ifd_len = argsize; ifd.ifd_data = arg; - ret = ioctl(s, SIOCSDRVSPEC, &ifd); - - cleanup: - VIR_FORCE_CLOSE(s); - - return ret; + return ioctl(s, SIOCSDRVSPEC, &ifd); } #endif @@ -167,10 +161,9 @@ static int virNetDevBridgeGet(const char *brname, const char *paramname, /* sysfs param name */ unsigned long *value) /* current value */ { - int ret = -1; - int fd = -1; struct ifreq ifr; VIR_AUTOFREE(char *) path = NULL; + VIR_AUTOCLOSE(fd); if (virAsprintf(&path, SYSFS_NET_DIR "%s/bridge/%s", brname, paramname) < 0) return -1; @@ -180,26 +173,26 @@ static int virNetDevBridgeGet(const char *brname, if (virFileReadAll(path, INT_BUFSIZE_BOUND(unsigned long), &valuestr) < 0) - goto cleanup; + return -1; if (virStrToLong_ul(valuestr, NULL, 10, value) < 0) { virReportSystemError(EINVAL, _("Unable to get bridge %s %s"), brname, paramname); - goto cleanup; + return -1; } } else { struct __bridge_info info; unsigned long args[] = { BRCTL_GET_BRIDGE_INFO, (unsigned long)&info, 0, 0 }; if ((fd = virNetDevSetupControl(brname, &ifr)) < 0) - goto cleanup; + return -1; ifr.ifr_data = (char*)&args; if (ioctl(fd, SIOCDEVPRIVATE, ifr) < 0) { virReportSystemError(errno, _("Unable to get bridge %s %s"), brname, paramname); - goto cleanup; + return -1; } if (STREQ(paramname, "stp_state")) { @@ -209,14 +202,11 @@ static int virNetDevBridgeGet(const char *brname, } else { virReportSystemError(EINVAL, _("Unable to get bridge %s %s"), brname, paramname); - goto cleanup; + return -1; } } - ret = 0; - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return 0; } #endif /* __linux__ */ @@ -391,8 +381,7 @@ virNetDevBridgePortSetUnicastFlood(const char *brname ATTRIBUTE_UNUSED, static int virNetDevBridgeCreateWithIoctl(const char *brname) { - int fd = -1; - int ret = -1; + VIR_AUTOCLOSE(fd); if ((fd = virNetDevSetupControl(NULL, NULL)) < 0) return -1; @@ -400,14 +389,10 @@ virNetDevBridgeCreateWithIoctl(const char *brname) if (ioctl(fd, SIOCBRADDBR, brname) < 0) { virReportSystemError(errno, _("Unable to create bridge %s"), brname); - goto cleanup; + return -1; } - ret = 0; - - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return 0; } #endif @@ -503,9 +488,8 @@ virNetDevBridgeCreate(const char *brname) int virNetDevBridgeCreate(const char *brname) { - int s; struct ifreq ifr; - int ret = - 1; + VIR_AUTOCLOSE(s); if ((s = virNetDevSetupControl("bridge", &ifr)) < 0) return -1; @@ -513,16 +497,13 @@ virNetDevBridgeCreate(const char *brname) if (ioctl(s, SIOCIFCREATE2, &ifr) < 0) { virReportSystemError(errno, "%s", _("Unable to create bridge device")); - goto cleanup; + return -1; } if (virNetDevSetName(ifr.ifr_name, brname) == -1) - goto cleanup; + return -1; - ret = 0; - cleanup: - VIR_FORCE_CLOSE(s); - return ret; + return 0; } #else int virNetDevBridgeCreate(const char *brname) @@ -545,8 +526,7 @@ int virNetDevBridgeCreate(const char *brname) static int virNetDevBridgeDeleteWithIoctl(const char *brname) { - int fd = -1; - int ret = -1; + VIR_AUTOCLOSE(fd); ignore_value(virNetDevSetOnline(brname, false)); @@ -556,14 +536,10 @@ virNetDevBridgeDeleteWithIoctl(const char *brname) if (ioctl(fd, SIOCBRDELBR, brname) < 0) { virReportSystemError(errno, _("Unable to delete bridge %s"), brname); - goto cleanup; + return -1; } - ret = 0; - - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return 0; } #endif @@ -596,9 +572,8 @@ virNetDevBridgeDelete(const char *brname) int virNetDevBridgeDelete(const char *brname) { - int s; struct ifreq ifr; - int ret = -1; + VIR_AUTOCLOSE(s); if ((s = virNetDevSetupControl(brname, &ifr)) < 0) return -1; @@ -607,13 +582,10 @@ virNetDevBridgeDelete(const char *brname) virReportSystemError(errno, _("Unable to remove bridge %s"), brname); - goto cleanup; + return -1; } - ret = 0; - cleanup: - VIR_FORCE_CLOSE(s); - return ret; + return 0; } #else int virNetDevBridgeDelete(const char *brname ATTRIBUTE_UNUSED) @@ -637,9 +609,8 @@ int virNetDevBridgeDelete(const char *brname ATTRIBUTE_UNUSED) int virNetDevBridgeAddPort(const char *brname, const char *ifname) { - int fd = -1; - int ret = -1; struct ifreq ifr; + VIR_AUTOCLOSE(fd); if ((fd = virNetDevSetupControl(brname, &ifr)) < 0) return -1; @@ -647,19 +618,16 @@ int virNetDevBridgeAddPort(const char *brname, if (!(ifr.ifr_ifindex = if_nametoindex(ifname))) { virReportSystemError(ENODEV, _("Unable to get interface index for %s"), ifname); - goto cleanup; + return -1; } if (ioctl(fd, SIOCBRADDIF, &ifr) < 0) { virReportSystemError(errno, _("Unable to add bridge %s port %s"), brname, ifname); - goto cleanup; + return -1; } - ret = 0; - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return 0; } #elif defined(HAVE_BSD_BRIDGE_MGMT) int virNetDevBridgeAddPort(const char *brname, @@ -706,9 +674,8 @@ int virNetDevBridgeAddPort(const char *brname, int virNetDevBridgeRemovePort(const char *brname, const char *ifname) { - int fd = -1; - int ret = -1; struct ifreq ifr; + VIR_AUTOCLOSE(fd); if ((fd = virNetDevSetupControl(brname, &ifr)) < 0) return -1; @@ -717,19 +684,16 @@ int virNetDevBridgeRemovePort(const char *brname, virReportSystemError(ENODEV, _("Unable to get interface index for %s"), ifname); - goto cleanup; + return -1; } if (ioctl(fd, SIOCBRDELIF, &ifr) < 0) { virReportSystemError(errno, _("Unable to remove bridge %s port %s"), brname, ifname); - goto cleanup; + return -1; } - ret = 0; - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return 0; } #elif defined(HAVE_BSD_BRIDGE_MGMT) int virNetDevBridgeRemovePort(const char *brname, @@ -778,19 +742,14 @@ int virNetDevBridgeRemovePort(const char *brname, int virNetDevBridgeSetSTPDelay(const char *brname, int delay) { - int fd = -1; - int ret = -1; struct ifreq ifr; + VIR_AUTOCLOSE(fd); if ((fd = virNetDevSetupControl(brname, &ifr)) < 0) - goto cleanup; + return -1; - ret = virNetDevBridgeSet(brname, "forward_delay", MS_TO_JIFFIES(delay), + return virNetDevBridgeSet(brname, "forward_delay", MS_TO_JIFFIES(delay), fd, &ifr); - - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; } @@ -831,19 +790,14 @@ int virNetDevBridgeGetSTPDelay(const char *brname, int virNetDevBridgeSetSTP(const char *brname, bool enable) { - int fd = -1; - int ret = -1; struct ifreq ifr; + VIR_AUTOCLOSE(fd); if ((fd = virNetDevSetupControl(brname, &ifr)) < 0) - goto cleanup; + return -1; - ret = virNetDevBridgeSet(brname, "stp_state", enable ? 1 : 0, + return virNetDevBridgeSet(brname, "stp_state", enable ? 1 : 0, fd, &ifr); - - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; } -- 2.17.1
By making use of GNU C's cleanup attribute handled by the VIR_AUTOCLOSE macro, many of the VIR_FORCE_CLOSE calls can be dropped, which in turn leads to getting rid of many of our cleanup sections. Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/util/virnetdev.c | 253 +++++++++++++++---------------------------- 1 file changed, 87 insertions(+), 166 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 5d4ad24..7f43f15 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -200,27 +200,22 @@ virNetDevSetupControl(const char *ifname ATTRIBUTE_UNUSED, */ int virNetDevExists(const char *ifname) { - int fd = -1; - int ret = -1; struct ifreq ifr; + VIR_AUTOCLOSE(fd); if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return -1; if (ioctl(fd, SIOCGIFFLAGS, &ifr)) { if (errno == ENODEV || errno == ENXIO) - ret = 0; - else - virReportSystemError(errno, - _("Unable to check interface flags for %s"), ifname); - goto cleanup; - } + return 0; - ret = 1; + virReportSystemError(errno, _("Unable to check interface flags for %s"), + ifname); + return -1; + } - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return 1; } #else int virNetDevExists(const char *ifname) @@ -251,20 +246,20 @@ virNetDevSetMACInternal(const char *ifname, const virMacAddr *macaddr, bool quiet) { - int fd = -1; - int ret = -1; struct ifreq ifr; char macstr[VIR_MAC_STRING_BUFLEN]; + VIR_AUTOCLOSE(fd); if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return -1; /* To fill ifr.ifr_hdaddr.sa_family field */ if (ioctl(fd, SIOCGIFHWADDR, &ifr) < 0) { - virReportSystemError(errno, - _("Cannot get interface MAC on '%s'"), + virReportSystemError(errno, _("Cannot get interface MAC on '%s'"), ifname); - goto cleanup; + + VIR_DEBUG("SIOCSIFHWADDR %s get MAC - Fail", ifname); + return -1; } virMacAddrGetRaw(macaddr, (unsigned char *)ifr.ifr_hwaddr.sa_data); @@ -272,24 +267,22 @@ virNetDevSetMACInternal(const char *ifname, if (ioctl(fd, SIOCSIFHWADDR, &ifr) < 0) { if (quiet && - (errno == EADDRNOTAVAIL || errno == EPERM)) - goto cleanup; + (errno == EADDRNOTAVAIL || errno == EPERM)) { + VIR_DEBUG("SIOCSIFHWADDR %s MAC=%s - Fail", + ifname, virMacAddrFormat(macaddr, macstr)); + return -1; + } virReportSystemError(errno, _("Cannot set interface MAC to %s on '%s'"), virMacAddrFormat(macaddr, macstr), ifname); - goto cleanup; + return -1; } - ret = 0; + VIR_DEBUG("SIOCSIFHWADDR %s MAC=%s - Success", + ifname, virMacAddrFormat(macaddr, macstr)); - cleanup: - VIR_DEBUG("SIOCSIFHWADDR %s MAC=%s - %s", - ifname, virMacAddrFormat(macaddr, macstr), - ret < 0 ? "Fail" : "Success"); - - VIR_FORCE_CLOSE(fd); - return ret; + return 0; } @@ -305,8 +298,7 @@ virNetDevSetMACInternal(const char *ifname, struct ifreq ifr; struct sockaddr_dl sdl; char mac[VIR_MAC_STRING_BUFLEN + 1] = ":"; - int s; - int ret = -1; + VIR_AUTOCLOSE(s); if ((s = virNetDevSetupControl(ifname, &ifr)) < 0) return -1; @@ -320,23 +312,19 @@ virNetDevSetMACInternal(const char *ifname, if (ioctl(s, SIOCSIFLLADDR, &ifr) < 0) { if (quiet && - (errno == EADDRNOTAVAIL || errno == EPERM)) - goto cleanup; + (errno == EADDRNOTAVAIL || errno == EPERM)) { + VIR_DEBUG("SIOCSIFLLADDR %s MAC=%s - Fail", ifname, mac + 1); + return -1; + } virReportSystemError(errno, _("Cannot set interface MAC to %s on '%s'"), mac + 1, ifname); - goto cleanup; + return -1; } - ret = 0; - cleanup: - VIR_DEBUG("SIOCSIFLLADDR %s MAC=%s - %s", ifname, mac + 1, - ret < 0 ? "Fail" : "Success"); - - VIR_FORCE_CLOSE(s); - - return ret; + VIR_DEBUG("SIOCSIFLLADDR %s MAC=%s - Success", ifname, mac + 1); + return 0; } @@ -379,9 +367,8 @@ virNetDevSetMAC(const char *ifname, int virNetDevGetMAC(const char *ifname, virMacAddrPtr macaddr) { - int fd = -1; - int ret = -1; struct ifreq ifr; + VIR_AUTOCLOSE(fd); if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return -1; @@ -390,16 +377,12 @@ int virNetDevGetMAC(const char *ifname, virReportSystemError(errno, _("Cannot get interface MAC on '%s'"), ifname); - goto cleanup; + return -1; } virMacAddrSetRaw(macaddr, (unsigned char *)ifr.ifr_hwaddr.sa_data); - ret = 0; - - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return 0; } #else int virNetDevGetMAC(const char *ifname, @@ -424,9 +407,8 @@ int virNetDevGetMAC(const char *ifname, */ int virNetDevGetMTU(const char *ifname) { - int fd = -1; - int ret = -1; struct ifreq ifr; + VIR_AUTOCLOSE(fd); if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return -1; @@ -435,14 +417,10 @@ int virNetDevGetMTU(const char *ifname) virReportSystemError(errno, _("Cannot get interface MTU on '%s'"), ifname); - goto cleanup; + return -1; } - ret = ifr.ifr_mtu; - - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return ifr.ifr_mtu; } #else int virNetDevGetMTU(const char *ifname) @@ -468,9 +446,8 @@ int virNetDevGetMTU(const char *ifname) */ int virNetDevSetMTU(const char *ifname, int mtu) { - int fd = -1; - int ret = -1; struct ifreq ifr; + VIR_AUTOCLOSE(fd); if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return -1; @@ -481,14 +458,10 @@ int virNetDevSetMTU(const char *ifname, int mtu) virReportSystemError(errno, _("Cannot set interface MTU on '%s'"), ifname); - goto cleanup; + return -1; } - ret = 0; - - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return 0; } #else int virNetDevSetMTU(const char *ifname, int mtu ATTRIBUTE_UNUSED) @@ -592,9 +565,8 @@ int virNetDevSetNamespace(const char *ifname, pid_t pidInNs) */ int virNetDevSetName(const char* ifname, const char *newifname) { - int fd = -1; - int ret = -1; struct ifreq ifr; + VIR_AUTOCLOSE(fd); if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return -1; @@ -604,7 +576,7 @@ int virNetDevSetName(const char* ifname, const char *newifname) virReportSystemError(ERANGE, _("Network interface name '%s' is too long"), newifname); - goto cleanup; + return -1; } # else ifr.ifr_data = (caddr_t)newifname; @@ -614,14 +586,10 @@ int virNetDevSetName(const char* ifname, const char *newifname) virReportSystemError(errno, _("Unable to rename '%s' to '%s'"), ifname, newifname); - goto cleanup; + return -1; } - ret = 0; - - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return 0; } #else int virNetDevSetName(const char* ifname, const char *newifname) @@ -638,10 +606,9 @@ int virNetDevSetName(const char* ifname, const char *newifname) static int virNetDevSetIFFlag(const char *ifname, int flag, bool val) { - int fd = -1; - int ret = -1; struct ifreq ifr; int ifflags; + VIR_AUTOCLOSE(fd); if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return -1; @@ -650,7 +617,7 @@ virNetDevSetIFFlag(const char *ifname, int flag, bool val) virReportSystemError(errno, _("Cannot get interface flags on '%s'"), ifname); - goto cleanup; + return -1; } if (val) @@ -664,15 +631,11 @@ virNetDevSetIFFlag(const char *ifname, int flag, bool val) virReportSystemError(errno, _("Cannot set interface flags on '%s'"), ifname); - goto cleanup; + return -1; } } - ret = 0; - - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return 0; } #else static int @@ -765,9 +728,8 @@ virNetDevSetRcvAllMulti(const char *ifname, static int virNetDevGetIFFlag(const char *ifname, int flag, bool *val) { - int fd = -1; - int ret = -1; struct ifreq ifr; + VIR_AUTOCLOSE(fd); if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return -1; @@ -776,15 +738,11 @@ virNetDevGetIFFlag(const char *ifname, int flag, bool *val) virReportSystemError(errno, _("Cannot get interface flags on '%s'"), ifname); - goto cleanup; + return -1; } *val = (ifr.ifr_flags & flag) ? true : false; - ret = 0; - - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return 0; } #else static int @@ -909,9 +867,9 @@ char *virNetDevGetName(int ifindex) #if defined(SIOCGIFINDEX) && defined(HAVE_STRUCT_IFREQ) int virNetDevGetIndex(const char *ifname, int *ifindex) { - int ret = -1; struct ifreq ifreq; - int fd = socket(VIR_NETDEV_FAMILY, SOCK_DGRAM, 0); + VIR_AUTOCLOSE(fd); + socket(VIR_NETDEV_FAMILY, SOCK_DGRAM, 0); if (fd < 0) { virReportSystemError(errno, "%s", @@ -925,13 +883,13 @@ int virNetDevGetIndex(const char *ifname, int *ifindex) virReportSystemError(ERANGE, _("invalid interface name %s"), ifname); - goto cleanup; + return -1; } if (ioctl(fd, SIOCGIFINDEX, &ifreq) < 0) { virReportSystemError(errno, _("Unable to get index for interface %s"), ifname); - goto cleanup; + return -1; } # ifdef HAVE_STRUCT_IFREQ_IFR_INDEX @@ -939,11 +897,7 @@ int virNetDevGetIndex(const char *ifname, int *ifindex) # else *ifindex = ifreq.ifr_ifindex; # endif - ret = 0; - - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return 0; } #else /* ! SIOCGIFINDEX */ int virNetDevGetIndex(const char *ifname ATTRIBUTE_UNUSED, @@ -1013,8 +967,8 @@ int virNetDevGetVLanID(const char *ifname, int *vlanid) struct vlan_ioctl_args vlanargs = { .cmd = GET_VLAN_VID_CMD, }; - int ret = -1; - int fd = socket(PF_PACKET, SOCK_DGRAM, 0); + VIR_AUTOCLOSE(fd); + socket(PF_PACKET, SOCK_DGRAM, 0); if (fd < 0) { virReportSystemError(errno, "%s", @@ -1026,22 +980,17 @@ int virNetDevGetVLanID(const char *ifname, int *vlanid) virReportSystemError(ERANGE, _("invalid interface name %s"), ifname); - goto cleanup; + return -1; } if (ioctl(fd, SIOCGIFVLAN, &vlanargs) != 0) { virReportSystemError(errno, _("Unable to get VLAN for interface %s"), ifname); - goto cleanup; + return -1; } *vlanid = vlanargs.u.VID; - ret = 0; - - cleanup: - VIR_FORCE_CLOSE(fd); - - return ret; + return 0; } #else /* ! SIOCGIFVLAN */ int virNetDevGetVLanID(const char *ifname ATTRIBUTE_UNUSED, @@ -1070,55 +1019,43 @@ int virNetDevGetVLanID(const char *ifname ATTRIBUTE_UNUSED, int virNetDevValidateConfig(const char *ifname, const virMacAddr *macaddr, int ifindex) { - int fd = -1; - int ret = -1; struct ifreq ifr; int idx; int rc; + VIR_AUTOCLOSE(fd); if ((rc = virNetDevExists(ifname)) < 0) return -1; - if (rc == 0) { - ret = 0; - goto cleanup; - } + if (rc == 0) + return 0; if (macaddr != NULL) { if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return -1; if (ioctl(fd, SIOCGIFHWADDR, &ifr) < 0) { - if (errno == ENODEV) { - ret = 0; - goto cleanup; - } + if (errno == ENODEV) + return 0; + virReportSystemError(errno, _("could not get MAC address of interface %s"), ifname); - goto cleanup; + return -1; } if (virMacAddrCmpRaw(macaddr, - (unsigned char *)ifr.ifr_hwaddr.sa_data) != 0) { - ret = 0; - goto cleanup; - } + (unsigned char *)ifr.ifr_hwaddr.sa_data) != 0) + return 0; } if (ifindex != -1) { if (virNetDevGetIndex(ifname, &idx) < 0) - goto cleanup; - if (idx != ifindex) { - ret = 0; - goto cleanup; - } + return -1; + if (idx != ifindex) + return 0; } - ret = 1; - - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return 1; } #else int virNetDevValidateConfig(const char *ifname ATTRIBUTE_UNUSED, @@ -2649,9 +2586,8 @@ virNetDevGetLinkInfo(const char *ifname, int virNetDevAddMulti(const char *ifname, virMacAddrPtr macaddr) { - int fd = -1; - int ret = -1; struct ifreq ifr; + VIR_AUTOCLOSE(fd); if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return -1; @@ -2664,13 +2600,10 @@ int virNetDevAddMulti(const char *ifname, virReportSystemError(errno, _("Cannot add multicast MAC %s on '%s' interface"), virMacAddrFormat(macaddr, macstr), ifname); - goto cleanup; + return -1; } - ret = 0; - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return 0; } #else int virNetDevAddMulti(const char *ifname ATTRIBUTE_UNUSED, @@ -2698,9 +2631,8 @@ int virNetDevAddMulti(const char *ifname ATTRIBUTE_UNUSED, int virNetDevDelMulti(const char *ifname, virMacAddrPtr macaddr) { - int fd = -1; - int ret = -1; struct ifreq ifr; + VIR_AUTOCLOSE(fd); if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return -1; @@ -2713,13 +2645,10 @@ int virNetDevDelMulti(const char *ifname, virReportSystemError(errno, _("Cannot add multicast MAC %s on '%s' interface"), virMacAddrFormat(macaddr, macstr), ifname); - goto cleanup; + return -1; } - ret = 0; - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return 0; } #else int virNetDevDelMulti(const char *ifname ATTRIBUTE_UNUSED, @@ -3388,10 +3317,9 @@ int virNetDevSetCoalesce(const char *ifname, virNetDevCoalescePtr coalesce, bool update) { - int fd = -1; - int ret = -1; struct ifreq ifr; struct ethtool_coalesce coal = {0}; + VIR_AUTOCLOSE(fd); if (!coalesce && !update) return 0; @@ -3433,7 +3361,7 @@ int virNetDevSetCoalesce(const char *ifname, virReportSystemError(errno, _("Cannot set coalesce info on '%s'"), ifname); - goto cleanup; + return -1; } if (coalesce) { @@ -3467,10 +3395,7 @@ int virNetDevSetCoalesce(const char *ifname, } } - ret = 0; - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return 0; } # else int virNetDevSetCoalesce(const char *ifname, @@ -3503,30 +3428,26 @@ virNetDevGetFeatures(const char *ifname, virBitmapPtr *out) { struct ifreq ifr; - int ret = -1; - int fd = -1; + VIR_AUTOCLOSE(fd); if (!(*out = virBitmapNew(VIR_NET_DEV_FEAT_LAST))) return -1; if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) - goto cleanup; + return -1; virNetDevGetEthtoolFeatures(*out, fd, &ifr); if (virNetDevGetEthtoolGFeatures(*out, fd, &ifr) < 0) - goto cleanup; + return -1; if (virNetDevRDMAFeature(ifname, out) < 0) - goto cleanup; + return -1; if (virNetDevSwitchdevFeature(ifname, out) < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return 0; } #else int -- 2.17.1
On Mon, Sep 10, 2018 at 11:47:53AM +0800, Shi Lei wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOCLOSE macro, many of the VIR_FORCE_CLOSE calls can be dropped, which in turn leads to getting rid of many of our cleanup sections.
Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/util/virnetdev.c | 253 +++++++++++++++---------------------------- 1 file changed, 87 insertions(+), 166 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 5d4ad24..7f43f15 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -200,27 +200,22 @@ virNetDevSetupControl(const char *ifname ATTRIBUTE_UNUSED, */ int virNetDevExists(const char *ifname) { - int fd = -1; - int ret = -1; struct ifreq ifr; + VIR_AUTOCLOSE(fd);
if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return -1;
if (ioctl(fd, SIOCGIFFLAGS, &ifr)) { if (errno == ENODEV || errno == ENXIO) - ret = 0; - else - virReportSystemError(errno, - _("Unable to check interface flags for %s"), ifname); - goto cleanup; - } + return 0;
- ret = 1; + virReportSystemError(errno, _("Unable to check interface flags for %s"), + ifname); + return -1; + }
- cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return 1; } #else int virNetDevExists(const char *ifname) @@ -251,20 +246,20 @@ virNetDevSetMACInternal(const char *ifname, const virMacAddr *macaddr, bool quiet) { - int fd = -1; - int ret = -1; struct ifreq ifr; char macstr[VIR_MAC_STRING_BUFLEN]; + VIR_AUTOCLOSE(fd);
if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return -1;
/* To fill ifr.ifr_hdaddr.sa_family field */ if (ioctl(fd, SIOCGIFHWADDR, &ifr) < 0) { - virReportSystemError(errno, - _("Cannot get interface MAC on '%s'"), + virReportSystemError(errno, _("Cannot get interface MAC on '%s'"), ifname); - goto cleanup; + + VIR_DEBUG("SIOCSIFHWADDR %s get MAC - Fail", ifname); + return -1; }
virMacAddrGetRaw(macaddr, (unsigned char *)ifr.ifr_hwaddr.sa_data); @@ -272,24 +267,22 @@ virNetDevSetMACInternal(const char *ifname, if (ioctl(fd, SIOCSIFHWADDR, &ifr) < 0) {
if (quiet && - (errno == EADDRNOTAVAIL || errno == EPERM)) - goto cleanup; + (errno == EADDRNOTAVAIL || errno == EPERM)) { + VIR_DEBUG("SIOCSIFHWADDR %s MAC=%s - Fail", + ifname, virMacAddrFormat(macaddr, macstr)); + return -1; + }
virReportSystemError(errno, _("Cannot set interface MAC to %s on '%s'"), virMacAddrFormat(macaddr, macstr), ifname); - goto cleanup; + return -1; }
- ret = 0; + VIR_DEBUG("SIOCSIFHWADDR %s MAC=%s - Success", + ifname, virMacAddrFormat(macaddr, macstr));
- cleanup: - VIR_DEBUG("SIOCSIFHWADDR %s MAC=%s - %s", - ifname, virMacAddrFormat(macaddr, macstr), - ret < 0 ? "Fail" : "Success"); - - VIR_FORCE_CLOSE(fd); - return ret; + return 0; }
@@ -305,8 +298,7 @@ virNetDevSetMACInternal(const char *ifname, struct ifreq ifr; struct sockaddr_dl sdl; char mac[VIR_MAC_STRING_BUFLEN + 1] = ":"; - int s; - int ret = -1; + VIR_AUTOCLOSE(s);
if ((s = virNetDevSetupControl(ifname, &ifr)) < 0) return -1; @@ -320,23 +312,19 @@ virNetDevSetMACInternal(const char *ifname,
if (ioctl(s, SIOCSIFLLADDR, &ifr) < 0) { if (quiet && - (errno == EADDRNOTAVAIL || errno == EPERM)) - goto cleanup; + (errno == EADDRNOTAVAIL || errno == EPERM)) { + VIR_DEBUG("SIOCSIFLLADDR %s MAC=%s - Fail", ifname, mac + 1); + return -1; + }
virReportSystemError(errno, _("Cannot set interface MAC to %s on '%s'"), mac + 1, ifname); - goto cleanup; + return -1; }
- ret = 0; - cleanup: - VIR_DEBUG("SIOCSIFLLADDR %s MAC=%s - %s", ifname, mac + 1, - ret < 0 ? "Fail" : "Success"); - - VIR_FORCE_CLOSE(s); - - return ret; + VIR_DEBUG("SIOCSIFLLADDR %s MAC=%s - Success", ifname, mac + 1); + return 0; }
@@ -379,9 +367,8 @@ virNetDevSetMAC(const char *ifname, int virNetDevGetMAC(const char *ifname, virMacAddrPtr macaddr) { - int fd = -1; - int ret = -1; struct ifreq ifr; + VIR_AUTOCLOSE(fd);
if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return -1; @@ -390,16 +377,12 @@ int virNetDevGetMAC(const char *ifname, virReportSystemError(errno, _("Cannot get interface MAC on '%s'"), ifname); - goto cleanup; + return -1; }
virMacAddrSetRaw(macaddr, (unsigned char *)ifr.ifr_hwaddr.sa_data);
- ret = 0; - - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return 0; } #else int virNetDevGetMAC(const char *ifname, @@ -424,9 +407,8 @@ int virNetDevGetMAC(const char *ifname, */ int virNetDevGetMTU(const char *ifname) { - int fd = -1; - int ret = -1; struct ifreq ifr; + VIR_AUTOCLOSE(fd);
if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return -1; @@ -435,14 +417,10 @@ int virNetDevGetMTU(const char *ifname) virReportSystemError(errno, _("Cannot get interface MTU on '%s'"), ifname); - goto cleanup; + return -1; }
- ret = ifr.ifr_mtu; - - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return ifr.ifr_mtu; } #else int virNetDevGetMTU(const char *ifname) @@ -468,9 +446,8 @@ int virNetDevGetMTU(const char *ifname) */ int virNetDevSetMTU(const char *ifname, int mtu) { - int fd = -1; - int ret = -1; struct ifreq ifr; + VIR_AUTOCLOSE(fd);
if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return -1; @@ -481,14 +458,10 @@ int virNetDevSetMTU(const char *ifname, int mtu) virReportSystemError(errno, _("Cannot set interface MTU on '%s'"), ifname); - goto cleanup; + return -1; }
- ret = 0; - - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return 0; } #else int virNetDevSetMTU(const char *ifname, int mtu ATTRIBUTE_UNUSED) @@ -592,9 +565,8 @@ int virNetDevSetNamespace(const char *ifname, pid_t pidInNs) */ int virNetDevSetName(const char* ifname, const char *newifname) { - int fd = -1; - int ret = -1; struct ifreq ifr; + VIR_AUTOCLOSE(fd);
if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return -1; @@ -604,7 +576,7 @@ int virNetDevSetName(const char* ifname, const char *newifname) virReportSystemError(ERANGE, _("Network interface name '%s' is too long"), newifname); - goto cleanup; + return -1; } # else ifr.ifr_data = (caddr_t)newifname; @@ -614,14 +586,10 @@ int virNetDevSetName(const char* ifname, const char *newifname) virReportSystemError(errno, _("Unable to rename '%s' to '%s'"), ifname, newifname); - goto cleanup; + return -1; }
- ret = 0; - - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return 0; } #else int virNetDevSetName(const char* ifname, const char *newifname) @@ -638,10 +606,9 @@ int virNetDevSetName(const char* ifname, const char *newifname) static int virNetDevSetIFFlag(const char *ifname, int flag, bool val) { - int fd = -1; - int ret = -1; struct ifreq ifr; int ifflags; + VIR_AUTOCLOSE(fd);
if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return -1; @@ -650,7 +617,7 @@ virNetDevSetIFFlag(const char *ifname, int flag, bool val) virReportSystemError(errno, _("Cannot get interface flags on '%s'"), ifname); - goto cleanup; + return -1; }
if (val) @@ -664,15 +631,11 @@ virNetDevSetIFFlag(const char *ifname, int flag, bool val) virReportSystemError(errno, _("Cannot set interface flags on '%s'"), ifname); - goto cleanup; + return -1; } }
- ret = 0; - - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return 0; } #else static int @@ -765,9 +728,8 @@ virNetDevSetRcvAllMulti(const char *ifname, static int virNetDevGetIFFlag(const char *ifname, int flag, bool *val) { - int fd = -1; - int ret = -1; struct ifreq ifr; + VIR_AUTOCLOSE(fd);
if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return -1; @@ -776,15 +738,11 @@ virNetDevGetIFFlag(const char *ifname, int flag, bool *val) virReportSystemError(errno, _("Cannot get interface flags on '%s'"), ifname); - goto cleanup; + return -1; }
*val = (ifr.ifr_flags & flag) ? true : false; - ret = 0; - - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; + return 0; } #else static int @@ -909,9 +867,9 @@ char *virNetDevGetName(int ifindex) #if defined(SIOCGIFINDEX) && defined(HAVE_STRUCT_IFREQ) int virNetDevGetIndex(const char *ifname, int *ifindex) { - int ret = -1; struct ifreq ifreq; - int fd = socket(VIR_NETDEV_FAMILY, SOCK_DGRAM, 0); + VIR_AUTOCLOSE(fd); + socket(VIR_NETDEV_FAMILY, SOCK_DGRAM, 0);
^this could not potentially work... Erik
On 2018-09-11 at 20:44, Erik Skultety wrote:
On Mon, Sep 10, 2018 at 11:47:53AM +0800, Shi Lei wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOCLOSE macro, many of the VIR_FORCE_CLOSE calls can be dropped, which in turn leads to getting rid of many of our cleanup sections.
Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- static int @@ -909,9 +867,9 @@ char *virNetDevGetName(int ifindex) #if defined(SIOCGIFINDEX) && defined(HAVE_STRUCT_IFREQ) int virNetDevGetIndex(const char *ifname, int *ifindex) { - int ret = -1; struct ifreq ifreq; - int fd = socket(VIR_NETDEV_FAMILY, SOCK_DGRAM, 0); + VIR_AUTOCLOSE(fd); + socket(VIR_NETDEV_FAMILY, SOCK_DGRAM, 0);
^this could not potentially work...
Erik
Sorry! I'm too careless. I think that a new syntax-check rule might make sense. This rule checks below: type foo(a0, a1 ...); [type] var = foo(a0, a1, ...); /* It's OK */ ignore_value(foo(a0, a1, ...)); /* It's OK */ foo(a0, a1, ...); /* Report this usage */ And it takes effect before compilation and even it's in the inactive side of the #if-#else conditons. It would not check macros since their name are upper case and we don't care for the function as condition in if-else statement. How about it? And I can try it if it's a bit helpful ... Shi Lei
On Tue, Sep 11, 2018 at 11:37:45PM +0800, Shi Lei wrote:
On 2018-09-11 at 20:44, Erik Skultety wrote:
On Mon, Sep 10, 2018 at 11:47:53AM +0800, Shi Lei wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOCLOSE macro, many of the VIR_FORCE_CLOSE calls can be dropped, which in turn leads to getting rid of many of our cleanup sections.
Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- static int @@ -909,9 +867,9 @@ char *virNetDevGetName(int ifindex) #if defined(SIOCGIFINDEX) && defined(HAVE_STRUCT_IFREQ) int virNetDevGetIndex(const char *ifname, int *ifindex) { - int ret = -1; struct ifreq ifreq; - int fd = socket(VIR_NETDEV_FAMILY, SOCK_DGRAM, 0); + VIR_AUTOCLOSE(fd); + socket(VIR_NETDEV_FAMILY, SOCK_DGRAM, 0);
^this could not potentially work...
Erik
Sorry! I'm too careless.
I think that a new syntax-check rule might make sense. This rule checks below:
type foo(a0, a1 ...);
[type] var = foo(a0, a1, ...); /* It's OK */ ignore_value(foo(a0, a1, ...)); /* It's OK */
foo(a0, a1, ...); /* Report this usage */
but this would go away with the syntax-check rule you proposed in you response to the first patch? If not, then would you be more specific to help me understand the problem more? Thanks, Erik
And it takes effect before compilation and even it's in the inactive side of the #if-#else conditons.
It would not check macros since their name are upper case and we don't care for the function as condition in if-else statement.
How about it? And I can try it if it's a bit helpful ...
Shi Lei
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
By making use of GNU C's cleanup attribute handled by the VIR_AUTOCLOSE macro, many of the VIR_FORCE_CLOSE calls can be dropped, which in turn leads to getting rid of many of our cleanup sections. Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/phyp/phyp_driver.c | 29 ++++++++++------------------- 1 file changed, 10 insertions(+), 19 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 28a1fa3..c813fac 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -463,37 +463,33 @@ phypUUIDTable_WriteFile(virConnectPtr conn) phyp_driverPtr phyp_driver = conn->privateData; uuid_tablePtr uuid_table = phyp_driver->uuid_table; size_t i = 0; - int fd = -1; char local_file[] = "./uuid_table"; + VIR_AUTOCLOSE(fd); if ((fd = creat(local_file, 0755)) == -1) - goto err; + return -1; for (i = 0; i < uuid_table->nlpars; i++) { if (safewrite(fd, &uuid_table->lpars[i]->id, sizeof(uuid_table->lpars[i]->id)) != sizeof(uuid_table->lpars[i]->id)) { VIR_ERROR(_("Unable to write information to local file.")); - goto err; + return -1; } if (safewrite(fd, uuid_table->lpars[i]->uuid, VIR_UUID_BUFLEN) != VIR_UUID_BUFLEN) { VIR_ERROR(_("Unable to write information to local file.")); - goto err; + return -1; } } if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("Could not close %s"), local_file); - goto err; + return -1; } return 0; - - err: - VIR_FORCE_CLOSE(fd); - return -1; } static int @@ -641,14 +637,14 @@ phypUUIDTable_ReadFile(virConnectPtr conn) phyp_driverPtr phyp_driver = conn->privateData; uuid_tablePtr uuid_table = phyp_driver->uuid_table; size_t i = 0; - int fd = -1; char local_file[] = "./uuid_table"; int rc = 0; int id; + VIR_AUTOCLOSE(fd); if ((fd = open(local_file, O_RDONLY)) == -1) { VIR_WARN("Unable to read information from local file."); - goto err; + return -1; } /* Creating a new data base and writing to local file */ @@ -658,28 +654,23 @@ phypUUIDTable_ReadFile(virConnectPtr conn) rc = read(fd, &id, sizeof(int)); if (rc == sizeof(int)) { if (VIR_ALLOC(uuid_table->lpars[i]) < 0) - goto err; + return -1; uuid_table->lpars[i]->id = id; } else { VIR_WARN ("Unable to read from information from local file."); - goto err; + return -1; } rc = read(fd, uuid_table->lpars[i]->uuid, VIR_UUID_BUFLEN); if (rc != VIR_UUID_BUFLEN) { VIR_WARN("Unable to read information from local file."); - goto err; + return -1; } } } - VIR_FORCE_CLOSE(fd); return 0; - - err: - VIR_FORCE_CLOSE(fd); - return -1; } static int -- 2.17.1
By making use of GNU C's cleanup attribute handled by the VIR_AUTOCLOSE macro, many of the VIR_FORCE_CLOSE calls can be dropped, which in turn leads to getting rid of many of our cleanup sections. Signed-off-by: Shi Lei <shi_lei@massclouds.com> --- src/uml/uml_conf.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c index f116e61..da0dc69 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -112,14 +112,14 @@ umlConnectTapDevice(virDomainDefPtr vm, const char *bridge) { bool template_ifname = false; - int tapfd = -1; + VIR_AUTOCLOSE(tapfd); if (!net->ifname || STRPREFIX(net->ifname, VIR_NET_GENERATED_TAP_PREFIX) || strchr(net->ifname, '%')) { VIR_FREE(net->ifname); if (VIR_STRDUP(net->ifname, VIR_NET_GENERATED_TAP_PREFIX "%d") < 0) - goto error; + return -1; /* avoid exposing vnet%d in getXMLDesc or error outputs */ template_ifname = true; } @@ -133,23 +133,18 @@ umlConnectTapDevice(virDomainDefPtr vm, VIR_NETDEV_TAP_CREATE_PERSIST) < 0) { if (template_ifname) VIR_FREE(net->ifname); - goto error; + return -1; } if (net->filter) { if (virDomainConfNWFilterInstantiate(vm->name, vm->uuid, net, false) < 0) { if (template_ifname) VIR_FREE(net->ifname); - goto error; + return -1; } } - VIR_FORCE_CLOSE(tapfd); return 0; - - error: - VIR_FORCE_CLOSE(tapfd); - return -1; } static char * -- 2.17.1
On Mon, Sep 10, 2018 at 11:47:49AM +0800, Shi Lei wrote:
This series introduce VIR_AUTOCLOSE macro which can force close fd of the file automatically when the fd goes out of scope. It's used to eliminate VIR_FORCE_CLOSE in cleanup sections. The new macro consults VIR_AUTOFREE and VIR_AUTOPTR.
Shi Lei (6): util: file: introduce VIR_AUTOCLOSE macro to close fd of the file automatically util: file: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup sections util: netdevbridge: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup sections util: netdev: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup sections phyp: driver: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup sections uml: conf: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup sections
src/phyp/phyp_driver.c | 29 ++--- src/uml/uml_conf.c | 13 +- src/util/virfile.c | 21 +-- src/util/virfile.h | 20 ++- src/util/virnetdev.c | 253 +++++++++++++------------------------ src/util/virnetdevbridge.c | 120 ++++++------------
This touches only a handful of util modules and random drivers, there should be some consistency here, so if we're introducing something like this, then we should do it in a similar manner as we did for VIR_AUTO{FREE,PTR}, i.e. util/ first (btw. we still haven't even finished that one yet). Erik
On 2018-09-11 at 20:50, Erik Skultety wrote:
On Mon, Sep 10, 2018 at 11:47:49AM +0800, Shi Lei wrote:
This series introduce VIR_AUTOCLOSE macro which can force close fd of the file automatically when the fd goes out of scope. It's used to eliminate VIR_FORCE_CLOSE in cleanup sections. The new macro consults VIR_AUTOFREE and VIR_AUTOPTR.
Shi Lei (6): util: file: introduce VIR_AUTOCLOSE macro to close fd of the file automatically util: file: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup sections util: netdevbridge: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup sections util: netdev: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup sections phyp: driver: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup sections uml: conf: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup sections
src/phyp/phyp_driver.c | 29 ++--- src/uml/uml_conf.c | 13 +- src/util/virfile.c | 21 +-- src/util/virfile.h | 20 ++- src/util/virnetdev.c | 253 +++++++++++++------------------------ src/util/virnetdevbridge.c | 120 ++++++------------
This touches only a handful of util modules and random drivers, there should be some consistency here, so if we're introducing something like this, then we should do it in a similar manner as we did for VIR_AUTO{FREE,PTR}, i.e. util/ first (btw. we still haven't even finished that one yet).
Erik
Okay. But how to do in the v2 series. Whether v2 should only deal with files in the util/ ? Shi Lei
On Tue, Sep 11, 2018 at 11:49:11PM +0800, Shi Lei wrote:
On 2018-09-11 at 20:50, Erik Skultety wrote:
On Mon, Sep 10, 2018 at 11:47:49AM +0800, Shi Lei wrote:
This series introduce VIR_AUTOCLOSE macro which can force close fd of the file automatically when the fd goes out of scope. It's used to eliminate VIR_FORCE_CLOSE in cleanup sections. The new macro consults VIR_AUTOFREE and VIR_AUTOPTR.
Shi Lei (6): util: file: introduce VIR_AUTOCLOSE macro to close fd of the file automatically util: file: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup sections util: netdevbridge: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup sections util: netdev: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup sections phyp: driver: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup sections uml: conf: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup sections
src/phyp/phyp_driver.c | 29 ++--- src/uml/uml_conf.c | 13 +- src/util/virfile.c | 21 +-- src/util/virfile.h | 20 ++- src/util/virnetdev.c | 253 +++++++++++++------------------------ src/util/virnetdevbridge.c | 120 ++++++------------
This touches only a handful of util modules and random drivers, there should be some consistency here, so if we're introducing something like this, then we should do it in a similar manner as we did for VIR_AUTO{FREE,PTR}, i.e. util/ first (btw. we still haven't even finished that one yet).
Erik
Okay. But how to do in the v2 series.
Whether v2 should only deal with files in the util/ ?
Yeah, I think that's the best approach we can take, since we still haven't finished converting util to the other macros, we should start with the those modules within util/ that are already completely switched to AUTO{FREE/PTR} and similarly do it in batches, as reviewing 30+ patches on one go is always demanding... Thanks, Erik
On 2018-09-12 at 15:24, Erik Skultety wrote:
On Tue, Sep 11, 2018 at 11:49:11PM +0800, Shi Lei wrote:
On 2018-09-11 at 20:50, Erik Skultety wrote:
On Mon, Sep 10, 2018 at 11:47:49AM +0800, Shi Lei wrote:
This series introduce VIR_AUTOCLOSE macro which can force close fd of the file automatically when the fd goes out of scope. It's used to eliminate VIR_FORCE_CLOSE in cleanup sections. The new macro consults VIR_AUTOFREE and VIR_AUTOPTR.
Shi Lei (6): util: file: introduce VIR_AUTOCLOSE macro to close fd of the file automatically util: file: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup sections util: netdevbridge: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup sections util: netdev: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup sections phyp: driver: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup sections uml: conf: use VIR_AUTOCLOSE instead of VIR_FORCE_CLOSE in cleanup sections
src/phyp/phyp_driver.c | 29 ++--- src/uml/uml_conf.c | 13 +- src/util/virfile.c | 21 +-- src/util/virfile.h | 20 ++- src/util/virnetdev.c | 253 +++++++++++++------------------------ src/util/virnetdevbridge.c | 120 ++++++------------
This touches only a handful of util modules and random drivers, there should be some consistency here, so if we're introducing something like this, then we should do it in a similar manner as we did for VIR_AUTO{FREE,PTR}, i.e. util/ first (btw. we still haven't even finished that one yet).
Erik
Okay. But how to do in the v2 series.
Whether v2 should only deal with files in the util/ ?
Yeah, I think that's the best approach we can take, since we still haven't finished converting util to the other macros, we should start with the those modules within util/ that are already completely switched to AUTO{FREE/PTR} and similarly do it in batches, as reviewing 30+ patches on one go is always demanding...
Thanks, Erik
Okay. I see. Thanks. Shi Lei
participants (3)
-
Erik Skultety -
Michal Privoznik -
Shi Lei