[libvirt] [PATCH v2 0/3] Couple of testuitls.c almost trivial fixes
This is v2 of: https://www.redhat.com/archives/libvir-list/2019-February/msg01077.html diff to v1: - Split one patch into smaller ones - Made myself a new coffee meanwhile Michal Prívozník (3): virTestCompareToULL: Use VIR_AUTOFREE() virTestCompareToULL: Rename local variables testutils: Explicitly name virTestCompare*() arguments tests/testutils.c | 51 +++++++++++++++++------------------------------ tests/testutils.h | 10 +++++----- 2 files changed, 23 insertions(+), 38 deletions(-) -- 2.19.2
In order to save a few lines of code, and also since it's hype let's use VIR_AUTOFREE() for the two strings we allocate there. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/testutils.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/tests/testutils.c b/tests/testutils.c index d2219ad21e..ac86418653 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -822,23 +822,16 @@ int virTestCompareToULL(unsigned long long content, unsigned long long src) { - char *strcontent = NULL; - char *strsrc = NULL; - int ret = -1; + VIR_AUTOFREE(char *) strcontent = NULL; + VIR_AUTOFREE(char *) strsrc = NULL; if (virAsprintf(&strcontent, "%llu", content) < 0) - goto cleanup; + return -1; if (virAsprintf(&strsrc, "%llu", src) < 0) - goto cleanup; + return -1; - ret = virTestCompareToString(strcontent, strsrc); - - cleanup: - VIR_FREE(strcontent); - VIR_FREE(strsrc); - - return ret; + return virTestCompareToString(strcontent, strsrc); } /* -- 2.19.2
On Wed, 2019-02-20 at 14:20 +0100, Michal Privoznik wrote:
In order to save a few lines of code, and also since it's hype
https://i.imgflip.com/2u5fpr.jpg
let's use VIR_AUTOFREE() for the two strings we allocate there.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/testutils.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Feb 20, 2019 at 02:20:08PM +0100, Michal Privoznik wrote:
In order to save a few lines of code, and also since it's hype let's use VIR_AUTOFREE() for the two strings we allocate there.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/testutils.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The current naming makes it hard for me to see which holds the expected value and which holds the actual value. Rename them to make it obvious. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/testutils.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/testutils.c b/tests/testutils.c index ac86418653..01f3e8bb93 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -822,16 +822,16 @@ int virTestCompareToULL(unsigned long long content, unsigned long long src) { - VIR_AUTOFREE(char *) strcontent = NULL; - VIR_AUTOFREE(char *) strsrc = NULL; + VIR_AUTOFREE(char *) expectStr = NULL; + VIR_AUTOFREE(char *) actualStr = NULL; - if (virAsprintf(&strcontent, "%llu", content) < 0) + if (virAsprintf(&expectStr, "%llu", content) < 0) return -1; - if (virAsprintf(&strsrc, "%llu", src) < 0) + if (virAsprintf(&actualStr, "%llu", src) < 0) return -1; - return virTestCompareToString(strcontent, strsrc); + return virTestCompareToString(expectStr, actualStr); } /* -- 2.19.2
On Wed, 2019-02-20 at 14:20 +0100, Michal Privoznik wrote:
The current naming makes it hard for me to see which holds the expected value and which holds the actual value. Rename them to make it obvious.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/testutils.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
Yeah, sorry for misunderstanding the previous commit message... This can safely be squashed into the next commit. Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Feb 20, 2019 at 02:20:09PM +0100, Michal Privoznik wrote:
The current naming makes it hard for me to see which holds the expected value and which holds the actual value. Rename them to make it obvious.
Well, the naming was confusing because of the function parameters, so I would mainly focus on those as from a caller this is still weird naming. I heard that Andrea already reviewed the whole series (as I can't see it because our ML is acting up again), so this is just an extra hint.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/testutils.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/tests/testutils.c b/tests/testutils.c index ac86418653..01f3e8bb93 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -822,16 +822,16 @@ int virTestCompareToULL(unsigned long long content, unsigned long long src) { - VIR_AUTOFREE(char *) strcontent = NULL; - VIR_AUTOFREE(char *) strsrc = NULL; + VIR_AUTOFREE(char *) expectStr = NULL; + VIR_AUTOFREE(char *) actualStr = NULL;
- if (virAsprintf(&strcontent, "%llu", content) < 0) + if (virAsprintf(&expectStr, "%llu", content) < 0) return -1;
- if (virAsprintf(&strsrc, "%llu", src) < 0) + if (virAsprintf(&actualStr, "%llu", src) < 0) return -1;
- return virTestCompareToString(strcontent, strsrc); + return virTestCompareToString(expectStr, actualStr); }
/* -- 2.19.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Feb 20, 2019 at 02:20:09PM +0100, Michal Privoznik wrote:
The current naming makes it hard for me to see which holds the expected value and which holds the actual value. Rename them to make it obvious.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/testutils.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Currently, some arguments are called strcontent and strsrc, or content and src or some other combination. This makes it impossible to see at the first glance what argument is supposed to represent 'expected' value and which one represents 'actual' value. Rename the arguments to make it obvious. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com> --- tests/testutils.c | 34 +++++++++++++--------------------- tests/testutils.h | 10 +++++----- 2 files changed, 18 insertions(+), 26 deletions(-) diff --git a/tests/testutils.c b/tests/testutils.c index 01f3e8bb93..13bb9630df 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -767,19 +767,19 @@ int virTestDifferenceBin(FILE *stream, } /* - * @param strcontent: String input content - * @param filename: File to compare strcontent against + * @param actual: String input content + * @param filename: File to compare @actual against * - * If @strcontent is NULL, it's treated as an empty string. + * If @actual is NULL, it's treated as an empty string. */ int -virTestCompareToFile(const char *strcontent, +virTestCompareToFile(const char *actual, const char *filename) { int ret = -1; char *filecontent = NULL; char *fixedcontent = NULL; - const char *cmpcontent = strcontent; + const char *cmpcontent = actual; if (!cmpcontent) cmpcontent = ""; @@ -814,36 +814,28 @@ virTestCompareToFile(const char *strcontent, return ret; } -/* - * @param content: Input content - * @param src: Source to compare @content against - */ int -virTestCompareToULL(unsigned long long content, - unsigned long long src) +virTestCompareToULL(unsigned long long expect, + unsigned long long actual) { VIR_AUTOFREE(char *) expectStr = NULL; VIR_AUTOFREE(char *) actualStr = NULL; - if (virAsprintf(&expectStr, "%llu", content) < 0) + if (virAsprintf(&expectStr, "%llu", expect) < 0) return -1; - if (virAsprintf(&actualStr, "%llu", src) < 0) + if (virAsprintf(&actualStr, "%llu", actual) < 0) return -1; return virTestCompareToString(expectStr, actualStr); } -/* - * @param strcontent: String input content - * @param strsrc: String source to compare strcontent against - */ int -virTestCompareToString(const char *strcontent, - const char *strsrc) +virTestCompareToString(const char *expect, + const char *actual) { - if (STRNEQ_NULLABLE(strcontent, strsrc)) { - virTestDifference(stderr, strcontent, strsrc); + if (STRNEQ_NULLABLE(expect, actual)) { + virTestDifference(stderr, expect, actual); return -1; } diff --git a/tests/testutils.h b/tests/testutils.h index 658f9053ad..d0a259e2d4 100644 --- a/tests/testutils.h +++ b/tests/testutils.h @@ -76,12 +76,12 @@ int virTestDifferenceBin(FILE *stream, const char *expect, const char *actual, size_t length); -int virTestCompareToFile(const char *strcontent, +int virTestCompareToFile(const char *actual, const char *filename); -int virTestCompareToString(const char *strcontent, - const char *strsrc); -int virTestCompareToULL(unsigned long long content, - unsigned long long src); +int virTestCompareToString(const char *expect, + const char *actual); +int virTestCompareToULL(unsigned long long expect, + unsigned long long actual); unsigned int virTestGetDebug(void); unsigned int virTestGetVerbose(void); -- 2.19.2
On Wed, 2019-02-20 at 14:20 +0100, Michal Privoznik wrote: [...]
/* - * @param strcontent: String input content - * @param filename: File to compare strcontent against + * @param actual: String input content + * @param filename: File to compare @actual against * - * If @strcontent is NULL, it's treated as an empty string. + * If @actual is NULL, it's treated as an empty string. */ int -virTestCompareToFile(const char *strcontent, +virTestCompareToFile(const char *actual, const char *filename)
Agree with John that 'actual' being the first argument rather than the second one here is pretty weird, but we can take care of that in a follow-up commit. This change is already a massive improvement over the status quo :) [...]
@@ -814,36 +814,28 @@ virTestCompareToFile(const char *strcontent, return ret; }
-/* - * @param content: Input content - * @param src: Source to compare @content against - */
Not sure why you removed the comments for this function and virTestCompareToString() rather than just updating them as you've done for the one above, but with the new names for the arguments it's pretty much self-explanatory so I'm totally fine with it. Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Feb 20, 2019 at 02:20:10PM +0100, Michal Privoznik wrote:
Currently, some arguments are called strcontent and strsrc, or content and src or some other combination. This makes it impossible to see at the first glance what argument is supposed to represent 'expected' value and which one represents 'actual' value. Rename the arguments to make it obvious.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Reviewed-by: John Ferlan <jferlan@redhat.com> --- tests/testutils.c | 34 +++++++++++++--------------------- tests/testutils.h | 10 +++++----- 2 files changed, 18 insertions(+), 26 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (4)
-
Andrea Bolognani -
Ján Tomko -
Martin Kletzander -
Michal Privoznik