On Mon, Aug 08, 2016 at 01:07:33PM +0200, Markus Armbruster wrote: > Fam Zheng <f...@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 <f...@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 <f...@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 <f...@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" >