7:20 a.m.
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
7:20 a.m.
New subject: [libvirt] [PATCH v2 1/3] virTestCompareToULL: Use VIR_AUTOFREE()
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(a)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
7:40 a.m.
New subject: [libvirt] [PATCH v2 1/3] virTestCompareToULL: Use VIR_AUTOFREE()
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(a)redhat.com>
---
tests/testutils.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
8:23 a.m.
New subject: [libvirt] [PATCH v2 1/3] virTestCompareToULL: Use VIR_AUTOFREE()
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(a)redhat.com>
---
tests/testutils.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
7:20 a.m.
New subject: [libvirt] [PATCH v2 2/3] virTestCompareToULL: Rename local variables
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(a)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
7:46 a.m.
New subject: [libvirt] [PATCH v2 2/3] virTestCompareToULL: Rename local variables
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(a)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(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
8:04 a.m.
New subject: [libvirt] [PATCH v2 2/3] virTestCompareToULL: Rename local variables
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:24 a.m.
New subject: [libvirt] [PATCH v2 2/3] virTestCompareToULL: Rename local variables
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(a)redhat.com>
---
tests/testutils.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
7:20 a.m.
New subject: [libvirt] [PATCH v2 3/3] testutils: Explicitly name virTestCompare*() arguments
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(a)redhat.com>
Reviewed-by: John Ferlan <jferlan(a)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
7:49 a.m.
New subject: [libvirt] [PATCH v2 3/3] testutils: Explicitly name virTestCompare*() arguments
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(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
8:24 a.m.
New subject: [libvirt] [PATCH v2 3/3] testutils: Explicitly name virTestCompare*() arguments
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(a)redhat.com>
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
---
tests/testutils.c | 34 +++++++++++++---------------------
tests/testutils.h | 10 +++++-----
2 files changed, 18 insertions(+), 26 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
2135
days inactive
2135
days old
10 comments
4 participants
participants (4)
-
Andrea Bolognani -
Ján Tomko -
Martin Kletzander -
Michal Privoznik