* [Qemu-devel] [PATCH] util: add is_equal to UUID API
@ 2017-11-24 14:32 Roman Kagan
2017-11-24 17:34 ` Thomas Huth
0 siblings, 1 reply; 5+ messages in thread
From: Roman Kagan @ 2017-11-24 14:32 UTC (permalink / raw)
To: qemu-devel, Fam Zheng; +Cc: Denis V. Lunev
It's going to be useful, in particular, in VMBus code massively using
uuids aka GUIDs.
Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
include/qemu/uuid.h | 2 ++
tests/test-uuid.c | 24 ++++++++++++++++++++++++
util/uuid.c | 7 ++++++-
3 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/include/qemu/uuid.h b/include/qemu/uuid.h
index afe4840296..09489ce5c5 100644
--- a/include/qemu/uuid.h
+++ b/include/qemu/uuid.h
@@ -48,6 +48,8 @@ void qemu_uuid_generate(QemuUUID *out);
int qemu_uuid_is_null(const QemuUUID *uu);
+int qemu_uuid_is_equal(const QemuUUID *lhv, const QemuUUID *rhv);
+
void qemu_uuid_unparse(const QemuUUID *uuid, char *out);
char *qemu_uuid_unparse_strdup(const QemuUUID *uuid);
diff --git a/tests/test-uuid.c b/tests/test-uuid.c
index d3a2791fd4..c6c8148117 100644
--- a/tests/test-uuid.c
+++ b/tests/test-uuid.c
@@ -116,6 +116,29 @@ static void test_uuid_is_null(void)
g_assert_false(qemu_uuid_is_null(&uuid_not_null_2));
}
+static void test_uuid_is_equal(void)
+{
+ int i;
+ QemuUUID uuid;
+ QemuUUID uuid_null = { };
+ QemuUUID uuid_not_null = { { {
+ 0x58, 0x6e, 0xce, 0x27, 0x7f, 0x09, 0x41, 0xe0,
+ 0x9e, 0x74, 0xe9, 0x01, 0x31, 0x7e, 0x9d, 0x42
+ } } };
+ QemuUUID uuid_null_2 = uuid_null;
+ QemuUUID uuid_not_null_2 = uuid_not_null;
+
+ g_assert(qemu_uuid_is_equal(&uuid_null, &uuid_null_2));
+ g_assert(qemu_uuid_is_equal(&uuid_not_null, &uuid_not_null_2));
+ g_assert_false(qemu_uuid_is_equal(&uuid_null, &uuid_not_null));
+
+ for (i = 0; i < 100; ++i) {
+ qemu_uuid_generate(&uuid);
+ g_assert_false(qemu_uuid_is_equal(&uuid_null, &uuid));
+ g_assert_false(qemu_uuid_is_equal(&uuid_not_null, &uuid));
+ }
+}
+
static void test_uuid_parse(void)
{
int i, r;
@@ -170,6 +193,7 @@ int main(int argc, char **argv)
g_test_init(&argc, &argv, NULL);
g_test_add_func("/uuid/generate", test_uuid_generate);
g_test_add_func("/uuid/is_null", test_uuid_is_null);
+ g_test_add_func("/uuid/is_equal", test_uuid_is_equal);
g_test_add_func("/uuid/parse", test_uuid_parse);
g_test_add_func("/uuid/unparse", test_uuid_unparse);
g_test_add_func("/uuid/unparse_strdup", test_uuid_unparse_strdup);
diff --git a/util/uuid.c b/util/uuid.c
index dd6b5fdf05..ebf06c049a 100644
--- a/util/uuid.c
+++ b/util/uuid.c
@@ -41,7 +41,12 @@ void qemu_uuid_generate(QemuUUID *uuid)
int qemu_uuid_is_null(const QemuUUID *uu)
{
static QemuUUID null_uuid;
- return memcmp(uu, &null_uuid, sizeof(QemuUUID)) == 0;
+ return qemu_uuid_is_equal(uu, &null_uuid);
+}
+
+int qemu_uuid_is_equal(const QemuUUID *lhv, const QemuUUID *rhv)
+{
+ return memcmp(lhv, rhv, sizeof(QemuUUID)) == 0;
}
void qemu_uuid_unparse(const QemuUUID *uuid, char *out)
--
2.14.3
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH] util: add is_equal to UUID API
2017-11-24 14:32 [Qemu-devel] [PATCH] util: add is_equal to UUID API Roman Kagan
@ 2017-11-24 17:34 ` Thomas Huth
2017-11-27 11:02 ` Roman Kagan
2017-11-27 11:45 ` Daniel P. Berrange
0 siblings, 2 replies; 5+ messages in thread
From: Thomas Huth @ 2017-11-24 17:34 UTC (permalink / raw)
To: Roman Kagan, qemu-devel, Fam Zheng; +Cc: Denis V. Lunev
On 24.11.2017 15:32, Roman Kagan wrote:
> It's going to be useful, in particular, in VMBus code massively using
> uuids aka GUIDs.
>
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> ---
> include/qemu/uuid.h | 2 ++
> tests/test-uuid.c | 24 ++++++++++++++++++++++++
> util/uuid.c | 7 ++++++-
> 3 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/include/qemu/uuid.h b/include/qemu/uuid.h
> index afe4840296..09489ce5c5 100644
> --- a/include/qemu/uuid.h
> +++ b/include/qemu/uuid.h
> @@ -48,6 +48,8 @@ void qemu_uuid_generate(QemuUUID *out);
>
> int qemu_uuid_is_null(const QemuUUID *uu);
>
> +int qemu_uuid_is_equal(const QemuUUID *lhv, const QemuUUID *rhv);
> +
> void qemu_uuid_unparse(const QemuUUID *uuid, char *out);
>
> char *qemu_uuid_unparse_strdup(const QemuUUID *uuid);
> diff --git a/tests/test-uuid.c b/tests/test-uuid.c
> index d3a2791fd4..c6c8148117 100644
> --- a/tests/test-uuid.c
> +++ b/tests/test-uuid.c
> @@ -116,6 +116,29 @@ static void test_uuid_is_null(void)
> g_assert_false(qemu_uuid_is_null(&uuid_not_null_2));
> }
>
> +static void test_uuid_is_equal(void)
> +{
> + int i;
> + QemuUUID uuid;
> + QemuUUID uuid_null = { };
> + QemuUUID uuid_not_null = { { {
> + 0x58, 0x6e, 0xce, 0x27, 0x7f, 0x09, 0x41, 0xe0,
> + 0x9e, 0x74, 0xe9, 0x01, 0x31, 0x7e, 0x9d, 0x42
> + } } };
> + QemuUUID uuid_null_2 = uuid_null;
> + QemuUUID uuid_not_null_2 = uuid_not_null;
> +
> + g_assert(qemu_uuid_is_equal(&uuid_null, &uuid_null_2));
> + g_assert(qemu_uuid_is_equal(&uuid_not_null, &uuid_not_null_2));
> + g_assert_false(qemu_uuid_is_equal(&uuid_null, &uuid_not_null));
> +
> + for (i = 0; i < 100; ++i) {
> + qemu_uuid_generate(&uuid);
> + g_assert_false(qemu_uuid_is_equal(&uuid_null, &uuid));
> + g_assert_false(qemu_uuid_is_equal(&uuid_not_null, &uuid));
Isn't there a very low chance that the last line triggers by accident?
Or uuid_no_null guaranteed to not match the generated one? In the latter
case, a comment with a short explanation might be helpful here...
Thomas
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH] util: add is_equal to UUID API
2017-11-24 17:34 ` Thomas Huth
@ 2017-11-27 11:02 ` Roman Kagan
2017-11-27 11:45 ` Daniel P. Berrange
1 sibling, 0 replies; 5+ messages in thread
From: Roman Kagan @ 2017-11-27 11:02 UTC (permalink / raw)
To: Thomas Huth; +Cc: qemu-devel, Fam Zheng, Denis V. Lunev
On Fri, Nov 24, 2017 at 06:34:03PM +0100, Thomas Huth wrote:
> On 24.11.2017 15:32, Roman Kagan wrote:
> > +++ b/tests/test-uuid.c
> > @@ -116,6 +116,29 @@ static void test_uuid_is_null(void)
> > g_assert_false(qemu_uuid_is_null(&uuid_not_null_2));
> > }
> >
> > +static void test_uuid_is_equal(void)
> > +{
> > + int i;
> > + QemuUUID uuid;
> > + QemuUUID uuid_null = { };
> > + QemuUUID uuid_not_null = { { {
> > + 0x58, 0x6e, 0xce, 0x27, 0x7f, 0x09, 0x41, 0xe0,
> > + 0x9e, 0x74, 0xe9, 0x01, 0x31, 0x7e, 0x9d, 0x42
> > + } } };
> > + QemuUUID uuid_null_2 = uuid_null;
> > + QemuUUID uuid_not_null_2 = uuid_not_null;
> > +
> > + g_assert(qemu_uuid_is_equal(&uuid_null, &uuid_null_2));
> > + g_assert(qemu_uuid_is_equal(&uuid_not_null, &uuid_not_null_2));
> > + g_assert_false(qemu_uuid_is_equal(&uuid_null, &uuid_not_null));
> > +
> > + for (i = 0; i < 100; ++i) {
> > + qemu_uuid_generate(&uuid);
> > + g_assert_false(qemu_uuid_is_equal(&uuid_null, &uuid));
> > + g_assert_false(qemu_uuid_is_equal(&uuid_not_null, &uuid));
>
> Isn't there a very low chance that the last line triggers by accident?
> Or uuid_no_null guaranteed to not match the generated one? In the latter
> case, a comment with a short explanation might be helpful here...
No, uuid_not_null passes the same validity criteria as the generated
ones so the collision probablity is not zero.
If we had an ideal random number generator, the 122 random bits would
coincide with a probability of 1 / (2 ** 122) ~= 2e-37 which I guess is
small enough in practical terms, so as a byproduct we'll test how random
the generated uuids are.
Which BTW may need certain attention: currently qemu_uuid_generate()
uses g_random_int() while, say, vmgenid wants it "cryptographically
random"random".
But that's another story...
Roman.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH] util: add is_equal to UUID API
2017-11-24 17:34 ` Thomas Huth
2017-11-27 11:02 ` Roman Kagan
@ 2017-11-27 11:45 ` Daniel P. Berrange
2017-11-27 12:24 ` Roman Kagan
1 sibling, 1 reply; 5+ messages in thread
From: Daniel P. Berrange @ 2017-11-27 11:45 UTC (permalink / raw)
To: Thomas Huth; +Cc: Roman Kagan, qemu-devel, Fam Zheng, Denis V. Lunev
On Fri, Nov 24, 2017 at 06:34:03PM +0100, Thomas Huth wrote:
> On 24.11.2017 15:32, Roman Kagan wrote:
> > It's going to be useful, in particular, in VMBus code massively using
> > uuids aka GUIDs.
> >
> > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> > ---
> > include/qemu/uuid.h | 2 ++
> > tests/test-uuid.c | 24 ++++++++++++++++++++++++
> > util/uuid.c | 7 ++++++-
> > 3 files changed, 32 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/qemu/uuid.h b/include/qemu/uuid.h
> > index afe4840296..09489ce5c5 100644
> > --- a/include/qemu/uuid.h
> > +++ b/include/qemu/uuid.h
> > @@ -48,6 +48,8 @@ void qemu_uuid_generate(QemuUUID *out);
> >
> > int qemu_uuid_is_null(const QemuUUID *uu);
> >
> > +int qemu_uuid_is_equal(const QemuUUID *lhv, const QemuUUID *rhv);
> > +
> > void qemu_uuid_unparse(const QemuUUID *uuid, char *out);
> >
> > char *qemu_uuid_unparse_strdup(const QemuUUID *uuid);
> > diff --git a/tests/test-uuid.c b/tests/test-uuid.c
> > index d3a2791fd4..c6c8148117 100644
> > --- a/tests/test-uuid.c
> > +++ b/tests/test-uuid.c
> > @@ -116,6 +116,29 @@ static void test_uuid_is_null(void)
> > g_assert_false(qemu_uuid_is_null(&uuid_not_null_2));
> > }
> >
> > +static void test_uuid_is_equal(void)
> > +{
> > + int i;
> > + QemuUUID uuid;
> > + QemuUUID uuid_null = { };
> > + QemuUUID uuid_not_null = { { {
> > + 0x58, 0x6e, 0xce, 0x27, 0x7f, 0x09, 0x41, 0xe0,
> > + 0x9e, 0x74, 0xe9, 0x01, 0x31, 0x7e, 0x9d, 0x42
> > + } } };
> > + QemuUUID uuid_null_2 = uuid_null;
> > + QemuUUID uuid_not_null_2 = uuid_not_null;
> > +
> > + g_assert(qemu_uuid_is_equal(&uuid_null, &uuid_null_2));
> > + g_assert(qemu_uuid_is_equal(&uuid_not_null, &uuid_not_null_2));
> > + g_assert_false(qemu_uuid_is_equal(&uuid_null, &uuid_not_null));
> > +
> > + for (i = 0; i < 100; ++i) {
> > + qemu_uuid_generate(&uuid);
> > + g_assert_false(qemu_uuid_is_equal(&uuid_null, &uuid));
> > + g_assert_false(qemu_uuid_is_equal(&uuid_not_null, &uuid));
>
> Isn't there a very low chance that the last line triggers by accident?
> Or uuid_no_null guaranteed to not match the generated one? In the latter
> case, a comment with a short explanation might be helpful here...
Regardless of that question, I think this is rather overkill when we are
just validating the memcmp() works correct in qemu_uuid_is_equal. The
for() loop here is not really adding any value over what the earlier
asserts already did. In fact the for() loop is arguably testing the
qemu_uuid_generate method, so better done in a separate unit test.
IOW, I would just suggest deleting this for() loop as its adding no
value.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH] util: add is_equal to UUID API
2017-11-27 11:45 ` Daniel P. Berrange
@ 2017-11-27 12:24 ` Roman Kagan
0 siblings, 0 replies; 5+ messages in thread
From: Roman Kagan @ 2017-11-27 12:24 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: Thomas Huth, qemu-devel, Fam Zheng, Denis V. Lunev
On Mon, Nov 27, 2017 at 11:45:52AM +0000, Daniel P. Berrange wrote:
> On Fri, Nov 24, 2017 at 06:34:03PM +0100, Thomas Huth wrote:
> > On 24.11.2017 15:32, Roman Kagan wrote:
> > > It's going to be useful, in particular, in VMBus code massively using
> > > uuids aka GUIDs.
> > >
> > > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> > > ---
> > > include/qemu/uuid.h | 2 ++
> > > tests/test-uuid.c | 24 ++++++++++++++++++++++++
> > > util/uuid.c | 7 ++++++-
> > > 3 files changed, 32 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/qemu/uuid.h b/include/qemu/uuid.h
> > > index afe4840296..09489ce5c5 100644
> > > --- a/include/qemu/uuid.h
> > > +++ b/include/qemu/uuid.h
> > > @@ -48,6 +48,8 @@ void qemu_uuid_generate(QemuUUID *out);
> > >
> > > int qemu_uuid_is_null(const QemuUUID *uu);
> > >
> > > +int qemu_uuid_is_equal(const QemuUUID *lhv, const QemuUUID *rhv);
> > > +
> > > void qemu_uuid_unparse(const QemuUUID *uuid, char *out);
> > >
> > > char *qemu_uuid_unparse_strdup(const QemuUUID *uuid);
> > > diff --git a/tests/test-uuid.c b/tests/test-uuid.c
> > > index d3a2791fd4..c6c8148117 100644
> > > --- a/tests/test-uuid.c
> > > +++ b/tests/test-uuid.c
> > > @@ -116,6 +116,29 @@ static void test_uuid_is_null(void)
> > > g_assert_false(qemu_uuid_is_null(&uuid_not_null_2));
> > > }
> > >
> > > +static void test_uuid_is_equal(void)
> > > +{
> > > + int i;
> > > + QemuUUID uuid;
> > > + QemuUUID uuid_null = { };
> > > + QemuUUID uuid_not_null = { { {
> > > + 0x58, 0x6e, 0xce, 0x27, 0x7f, 0x09, 0x41, 0xe0,
> > > + 0x9e, 0x74, 0xe9, 0x01, 0x31, 0x7e, 0x9d, 0x42
> > > + } } };
> > > + QemuUUID uuid_null_2 = uuid_null;
> > > + QemuUUID uuid_not_null_2 = uuid_not_null;
> > > +
> > > + g_assert(qemu_uuid_is_equal(&uuid_null, &uuid_null_2));
> > > + g_assert(qemu_uuid_is_equal(&uuid_not_null, &uuid_not_null_2));
> > > + g_assert_false(qemu_uuid_is_equal(&uuid_null, &uuid_not_null));
> > > +
> > > + for (i = 0; i < 100; ++i) {
> > > + qemu_uuid_generate(&uuid);
> > > + g_assert_false(qemu_uuid_is_equal(&uuid_null, &uuid));
> > > + g_assert_false(qemu_uuid_is_equal(&uuid_not_null, &uuid));
> >
> > Isn't there a very low chance that the last line triggers by accident?
> > Or uuid_no_null guaranteed to not match the generated one? In the latter
> > case, a comment with a short explanation might be helpful here...
>
> Regardless of that question, I think this is rather overkill when we are
> just validating the memcmp() works correct in qemu_uuid_is_equal. The
> for() loop here is not really adding any value over what the earlier
> asserts already did. In fact the for() loop is arguably testing the
> qemu_uuid_generate method, so better done in a separate unit test.
>
> IOW, I would just suggest deleting this for() loop as its adding no
> value.
Well I thought it was just too dumb to have a testcase that tests
basically nothing but memcmp, so I added this for() loop. I guess I'd
better just drop this as a separate testcase, and merge the assertions
into test_uuid_generate instead.
Roman.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-11-27 12:24 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-24 14:32 [Qemu-devel] [PATCH] util: add is_equal to UUID API Roman Kagan
2017-11-24 17:34 ` Thomas Huth
2017-11-27 11:02 ` Roman Kagan
2017-11-27 11:45 ` Daniel P. Berrange
2017-11-27 12:24 ` Roman Kagan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.