Re: [Qemu-block] [PATCH 1/7] util: Add UUID API

[Jeff Cody]

On Tue, Aug 02, 2016 at 05:18:32PM +0800, 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
> rely on. Except for qemu_uuid_generate which is new code, all other
> functions are just copy from existing fallbacks from other files.
> 
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  arch_init.c             | 19 ---------------
>  block/iscsi.c           |  2 +-
>  hw/smbios/smbios.c      |  1 +
>  include/qemu/uuid.h     | 37 +++++++++++++++++++++++++++++
>  include/sysemu/sysemu.h |  4 ----
>  qmp.c                   |  1 +
>  stubs/uuid.c            |  2 +-
>  util/Makefile.objs      |  1 +
>  util/uuid.c             | 63 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  vl.c                    |  1 +
>  10 files changed, 106 insertions(+), 25 deletions(-)
>  create mode 100644 include/qemu/uuid.h
>  create mode 100644 util/uuid.c
> 
> diff --git a/arch_init.c b/arch_init.c
> index fa05973..5cc58b2 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -235,25 +235,6 @@ void audio_init(void)
>      }
>  }
>  
> -int qemu_uuid_parse(const char *str, uint8_t *uuid)
> -{
> -    int ret;
> -
> -    if (strlen(str) != 36) {
> -        return -1;
> -    }
> -
> -    ret = sscanf(str, UUID_FMT, &uuid[0], &uuid[1], &uuid[2], &uuid[3],
> -                 &uuid[4], &uuid[5], &uuid[6], &uuid[7], &uuid[8], &uuid[9],
> -                 &uuid[10], &uuid[11], &uuid[12], &uuid[13], &uuid[14],
> -                 &uuid[15]);
> -
> -    if (ret != 16) {
> -        return -1;
> -    }
> -    return 0;
> -}
> -
>  void do_acpitable_option(const QemuOpts *opts)
>  {
>  #ifdef TARGET_I386
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 95ce9e1..961ac76 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -36,7 +36,7 @@
>  #include "block/block_int.h"
>  #include "block/scsi.h"
>  #include "qemu/iov.h"
> -#include "sysemu/sysemu.h"
> +#include "qemu/uuid.h"
>  #include "qmp-commands.h"
>  #include "qapi/qmp/qstring.h"
>  #include "crypto/secret.h"
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index 74c7102..0705eb1 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -20,6 +20,7 @@
>  #include "qemu/config-file.h"
>  #include "qemu/error-report.h"
>  #include "sysemu/sysemu.h"
> +#include "qemu/uuid.h"
>  #include "sysemu/cpus.h"
>  #include "hw/smbios/smbios.h"
>  #include "hw/loader.h"
> diff --git a/include/qemu/uuid.h b/include/qemu/uuid.h
> new file mode 100644
> index 0000000..53d572f
> --- /dev/null
> +++ b/include/qemu/uuid.h
> @@ -0,0 +1,37 @@
> +/*
> + *  QEMU UUID functions
> + *
> + *  Copyright 2016 Red Hat, Inc.,
> + *
> + *  Authors:
> + *   Fam Zheng <address@hidden>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the Free
> + * Software Foundation; either version 2 of the License, or (at your option)
> + * any later version.
> + *
> + */
> +
> +#ifndef QEMU_UUID_H
> +#define QEMU_UUID_H
> +
> +#include "qemu-common.h"
> +
> +typedef unsigned char qemu_uuid_t[16];
> +
> +#define UUID_FMT "%02hhx%02hhx%02hhx%02hhx-" \
> +                 "%02hhx%02hhx-%02hhx%02hhx-" \
> +                 "%02hhx%02hhx-" \
> +                 "%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
> +#define UUID_NONE "00000000-0000-0000-0000-000000000000"
> +
> +void qemu_uuid_generate(qemu_uuid_t out);
> +
> +int qemu_uuid_is_null(const qemu_uuid_t uu);
> +
> +void qemu_uuid_unparse(const qemu_uuid_t uu, char *out);
> +
> +int qemu_uuid_parse(const char *str, uint8_t *uuid);
> +
> +#endif
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index ee7c760..6111950 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -18,10 +18,6 @@ extern const char *bios_name;
>  extern const char *qemu_name;
>  extern uint8_t qemu_uuid[];
>  extern bool qemu_uuid_set;
> -int qemu_uuid_parse(const char *str, uint8_t *uuid);
> -
> -#define UUID_FMT 
> "%02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
> -#define UUID_NONE "00000000-0000-0000-0000-000000000000"
>  
>  bool runstate_check(RunState state);
>  void runstate_set(RunState new_state);
> diff --git a/qmp.c b/qmp.c
> index b6d531e..7fbde29 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -35,6 +35,7 @@
>  #include "qom/object_interfaces.h"
>  #include "hw/mem/pc-dimm.h"
>  #include "hw/acpi/acpi_dev_interface.h"
> +#include "qemu/uuid.h"
>  
>  NameInfo *qmp_query_name(Error **errp)
>  {
> diff --git a/stubs/uuid.c b/stubs/uuid.c
> index 92ad717..a880de8 100644
> --- a/stubs/uuid.c
> +++ b/stubs/uuid.c
> @@ -1,6 +1,6 @@
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
> -#include "sysemu/sysemu.h"
> +#include "qemu/uuid.h"
>  #include "qmp-commands.h"
>  
>  UuidInfo *qmp_query_uuid(Error **errp)
> diff --git a/util/Makefile.objs b/util/Makefile.objs
> index 96cb1e0..31bba15 100644
> --- a/util/Makefile.objs
> +++ b/util/Makefile.objs
> @@ -20,6 +20,7 @@ util-obj-y += iov.o qemu-config.o qemu-sockets.o uri.o 
> notify.o
>  util-obj-y += qemu-option.o qemu-progress.o
>  util-obj-y += hexdump.o
>  util-obj-y += crc32c.o
> +util-obj-y += uuid.o
>  util-obj-y += throttle.o
>  util-obj-y += getauxval.o
>  util-obj-y += readline.o
> diff --git a/util/uuid.c b/util/uuid.c
> new file mode 100644
> index 0000000..ceca343
> --- /dev/null
> +++ b/util/uuid.c
> @@ -0,0 +1,63 @@
> +/*
> + *  QEMU UUID functions
> + *
> + *  Copyright 2016 Red Hat, Inc.,
> + *
> + *  Authors:
> + *   Fam Zheng <address@hidden>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the Free
> + * Software Foundation; either version 2 of the License, or (at your option)
> + * any later version.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu-common.h"
> +#include "qemu/uuid.h"
> +#include <glib.h>
> +
> +void qemu_uuid_generate(qemu_uuid_t out)
> +{
> +    /* Version 4 UUID, RFC4122 4.4. */
> +    QEMU_BUILD_BUG_ON(sizeof(qemu_uuid_t) != 16);
> +    *((guint32 *)out + 0) = g_random_int();
> +    *((guint32 *)out + 1) = g_random_int();
> +    *((guint32 *)out + 2) = g_random_int();
> +    *((guint32 *)out + 3) = g_random_int();
> +    out[7] = (out[7] & 0xf) | 0x40;

Hi Fam,

I think this gets the endianness wrong of the time_hi_and_version field.

I believe this should be:
    out[6] = (out[6] & 0xf) | 0x40;

(but I don't think we should generate it this way, see below)


Per RFC 4122, Section 4.1.2:

   The fields are presented with the most significant one first.
    [...]

   time_hi_and_version    unsigned 16   6-7    The high field of the
                          bit integer          timestamp multiplexed
                                               with the version number

For random UUID generation, Section 4.4:

      Set the four most significant bits (bits 12 through 15) of the
      time_hi_and_version field to the 4-bit version number from
      Section 4.1.3.


So octet 7 is really offset 6 in your array.


It makes sense to represent the uuid internally as a struct with the octet
groupings of uint32_t, uint16_t, etc.., for endianness:

https://github.com/karelzak/util-linux/blob/master/libuuid/src/uuidP.h#L48

(Note, in block/vhdx.h we also represent it that way, to make sure all
platforms copy the UUID out correctly to the image format header).

I'd recommend doing something similar to libuuid:

https://github.com/karelzak/util-linux/blob/master/libuuid/src/gen_uuid.c#L498

BTW, here is a UUID generated with this patch, compared to one that was
generated via libuuid (pulled from qemu-img create -f vhdx):

Current patch:
797cbee7 ffd0 5445  a6 fe ee 22 c2 eb 32 79
              ^^
libuuid:
3f2c08ba 9e5a 4c99  8b bc d4 bd d8 3a 6e e9 
              ^^
The underlined bytes should have their most significant nibble set to 0x4
for a random uuid.

Thanks,
Jeff

> +    out[8] = (out[8] & 0x3f) | 0x80;
> +}
> +
> +int qemu_uuid_is_null(const qemu_uuid_t uu)
> +{
> +    qemu_uuid_t null_uuid = { 0 };
> +    return memcmp(uu, null_uuid, sizeof(qemu_uuid_t)) == 0;
> +}
> +
> +void qemu_uuid_unparse(const qemu_uuid_t uu, char *out)
> +{
> +    snprintf(out, 37, UUID_FMT,
> +             uu[0], uu[1], uu[2], uu[3], uu[4], uu[5], uu[6], uu[7],
> +             uu[8], uu[9], uu[10], uu[11], uu[12], uu[13], uu[14], uu[15]);
> +}
> +
> +int qemu_uuid_parse(const char *str, uint8_t *uuid)
> +{
> +    int ret;
> +
> +    if (strlen(str) != 36) {
> +        return -1;
> +    }
> +
> +    ret = sscanf(str, UUID_FMT, &uuid[0], &uuid[1], &uuid[2], &uuid[3],
> +                 &uuid[4], &uuid[5], &uuid[6], &uuid[7], &uuid[8], &uuid[9],
> +                 &uuid[10], &uuid[11], &uuid[12], &uuid[13], &uuid[14],
> +                 &uuid[15]);
> +
> +    if (ret != 16) {
> +        return -1;
> +    }
> +    return 0;
> +}
> diff --git a/vl.c b/vl.c
> index e7c2c62..8b12f34 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -25,6 +25,7 @@
>  #include "qemu-version.h"
>  #include "qemu/cutils.h"
>  #include "qemu/help_option.h"
> +#include "qemu/uuid.h"
>  
>  #ifdef CONFIG_SECCOMP
>  #include "sysemu/seccomp.h"
> -- 
> 2.7.4
> 
>