Re: [Qemu-devel] [PATCH v2 1/8] util: Add UUID API

[Jeff Cody <jcody@redhat.com>]

On Mon, Aug 08, 2016 at 01:07:33PM +0200, Markus Armbruster wrote:
> Fam Zheng <famz@redhat.com> writes:
> 
> > 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 <famz@redhat.com>
> > ---
> >  arch_init.c             | 19 --------------
> >  block/iscsi.c           |  2 +-
> >  hw/smbios/smbios.c      |  1 +
> >  include/qemu/uuid.h     | 40 +++++++++++++++++++++++++++++
> >  include/sysemu/sysemu.h |  4 ---
> >  qmp.c                   |  1 +
> >  stubs/uuid.c            |  2 +-
> >  util/Makefile.objs      |  1 +
> >  util/uuid.c             | 68 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  vl.c                    |  1 +
> >  10 files changed, 114 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..854a208
> > --- /dev/null
> > +++ b/include/qemu/uuid.h
> > @@ -0,0 +1,40 @@
> > +/*
> > + *  QEMU UUID functions
> > + *
> > + *  Copyright 2016 Red Hat, Inc.,
> > + *
> > + *  Authors:
> > + *   Fam Zheng <famz@redhat.com>
> > + *
> > + * 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 QemuUUID[16];
> 
> I'm afraid this typedef is problematic.  Consider:
> 
>     void use_uuid(QemuUUID uuid)
>     {
>         printf("sizeof(uuid) %zd\n", sizeof(uuid));
>         uuid[0]++;
>     }
> 
> QemuUUID is obviously a typedef name, so a reasonable reader may assume
> (1) sizeof(uuid) is the size of the uuid, and (2) since uuid is passed
> by value, the increment is not visible outside the function.  Both
> assumptions are wrong, because array arguments degenerate into pointers.
> 
> I recommend to wrap it in a struct.
>

If we are going for a semantic drop-in replacement for libuuid's uuid_t,
Fam's typedef here is consistent with libuuid:

    typedef unsigned char uuid_t[16];

(Not to say that prohibits changing it, just pointing out there is value in
mimicking libuuid's interfaces).

> > +
> > +#define UUID_FMT "%02hhx%02hhx%02hhx%02hhx-" \
> > +                 "%02hhx%02hhx-%02hhx%02hhx-" \
> > +                 "%02hhx%02hhx-" \
> > +                 "%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
> > +
> > +#define UUID_FMT_LEN 36
> > +
> > +#define UUID_NONE "00000000-0000-0000-0000-000000000000"
> > +
> > +void qemu_uuid_generate(QemuUUID out);
> > +
> > +int qemu_uuid_is_null(const QemuUUID uu);
> > +
> > +void qemu_uuid_unparse(const QemuUUID 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..9e26560
> > --- /dev/null
> > +++ b/util/uuid.c
> > @@ -0,0 +1,68 @@
> > +/*
> > + *  QEMU UUID functions
> > + *
> > + *  Copyright 2016 Red Hat, Inc.,
> > + *
> > + *  Authors:
> > + *   Fam Zheng <famz@redhat.com>
> > + *
> > + * 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(QemuUUID out)
> > +{
> > +    /* Version 4 UUID, RFC4122 4.4. */
> > +    QEMU_BUILD_BUG_ON(sizeof(QemuUUID) != 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();
> 
> I loathe GLib's pointless duplication of the standard integer types.
> Sure we can't simply use uint32_t?
> 
> Possibly:
> 
>     uint32_t *out32 = (uint32_t)out;
>     out32[0] = g_random_int();
>     out32[1] = g_random_int();
>     out32[2] = g_random_int();
>     out32[3] = g_random_int();
> 
> > +    /* Set the two most significant bits (bits 6 and 7) of the
> > +      clock_seq_hi_and_reserved to zero and one, respectively. */
> > +    out[8] = (out[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.
> > +      */
> > +    out[6] = (out[6] & 0xf) | 0x40;
> > +}
> > +
> > +int qemu_uuid_is_null(const QemuUUID uu)
> > +{
> > +    QemuUUID null_uuid = { 0 };
> > +    return memcmp(uu, null_uuid, sizeof(QemuUUID)) == 0;
> > +}
> > +
> > +void qemu_uuid_unparse(const QemuUUID uu, char *out)
> 
> While there, you could write a function contract that spells out that
> @out[] must have space for 37 characters.
> 
> > +{
> > +    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"
>