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

[Eric Blake]

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 <address@hidden>
> ---

> +++ b/include/qemu/uuid.h
> @@ -0,0 +1,59 @@
> +/*
> + *  QEMU UUID functions
> + *
> + *  Copyright 2016 Red Hat, Inc.,

Why the trailing comma?

> +++ 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?

> +
> +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?)

> +    return memcmp(uu, &null_uuid, sizeof(QemuUUID)) == 0;
> +}
> +

Once the nits are cleaned up, you can add:
Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature