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 <f...@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 <ebl...@redhat.com>
Thanks!
Fam
