[Qemu-devel] [PATCH] util: add is_equal to UUID API

Roman Kagan posted 1 patch 6 years, 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20171124143206.28833-1-rkagan@virtuozzo.com
Test checkpatch passed
Test docker passed
Test ppc passed
Test s390x passed
There is a newer version of this series
include/qemu/uuid.h |  2 ++
tests/test-uuid.c   | 24 ++++++++++++++++++++++++
util/uuid.c         |  7 ++++++-
3 files changed, 32 insertions(+), 1 deletion(-)
[Qemu-devel] [PATCH] util: add is_equal to UUID API
Posted by Roman Kagan 6 years, 4 months ago
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


Re: [Qemu-devel] [PATCH] util: add is_equal to UUID API
Posted by Thomas Huth 6 years, 4 months ago
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

Re: [Qemu-devel] [PATCH] util: add is_equal to UUID API
Posted by Roman Kagan 6 years, 4 months ago
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.

Re: [Qemu-devel] [PATCH] util: add is_equal to UUID API
Posted by Daniel P. Berrange 6 years, 4 months ago
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 :|

Re: [Qemu-devel] [PATCH] util: add is_equal to UUID API
Posted by Roman Kagan 6 years, 4 months ago
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.