[libvirt] [PATCH 0/3] virFileWrapper: Couple of small improvements
*** BLURB HERE *** Michal Prívozník (3): tests: Turn virFileWrapperAddPrefix to void virFileWrapper: Use VIR_AUTOFREE() tests: Call virFileWrapperClearPrefixes() for tests using virFileWrapper tests/qemufirmwaretest.c | 2 + tests/qemuxml2argvtest.c | 1 + tests/virfilewrapper.c | 94 +++++++++++----------------------------- tests/virfilewrapper.h | 2 +- 4 files changed, 29 insertions(+), 70 deletions(-) -- 2.19.2
In theory, it's nice to have virFileWrapperAddPrefix() return a value that indicates if the function succeeded or not. But in practice, nobody checks for that and in fact blindly believes that the function succeeded. Therefore, make the function return nothing and just abort() if it would fail. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virfilewrapper.c | 13 +++++++------ tests/virfilewrapper.h | 2 +- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/tests/virfilewrapper.c b/tests/virfilewrapper.c index f7a7a931e1..497e91bd45 100644 --- a/tests/virfilewrapper.c +++ b/tests/virfilewrapper.c @@ -66,13 +66,15 @@ static void init_syms(void) } -int +void virFileWrapperAddPrefix(const char *prefix, const char *override) { /* Both parameters are mandatory */ - if (!prefix || !override) - return -1; + if (!prefix || !override) { + fprintf(stderr, "Attempt to add invalid path override\n"); + abort(); + } init_syms(); @@ -80,10 +82,9 @@ virFileWrapperAddPrefix(const char *prefix, VIR_APPEND_ELEMENT_QUIET(overrides, noverrides, override) < 0) { VIR_FREE(prefixes); VIR_FREE(overrides); - return -1; + fprintf(stderr, "Unable to add path override for '%s'\n", prefix); + abort(); } - - return 0; } diff --git a/tests/virfilewrapper.h b/tests/virfilewrapper.h index 044c532232..80fd6244ad 100644 --- a/tests/virfilewrapper.h +++ b/tests/virfilewrapper.h @@ -19,7 +19,7 @@ #ifndef LIBVIRT_VIRFILEWRAPPER_H # define LIBVIRT_VIRFILEWRAPPER_H -int +void virFileWrapperAddPrefix(const char *prefix, const char *override); -- 2.19.2
On Wed, Mar 13, 2019 at 11:13:07AM +0100, Michal Privoznik wrote:
In theory, it's nice to have virFileWrapperAddPrefix() return a value that indicates if the function succeeded or not. But in practice, nobody checks for that and in fact blindly believes that the function succeeded. Therefore, make the function return nothing and just abort() if it would fail.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virfilewrapper.c | 13 +++++++------ tests/virfilewrapper.h | 2 +- 2 files changed, 8 insertions(+), 7 deletions(-)
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
On Wed, Mar 13, 2019 at 11:13:07AM +0100, Michal Privoznik wrote:
In theory, it's nice to have virFileWrapperAddPrefix() return a value that indicates if the function succeeded or not. But in practice, nobody checks for that and in fact blindly believes that the function succeeded. Therefore, make the function return nothing and just abort() if it would fail.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
This enables us to simplify the code a bit. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/virfilewrapper.c | 81 ++++++++++-------------------------------- 1 file changed, 18 insertions(+), 63 deletions(-) diff --git a/tests/virfilewrapper.c b/tests/virfilewrapper.c index 497e91bd45..88441331ce 100644 --- a/tests/virfilewrapper.c +++ b/tests/virfilewrapper.c @@ -152,110 +152,74 @@ virFileWrapperOverridePrefix(const char *path) FILE *fopen(const char *path, const char *mode) { - FILE *ret = NULL; - char *newpath = NULL; + VIR_AUTOFREE(char *) newpath = NULL; PATH_OVERRIDE(newpath, path); - ret = real_fopen(newpath, mode); - - VIR_FREE(newpath); - - return ret; + return real_fopen(newpath, mode); } int access(const char *path, int mode) { - int ret = -1; - char *newpath = NULL; + VIR_AUTOFREE(char *) newpath = NULL; PATH_OVERRIDE(newpath, path); - ret = real_access(newpath, mode); - - VIR_FREE(newpath); - - return ret; + return real_access(newpath, mode); } # ifdef HAVE___LXSTAT int __lxstat(int ver, const char *path, struct stat *sb) { - int ret = -1; - char *newpath = NULL; + VIR_AUTOFREE(char *) newpath = NULL; PATH_OVERRIDE(newpath, path); - ret = real___lxstat(ver, newpath, sb); - - VIR_FREE(newpath); - - return ret; + return real___lxstat(ver, newpath, sb); } # endif /* HAVE___LXSTAT */ int lstat(const char *path, struct stat *sb) { - int ret = -1; - char *newpath = NULL; + VIR_AUTOFREE(char *) newpath = NULL; PATH_OVERRIDE(newpath, path); - ret = real_lstat(newpath, sb); - - VIR_FREE(newpath); - - return ret; + return real_lstat(newpath, sb); } # ifdef HAVE___XSTAT int __xstat(int ver, const char *path, struct stat *sb) { - int ret = -1; - char *newpath = NULL; + VIR_AUTOFREE(char *) newpath = NULL; PATH_OVERRIDE(newpath, path); - ret = real___xstat(ver, newpath, sb); - - VIR_FREE(newpath); - - return ret; + return real___xstat(ver, newpath, sb); } # endif /* HAVE___XSTAT */ int stat(const char *path, struct stat *sb) { - int ret = -1; - char *newpath = NULL; + VIR_AUTOFREE(char *) newpath = NULL; PATH_OVERRIDE(newpath, path); - ret = real_stat(newpath, sb); - - VIR_FREE(newpath); - - return ret; + return real_stat(newpath, sb); } int mkdir(const char *path, mode_t mode) { - int ret = -1; - char *newpath = NULL; + VIR_AUTOFREE(char *) newpath = NULL; PATH_OVERRIDE(newpath, path); - ret = real_mkdir(newpath, mode); - - VIR_FREE(newpath); - - return ret; + return real_mkdir(newpath, mode); } int open(const char *path, int flags, ...) { - int ret = -1; - char *newpath = NULL; + VIR_AUTOFREE(char *) newpath = NULL; va_list ap; mode_t mode = 0; @@ -270,24 +234,15 @@ int open(const char *path, int flags, ...) va_end(ap); } - ret = real_open(newpath, flags, mode); - - VIR_FREE(newpath); - - return ret; + return real_open(newpath, flags, mode); } DIR *opendir(const char *path) { - DIR *ret = NULL; - char *newpath = NULL; + VIR_AUTOFREE(char *) newpath = NULL; PATH_OVERRIDE(newpath, path); - ret = real_opendir(newpath); - - VIR_FREE(newpath); - - return ret; + return real_opendir(newpath); } #endif -- 2.19.2
On Wed, Mar 13, 2019 at 11:13:08AM +0100, Michal Privoznik wrote:
This enables us to simplify the code a bit.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
This is mostly to avoid a memleak that is not a true memleak anyway - prefixes will be freed by kernel upon test exit. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemufirmwaretest.c | 2 ++ tests/qemuxml2argvtest.c | 1 + 2 files changed, 3 insertions(+) diff --git a/tests/qemufirmwaretest.c b/tests/qemufirmwaretest.c index 3fbeb2b852..340344ebba 100644 --- a/tests/qemufirmwaretest.c +++ b/tests/qemufirmwaretest.c @@ -128,6 +128,8 @@ mymain(void) if (virTestRun("QEMU FW precedence test", testFWPrecedence, NULL) < 0) ret = -1; + virFileWrapperClearPrefixes(); + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index bd9619ae38..03e8d79595 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -3044,6 +3044,7 @@ mymain(void) qemuTestDriverFree(&driver); VIR_FREE(fakerootdir); virHashFree(capslatest); + virFileWrapperClearPrefixes(); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.19.2
On Wed, Mar 13, 2019 at 11:13:09AM +0100, Michal Privoznik wrote:
This is mostly to avoid a memleak that is not a true memleak anyway - prefixes will be freed by kernel upon test exit.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemufirmwaretest.c | 2 ++ tests/qemuxml2argvtest.c | 1 + 2 files changed, 3 insertions(+)
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
On Wed, Mar 13, 2019 at 11:13:09AM +0100, Michal Privoznik wrote:
This is mostly to avoid a memleak that is not a true memleak anyway - prefixes will be freed by kernel upon test exit.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
participants (3)
-
Erik Skultety -
Martin Kletzander -
Michal Privoznik