Re: [Qemu-devel] [PATCH v6 1/9] util: Add UUID API

[Fam Zheng <famz@redhat.com>]

On Sat, 09/17 16:29, Eric Blake wrote:
> On 08/17/2016 02:28 AM, Fam Zheng wrote:
> > A number of different places across the code base use CONFIG_UUID. Some
> > of them are soft dependency, some are not built if libuuid is not
> > available, some come with dummy fallback, some throws runtime error.
> > 
> > It is hard to maintain, and hard to reason for users.
> > 
> > Since UUID is a simple standard with only a small number of operations,
> > it is cleaner to have a central support in libqemuutil. This patch adds
> > qemu_uuid_* the functions so that all uuid users in the code base can
> 
> s/the functions so/functions/
> 
> > rely on. Except for qemu_uuid_generate which is new code, all other
> > functions are just copy from existing fallbacks from other files.
> > 
> > Note that qemu_uuid_parse is moved without updating the function
> > signature to use QemuUUID, to keep this patch simple.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> 
> > +++ b/include/qemu/uuid.h
> > @@ -0,0 +1,59 @@
> > +/*
> > + *  QEMU UUID functions
> > + *
> > + *  Copyright 2016 Red Hat, Inc.,
> 
> Why the trailing comma?

Simply a typo. Will fix.

> 
> > +++ b/util/uuid.c
> > @@ -0,0 +1,92 @@
> > +/*
> > + *  QEMU UUID functions
> > + *
> > + *  Copyright 2016 Red Hat, Inc.,
> 
> copy-and-paste? :)
> 
> 
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu-common.h"
> > +#include "qemu/uuid.h"
> > +#include "qemu/bswap.h"
> > +#include <glib.h>
> 
> Do you still need the <glib.h> header here? And if so, shouldn't it be
> right after osdep.h?

Yes, it will be removed.

> 
> > +
> > +void qemu_uuid_generate(QemuUUID *uuid)
> > +{
> > +    int i;
> > +    uint32_t tmp[4];
> > +
> > +    QEMU_BUILD_BUG_ON(sizeof(QemuUUID) != 16);
> > +
> > +    for (i = 0; i < 4; ++i) {
> > +        tmp[i] = g_random_int();
> > +    }
> > +    memcpy(uuid, tmp, sizeof(tmp));
> > +    /* Set the two most significant bits (bits 6 and 7) of the
> > +      clock_seq_hi_and_reserved to zero and one, respectively. */
> > +    uuid->data[8] = (uuid->data[8] & 0x3f) | 0x80;
> > +    /* Set the four most significant bits (bits 12 through 15) of the
> > +      time_hi_and_version field to the 4-bit version number.
> > +      */
> > +    uuid->data[6] = (uuid->data[6] & 0xf) | 0x40;
> > +}
> 
> Looks okay.
> 
> > +
> > +int qemu_uuid_is_null(const QemuUUID *uu)
> > +{
> > +    QemuUUID null_uuid = { 0 };
> 
> Could make this static, so that it doesn't have to be zeroed on every
> entry to this function (but in a separate patch, since this one just
> moved it, right?)

Sure, will do.

> 
> > +    return memcmp(uu, &null_uuid, sizeof(QemuUUID)) == 0;
> > +}
> > +
> 
> Once the nits are cleaned up, you can add:
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Fam