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

[Jeff Cody]

On Tue, Aug 09, 2016 at 02:49:59PM +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.
> 
> 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>
> ---
>  arch_init.c             | 19 -----------
>  block/iscsi.c           |  2 +-
>  hw/smbios/smbios.c      |  1 +
>  include/qemu/uuid.h     | 48 ++++++++++++++++++++++++++
>  include/sysemu/sysemu.h |  4 ---
>  qmp.c                   |  1 +
>  stubs/uuid.c            |  2 +-
>  util/Makefile.objs      |  1 +
>  util/uuid.c             | 91 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  vl.c                    |  1 +
>  10 files changed, 145 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..80a75c2
> --- /dev/null
> +++ b/include/qemu/uuid.h
> @@ -0,0 +1,48 @@
> +/*
> + *  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"
> +
> +/* Version 4 UUID (pseudo random numbers), RFC4122 4.4. */
> +
> +typedef struct {
> +    unsigned char data[16];
> +} QemuUUID;
> +
> +#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(char *out, const QemuUUID *uuid);
> +
> +char *qemu_uuid_unparse_strdup(const QemuUUID *uuid);
> +
> +int qemu_uuid_parse(const char *str, uint8_t *uuid);
> +
> +void qemu_uuid_convert(QemuUUID *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..0b65ba3 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -18,6 +18,7 @@
>  #include "qemu/cutils.h"
>  #include "monitor/monitor.h"
>  #include "sysemu/sysemu.h"
> +#include "qemu/uuid.h"
>  #include "qmp-commands.h"
>  #include "sysemu/char.h"
>  #include "ui/qemu-spice.h"
> 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..80c89f0
> --- /dev/null
> +++ b/util/uuid.c
> @@ -0,0 +1,91 @@
> +/*
> + *  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 "qemu/bswap.h"
> +#include <glib.h>
> +
> +void qemu_uuid_generate(QemuUUID *uuid)
> +{
> +    int i;
> +    uint32_t *out = (uint32_t *)&uuid->data[0];
> +
> +    QEMU_BUILD_BUG_ON(sizeof(QemuUUID) != 16);
> +
> +    for (i = 0; i < 4; ++i) {
> +        out[i] = 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. */
> +    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;
> +}
> +
> +int qemu_uuid_is_null(const QemuUUID *uu)
> +{
> +    QemuUUID null_uuid = { 0 };
> +    return memcmp(uu, &null_uuid, sizeof(QemuUUID)) == 0;
> +}
> +
> +void qemu_uuid_unparse(char *out, const QemuUUID *uuid)
> +{
> +    const unsigned char *uu = &uuid->data[0];
> +    snprintf(out, UUID_FMT_LEN + 1, 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]);
> +}
> +
> +char *qemu_uuid_unparse_strdup(const QemuUUID *uuid)
> +{
> +    const unsigned char *uu = &uuid->data[0];
> +    return g_strdup_printf(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;
> +}
> +


> +/* Change UUID from CPU endian to big endian format (network byte order,
> + * standard, see RFC 4122) and vice versa.
> + */
> +void qemu_uuid_convert(QemuUUID *uuid)
> +{
> +    be32_to_cpus((uint32_t *)&uuid->data[0]);
> +    be16_to_cpus((uint16_t *)&uuid->data[4]);
> +    be16_to_cpus((uint16_t *)&uuid->data[6]);
> +}

Hmm... what's this function for?  The comment says it converts from CPU
endian to BE, but it appears to do the opposite (BE to CPU).

The new qemu_uuid_convert() replaces VDI's uuid_convert() in patch 3, but
that function does something different - it does blind bswaps.  This works
because the driver knows the UUID is always stored in LE format in the file
header, and converts UUID to the RFC BE format.  And since the internal
representation is known to always be in BE format, it can do a blanket bswap
again with the same function to write it back out to the header.

On a BE machine, qemu_uuid_convert() won't do what the VDI driver needs
(be??_to_cpus() is a noop).

So the short of it:  I think this new function should be explicitly called
something like "qemu_uuid_bswap()", and do blind bswaps.  That makes it a
drop in replacement for VDI.


...


Now for some odd stuff, but not directly dealing with your patch series:

This made me double check the VHDX driver, and how it handles the UUIDs.
The VHDX spec declares that everything is stored in LE format.  There
are some "known" UUIDs for metadata IDs defined in the spec, and when I
check those in our qemu-created VHDX files against MS-created VHDX files,
they are indeed both stored LE as expected, per spec:

Known UUID: 8141BF1D-A96F-4709-BA47-F233A8FAAB5F
Stored in file header as:

00300080  1d bf 41 81 6f a9 09 47  ba 47 f2 33 a8 fa ab 5f  |..A.o..G.G.3..._|
                               ^^
                               octet 6, time_hi_version

Yay!

However, thinking through this made me realize there is a bug in the VHDX
driver (my fault, too).  When we _generate_ a new UUID for the "head"
header, the uuid_generate() call will store it internally in BE format.  So
technically, we should be doing what VDI does, and bswap'ing UUIDs we
generate when we write them out; but instead, we are using cpu_to_le(),
which does nothing on a LE machine.  Looking at a qemu created VHDX file, we
can confirm that we are storing our generated header UUIDs incorrectly in
BE format, contrary to the VHDX spec:

00010010  82 8a b6 52 53 3d 48 e6  aa 33 5c 64 9e fe 41 4b  |...RS=H..3\d..AK|
                            ^^^^^
                            (These bytes should be swapped in the vhdx file)


This is where it gets weird - checking the MS Hyper-V created images we have
in sample_images, it appears MS _also_ stores those "head" header UUIDs
incorrectly. The only correctly stored LE UUIDs are the "known" metadata
UUIDs... so they have the same bug as our VHDX driver.  Yay?

MS "head" UUID stored incorrectly by Hyper-V:
===================================================
00010010  6d ea 90 8e 36 b6 49 1c  b7 d4 35 10 9e 60 0c 0c  |m...6.I...5..`..|
                            ^^^^^
                            (These bytes should be swapped in the vhdx file)

I would say "meh", as our goal is to be compatible in the real world for 3rd
party formats, rather than be strictly beholden to the spec.  However, no
celebration yet; MS's tool disk2vhd (to virtualize a physical disk) does
_not_ have that bug! Our sample image test-disk2vhd.vhdx.bz2 shows the
generated UUIDs in the "head" header in the VHDX spec-correct LE format:

Two MS "head" UUIDs stored correctly by disk2vhd:
===================================================
00020010  13 2b 30 81 aa c7 cd 47  8f 27 96 d4 c4 6b f8 ea  |.+0....G.'...k..|
                               ^^
00020020  1c 89 03 fd e5 29 d6 4a  8e e1 71 98 d3 b1 e2 63  |.....).J..q....c|
                               ^^
                               (correct LE position of RFC4122 octet 6)

So, I guess we should fix VHDX at some point, but it doesn't have to be in
this series, since the patches don't affect this VHDX behavior.


-Jeff

> diff --git a/vl.c b/vl.c
> index c4eeaff..3d96684 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
>