diff mbox

util: add is_equal to UUID API

Message ID 20171124143206.28833-1-rkagan@virtuozzo.com (mailing list archive)
State New, archived
Headers show

Commit Message

Roman Kagan Nov. 24, 2017, 2:32 p.m. UTC 
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(-)

Comments

Thomas Huth Nov. 24, 2017, 5:34 p.m. UTC | #1 
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
Roman Kagan Nov. 27, 2017, 11:02 a.m. UTC | #2 
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.
Daniel P. Berrangé Nov. 27, 2017, 11:45 a.m. UTC | #3 
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
Roman Kagan Nov. 27, 2017, 12:24 p.m. UTC | #4 
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.
diff mbox

Patch

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)