| Message ID | 1470129518-21087-2-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
----- Original Message ----- > From: "Fam Zheng" <famz@redhat.com> > To: qemu-devel@nongnu.org > Cc: famz@redhat.com, berrange@redhat.com, pbonzini@redhat.com, kwolf@redhat.com, mreitz@redhat.com, > mdroth@linux.vnet.ibm.com, armbru@redhat.com, sw@weilnetz.de, qemu-block@nongnu.org > Sent: Tuesday, August 2, 2016 11:18:32 AM > Subject: [PATCH 1/7] util: Add UUID API > > 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. How is g_random_* seeded? Paolo
On Tue, 08/02 15:45, Paolo Bonzini wrote: > > > ----- Original Message ----- > > From: "Fam Zheng" <famz@redhat.com> > > To: qemu-devel@nongnu.org > > Cc: famz@redhat.com, berrange@redhat.com, pbonzini@redhat.com, kwolf@redhat.com, mreitz@redhat.com, > > mdroth@linux.vnet.ibm.com, armbru@redhat.com, sw@weilnetz.de, qemu-block@nongnu.org > > Sent: Tuesday, August 2, 2016 11:18:32 AM > > Subject: [PATCH 1/7] util: Add UUID API > > > > 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. > > How is g_random_* seeded? According to glib doc: > GLib changed the seeding algorithm for the pseudo-random number generator > Mersenne Twister, as used by GRand. The urandom source is /dev/urandom (or time based if unavailable). (RFC 4122 explicitly accepts pseudo-random.) Fam
On Wed, Aug 03, 2016 at 10:36:40AM +0800, Fam Zheng wrote: > On Tue, 08/02 15:45, Paolo Bonzini wrote: > > > > > > ----- Original Message ----- > > > From: "Fam Zheng" <famz@redhat.com> > > > To: qemu-devel@nongnu.org > > > Cc: famz@redhat.com, berrange@redhat.com, pbonzini@redhat.com, kwolf@redhat.com, mreitz@redhat.com, > > > mdroth@linux.vnet.ibm.com, armbru@redhat.com, sw@weilnetz.de, qemu-block@nongnu.org > > > Sent: Tuesday, August 2, 2016 11:18:32 AM > > > Subject: [PATCH 1/7] util: Add UUID API > > > > > > 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. > > > > How is g_random_* seeded? > > According to glib doc: > > > GLib changed the seeding algorithm for the pseudo-random number generator > > Mersenne Twister, as used by GRand. > > The urandom source is /dev/urandom (or time based if unavailable). > > (RFC 4122 explicitly accepts pseudo-random.) > > Fam > To piggyback on Fam's answer: It is as if qemu called g_rand_new() [1] for a global static GRand struct. The g_random_* functions use the glib default global GRand struct. If you don't set the global seed yourself with g_random_set_seed(), then the first call into one of the g_random_ functions will create the global GRand struct seeded from /dev/urandom (if available), or the current time (if /dev/urandom is not available). [1] https://developer.gnome.org/glib/stable/glib-Random-Numbers.html#g-rand-new -Jeff
On 03/08/2016 04:36, Fam Zheng wrote: > On Tue, 08/02 15:45, Paolo Bonzini wrote: >> >> >> ----- Original Message ----- >>> From: "Fam Zheng" <famz@redhat.com> >>> To: qemu-devel@nongnu.org >>> Cc: famz@redhat.com, berrange@redhat.com, pbonzini@redhat.com, kwolf@redhat.com, mreitz@redhat.com, >>> mdroth@linux.vnet.ibm.com, armbru@redhat.com, sw@weilnetz.de, qemu-block@nongnu.org >>> Sent: Tuesday, August 2, 2016 11:18:32 AM >>> Subject: [PATCH 1/7] util: Add UUID API >>> >>> 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. >> >> How is g_random_* seeded? > > According to glib doc: > >> GLib changed the seeding algorithm for the pseudo-random number generator >> Mersenne Twister, as used by GRand. > > The urandom source is /dev/urandom (or time based if unavailable). That's great then. Paolo
Hi On Thu, Aug 4, 2016 at 4:44 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 03/08/2016 04:36, Fam Zheng wrote: > > On Tue, 08/02 15:45, Paolo Bonzini wrote: > >> > >> > >> ----- Original Message ----- > >>> From: "Fam Zheng" <famz@redhat.com> > >>> To: qemu-devel@nongnu.org > >>> Cc: famz@redhat.com, berrange@redhat.com, pbonzini@redhat.com, > kwolf@redhat.com, mreitz@redhat.com, > >>> mdroth@linux.vnet.ibm.com, armbru@redhat.com, sw@weilnetz.de, > qemu-block@nongnu.org > >>> Sent: Tuesday, August 2, 2016 11:18:32 AM > >>> Subject: [PATCH 1/7] util: Add UUID API > >>> > >>> 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. > For info, there has been glib UUID proposal for a while: https://bugzilla.gnome.org/show_bug.cgi?id=639078 Although quite ready, the discussion stopped because some dev believe g_uuid_string_random() is all you need. Anyway, if ever accepted, it would take another 5y or so to be acceptable for qemu :)
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 <famz@redhat.com> > --- > 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 <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 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 <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(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 > >
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 <famz@redhat.com> > --- > 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 It would be nice to see you add a tests/test-uuid.c unit test to exercise all the new utility APIs you're adding & check their corner cases. Regards, Daniel
On Thu, 08/04 16:48, Daniel P. Berrange wrote: > 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 <famz@redhat.com> > > --- > > 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 > > It would be nice to see you add a tests/test-uuid.c unit test to > exercise all the new utility APIs you're adding & check their > corner cases. Sure, I'll add a test case. Fam
On Thu, 08/04 11:33, Jeff Cody wrote: > > +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) Right, I'm using a wrong endianness. Will fix this. Fam
On Thu, 08/04 11:33, Jeff Cody wrote: > It makes sense to represent the uuid internally as a struct with the octet > groupings of uint32_t, uint16_t, etc.., for endianness: BTW I'm not sure it's worth to add that internal representation. libuuid uses local endianness when generating and setting version bits, then convert output to BE anyway. Fam
Am 02.08.2016 um 11:18 schrieb Fam Zheng: [...] > +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(); I suggest using uint32_t instead of guint32. Up to now, nearly all QEMU code uses the POSIX data types. Regards Stefan
On Mon, 08/08 08:30, Stefan Weil wrote: > Am 02.08.2016 um 11:18 schrieb Fam Zheng: > [...] > > +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(); > > I suggest using uint32_t instead of guint32. > Up to now, nearly all QEMU code uses the POSIX data types. This is merely to keep consistent with the g_random_int() return type. If the two types had any chance to vary (surely they don't), the uint32_t way would look like this: *((uint32_t *)out + 0) = (uint32_t)g_random_int(); So I think the current way is fine. Fam
Am 08.08.2016 um 08:33 schrieb Fam Zheng: > On Mon, 08/08 08:30, Stefan Weil wrote: >> Am 02.08.2016 um 11:18 schrieb Fam Zheng: >> [...] >>> +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(); >> >> I suggest using uint32_t instead of guint32. >> Up to now, nearly all QEMU code uses the POSIX data types. > > This is merely to keep consistent with the g_random_int() return type. If the > two types had any chance to vary (surely they don't), the uint32_t way would > look like this: > > *((uint32_t *)out + 0) = (uint32_t)g_random_int(); > > So I think the current way is fine. > > Fam > Yes, technically it is fine, and I know that you had chosen guint32 to match the type returned by the GLIB function. I have a similar issue with the Windows API which also uses its own data types. Many software APIs bring their own data types with them. Up to now, we obviously avoided using guint32 although we are using the GLIB API for several years. I also noticed (after I had send my comment) that you just have made a v2 of your series, so I'm sorry that my comment came so late. Stefan
Stefan Weil <sw@weilnetz.de> writes: > Am 08.08.2016 um 08:33 schrieb Fam Zheng: >> On Mon, 08/08 08:30, Stefan Weil wrote: >>> Am 02.08.2016 um 11:18 schrieb Fam Zheng: >>> [...] >>>> +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(); >>> >>> I suggest using uint32_t instead of guint32. >>> Up to now, nearly all QEMU code uses the POSIX data types. >> >> This is merely to keep consistent with the g_random_int() return type. If the >> two types had any chance to vary (surely they don't), the uint32_t way would >> look like this: >> >> *((uint32_t *)out + 0) = (uint32_t)g_random_int(); Why would you need to cast the value of g_random_int()? >> So I think the current way is fine. >> >> Fam >> > > Yes, technically it is fine, and I know that you had chosen > guint32 to match the type returned by the GLIB function. > > I have a similar issue with the Windows API which also uses its > own data types. Many software APIs bring their own data types > with them. Up to now, we obviously avoided using guint32 > although we are using the GLIB API for several years. Yep. A few uses have crept in here and there, and I hate every one of them. [...]
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 <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 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 <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(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; + 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"
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 | 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