GNOME Bugzilla – Bug 639078
UUID support feature request
Last modified: 2017-02-16 06:01:52 UTC
I'm in the process of moving my own utility library to use glib as it's more portable, tested and feature rich, but I miss one thing - UUID handling, parsing and generation. Any hope for including it in future glib releases?
that would great indeed
libuuid from util-linux is not so easy to cross-compile, and I am not sure upstream would like patches for other systems. It's Modified BSD, so could be copied and adapted? Some other references: http://www.ietf.org/rfc/rfc4122.txt http://www.boost.org/doc/libs/1_52_0/libs/uuid/uuid.html http://doc.qt.digia.com/qt/quuid.html http://msdn.microsoft.com/en-us/library/system.guid.aspx http://docs.python.org/3/library/uuid.html http://git.kernel.org/?p=utils/util-linux/util-linux.git;a=blob;f=libuuid/src/uuid.h http://msdn.microsoft.com/en-us/library/windows/desktop/aa379205%28v=vs.85%29.aspx
NM generates UUIDs as well. Currently it uses libuuid directly (which isn't a problem since NM is also Linux-only), but it could switch to a glib API.
I would be pretty happy with that: typedef guint8 GUUID[16]; G_DEFINE_UUID(name,u0,u1..) - like linux libuuid GUUIDError { G_UUID_ERROR_INVALID_STRING G_UUID_ERROR_NOT_SUPPORTED } gboolean g_uuid_generate4(GUUID *, GError *) - more versions could be added later, they can take other arguments, so no enum there gchar* g_uuid_to_string() gboolean g_uuid_parse_string(GUUID *, gchar *, GError *) - it could accept common variants with enclosing {} or ()... gboolean g_uuid_is_nil() gboolean g_uuid_equal() guint g_uuid_hash() g_uuid_get_type()
I think it's better to embed the bytes in a struct, make it more a referenced value that we can later extend. I will work on a patch during the weekend
That doesn't feel like very glib-ish API. What exactly do you need to do? All NM would want is: gchar *g_uuid_generate (void) (or maybe "g_uuid_generate_random" or "g_uuid_generate(G_UUID_TYPE_RANDOM)" if we need to support the other types)
(In reply to comment #6) > That doesn't feel like very glib-ish API. What exactly do you need to do? > > All NM would want is: > > gchar *g_uuid_generate (void) > > (or maybe "g_uuid_generate_random" or "g_uuid_generate(G_UUID_TYPE_RANDOM)" if > we need to support the other types) Right, that is really the minimum I am looking for. A typical desktop application I know of that use uuid is GNote. I don't work on so many projects, but Boxes / libvirt-glib, spice-gtk, msitools/wix. Ie pretty much every project I worked own recently need a bit of uuid API. and libuuid is not portable (it used to be though), hence my motivation. Not only NM, I see tracker and rygel are users too and more. We could choose string as a preferend representation. Indeed it seems that many projects have choosen it. So no need for cmp/copy/hash/equal etc.. too bad it will not be typed.. The question is, one did choose strings, because it was easier? If I count right, we are talking about 37 bytes vs 16 bytes. Not a big deal probably. Though I use uint8[16] <-> string in spice-gtk and qemu, I see not so many projects using directly the byte array. However, many projects, Boxes, wixl/msitools, rygel and gnote, would want to parse a UUID, to check validity when reading from files or network. They don't, I wish we would propose a simple function for that that would throw an error. I was looking forward for a real UUID type in GLib. But if we prefer to stick to strings, that would probably cover nicely 90% of use case, even if we loose some type-ness and efficiency. Regarding the generate version, I think it's a good idea to use the version number in the function (like python). That makes it clear which method is used, and allows easy extensibility by adding the appropriate function in the future (it's just a few). Using only an enum is not an option, since other versions requires argument. I am really interested by the version 3/5 in particular, which should probably be recommended to use in various applications instead of pseudo-randomness (it is just a hash). Those version need the byte representation. It's hard to avoid the byte representation at some point. Since it's just a simple array of byte, why not make it a real type?...
(In reply to comment #7) > The question is, one did choose strings, because it was > easier? If I count right, we are talking about 37 bytes vs 16 bytes. Not a big > deal probably. well, NM uses strings because they get written out to (ASCII) files. > I was looking forward for a real UUID type in GLib. But if we prefer to stick > to strings, that would probably cover nicely 90% of use case, even if we loose > some type-ness and efficiency. Well, if there are multiple modules where a binary format would be more useful, then we should do that. (But with string-only convenience APIs too.)
Created attachment 233900 [details] [review] Add GUUID to GLib Add a new GUUID type representing a UUID described in RFC 4122. The version 3 to 5 of the RFC 4122 mechansims are provided. Many UUID users will just need a random string, which can be generated simply by calling the function g_uuid_random().
Created attachment 233901 [details] [review] Add G_TYPE_UUID
Created attachment 233905 [details] [review] Add GUUID to GLib Add a new GUUID type representing a UUID described in RFC 4122. The version 3 to 5 of the RFC 4122 mechansims are provided. Many UUID users will just need a random string, which can be generated simply by calling the function g_uuid_random().
Created attachment 233917 [details] [review] Add G_TYPE_UUID
Review of attachment 233905 [details] [review]: ::: glib/guuid.c @@ +36,3 @@ + * @title: GUUID + * @short_description: a universal unique identifier + * You should cite the RFC at some point in here. In particular, it's totally nonobvious what the numbers in g_uuid_generate[345] mean otherwise. @@ +42,3 @@ + * The creation of UUIDs does not require a centralized authority. + * + * UUIDs are of relatively small size (128-bits, or 16-bytes). The no hyphens @@ +90,3 @@ + * G_UUID_NIL: + * + * Initialize a #GUUID to the nil value. That makes it sound like the macro initializes the GUUID itself. I'd say something like "a macro that can be used to initialize a #GUUID to the nil value" @@ +103,3 @@ +G_DEFINE_QUARK ("g-uuid-error-quark", g_uuid_error) + +G_UUID_DEFINE_STATIC(uuid_nil, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) need a space before the "(" here and in a lot of other places. (Assuming this macro gets kept.) @@ +174,3 @@ + * @uuid: a #GUUID + * + * Creates a string representation of @uuid. mention that it's just the UUID, no {}s or "urn:uuid:" @@ +199,3 @@ + * g_uuid_from_string: + * @str: a string representing a UUID + * @uuid: the #GUUID to store the parsed UUID value, or #NULL needs annotation: @uuid: (out) (caller-allocates): the #GUUID ... So, the reason for allowing NULL here is to allow using it to check "is this string a UUID", right? But having to pass two NULLs in that case (since you presumably don't care about the exact error either) seems clunky. Maybe an explicit g_uuid_string_is_valid(str) would be better? (And then you don't have to allow NULL here.) @@ +205,3 @@ + * in @uuid, unless it is %NULL. + * + * Curly braces, and URN prefix are optional. I think it would be better to just give examples of the different styles it parses. @@ +294,3 @@ + * @uuid: a #GUUID + * + * Generates a random UUID (rfc4122 version 4). should be "RFC 4122" everywhere @@ +362,3 @@ + gint version, + GChecksumType checksum_type, + const GUUID *namespace, you need an extra column of spaces in there. (ie, there should be a column that doesn't have any letters or any "*"s in it) @@ +363,3 @@ + GChecksumType checksum_type, + const GUUID *namespace, + const gchar *name) Is the name going to be a string for all namespaces? eg, for the OID namespace it would probably be raw ASN.1, right? I went to check what libuuid does, and apparently it doesn't even support type 3 and 5... which makes me wonder if we need to... @@ +378,3 @@ + bytes = g_byte_array_new (); + g_byte_array_append (bytes, namespace->bytes, sizeof (namespace->bytes)); + g_byte_array_append (bytes, (guint8*)name, strlen (name)); you don't need to do this; you can just call g_checksum_update() twice @@ +402,3 @@ + * + * Generates a UUID based on the MD5 hash of a namespace UUID and a + * string (rfc4122 version 3). needs something like: MD5 is no longer considered secure, and you should only use this if you need interoperability with existing systems that use version 3 UUIDs. For new code, you should use g_uuid_generate5(). ::: glib/guuid.h @@ +75,3 @@ + +struct _GUUID { + guint8 bytes[16]; Is there a reason to have a struct rather than just typedef guint8 GUUID[16]; ? @@ +80,3 @@ + +#define G_UUID_DEFINE_STATIC(name,u0,u1,u2,u3,u4,u5,u6,u7,u8,u9,u10,u11,u12,u13,u14,u15) \ + static const GUUID name G_GNUC_UNUSED = \ This seems very specific... what if someone wants to define a non-static UUID? Or what if they do want a compiler warning if it's not used? If you keep GUUID as a struct, then maybe there's an argument for: #define G_UUID(u0,u1,u2,u3,u4,u5,u6,u7,u8,u9,u10,u11,u12,u13,u14,u15) \ { {u0,u1,u2,u3,u4,u5,u6,u7,u8,u9,u10,u11,u12,u13,u14,u15} }; (since the double braces are a little confusing). But if not, I don't think we need a macro at all. @@ +83,3 @@ + { {u0,u1,u2,u3,u4,u5,u6,u7,u8,u9,u10,u11,u12,u13,u14,u15} }; + +#define G_UUID_NIL { { 0, } } You have to fill in all 16 0s so -Wmissing-field-initializers won't warn. (glib doesn't use that warning, but some other modules do, and they might want to use this macro.) ::: glib/tests/guuid.c @@ +28,3 @@ + +G_UUID_DEFINE_STATIC (gnome_uuid3, + 0xee, 0xec, 0x79, 0xff, presumably this was originally generated with someone else's code?
Review of attachment 233905 [details] [review]: ::: glib/guuid.c @@ +36,3 @@ + * @title: GUUID + * @short_description: a universal unique identifier + * added @@ +42,3 @@ + * The creation of UUIDs does not require a centralized authority. + * + * UUIDs are of relatively small size (128-bits, or 16-bytes). The ok @@ +90,3 @@ + * G_UUID_NIL: + * + * Initialize a #GUUID to the nil value. ok @@ +103,3 @@ +G_DEFINE_QUARK ("g-uuid-error-quark", g_uuid_error) + +G_UUID_DEFINE_STATIC(uuid_nil, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) fixed @@ +174,3 @@ + * @uuid: a #GUUID + * + * Creates a string representation of @uuid. ok @@ +199,3 @@ + * g_uuid_from_string: + * @str: a string representing a UUID + * @uuid: the #GUUID to store the parsed UUID value, or #NULL agree @@ +205,3 @@ + * in @uuid, unless it is %NULL. + * + * Curly braces, and URN prefix are optional. added @@ +294,3 @@ + * @uuid: a #GUUID + * + * Generates a random UUID (rfc4122 version 4). yeah @@ +362,3 @@ + gint version, + GChecksumType checksum_type, + const GUUID *namespace, ah ok @@ +363,3 @@ + GChecksumType checksum_type, + const GUUID *namespace, + const gchar *name) It's implementation in ossp-uuid (uuid-devel on fedora) Version 3/5 is actually quite interesting, since it is not random. It allows you to attributes uuid to your objects within your namespace/application. This is used for example in WiX (of which I am writing a clone) http://wix.sourceforge.net/manual-wix3/generate_guids.htm "http://wix.sourceforge.net/manual-wix3/generate_guids.htm". It's basically as useful as a hash, except that it has the UUID format required in some cases.. @@ +378,3 @@ + bytes = g_byte_array_new (); + g_byte_array_append (bytes, namespace->bytes, sizeof (namespace->bytes)); + g_byte_array_append (bytes, (guint8*)name, strlen (name)); ah! thanks @@ +402,3 @@ + * + * Generates a UUID based on the MD5 hash of a namespace UUID and a + * string (rfc4122 version 3). added ::: glib/guuid.h @@ +75,3 @@ + +struct _GUUID { + guint8 bytes[16]; It's easier to extend, it's easier to copy (GUUID a = b), it makes compiler warnings easier to understand (uses struct name GUUID* vs real type ‘char (*)[16]’). @@ +80,3 @@ + +#define G_UUID_DEFINE_STATIC(name,u0,u1,u2,u3,u4,u5,u6,u7,u8,u9,u10,u11,u12,u13,u14,u15) \ + static const GUUID name G_GNUC_UNUSED = \ Why would you want to define in differently? if you already have a UUID in your code (like in the tests and in guuid.c), it should really be const. static will allow the compiler to discard it if it's not used. It's how linux-util/libuuid defines it too. I guess the UNUSED is needed for headers: http://www.opensource.apple.com/source/gpt/gpt-5/gpt.h (second google hiton UUID_DEFINE) @@ +83,3 @@ + { {u0,u1,u2,u3,u4,u5,u6,u7,u8,u9,u10,u11,u12,u13,u14,u15} }; + +#define G_UUID_NIL { { 0, } } ok ::: glib/tests/guuid.c @@ +28,3 @@ + +G_UUID_DEFINE_STATIC (gnome_uuid3, + 0xee, 0xec, 0x79, 0xff, yeah, generated with python, added a comment
Created attachment 233968 [details] [review] Add GUUID to GLib Add a new GUUID type representing a UUID described in RFC 4122. The version 3 to 5 of the RFC 4122 mechansims are provided. Many UUID users will just need a random string, which can be generated simply by calling the function g_uuid_random().
On second thought, it would probably be useful to have G_UUID_DEFINE, which would be non-static and would warn when not used, and G_UUID_DEFINE_STATIC, the same macro, probably to be used exclusively in headers or on conditionnal code.
if so, would it be better to have the well-known UUIDs in the header like this? G_UUID_DEFINE_STATIC (G_UUID_NIL, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) G_UUID_DEFINE_STATIC (G_UUID_NAMESPACE_DNS,...)
hmm, that would not be very friendly for runtime bindings. So let's get rid of the STATIC version, shall we?
Created attachment 233971 [details] [review] Add GUUID to GLib Add a new GUUID type representing a UUID described in RFC 4122. The version 3 to 5 of the RFC 4122 mechansims are provided. Many UUID users will just need a random string, which can be generated simply by calling the function g_uuid_random().
Created attachment 233972 [details] [review] Add GUUID to GLib Add a new GUUID type representing a UUID described in RFC 4122. The version 3 to 5 of the RFC 4122 mechansims are provided. Many UUID users will just need a random string, which can be generated simply by calling the function g_uuid_random().
Is this patch being considered? UUIDs are very useful and the implementation is clean and simple, user a user of UUIDs I'd like to see this feature in GLib
Review of attachment 233972 [details] [review]: OK, yeah, I think we should get this in, assuming no one else has any objections? Anatoly, does the current proposed API make sense to you / would it work in your app? ::: glib/glib.symbols @@ +1693,3 @@ g_rec_mutex_trylock g_rec_mutex_unlock +g_uuid_equal glib.symbols is gone now, so this part can go away ::: glib/guuid.c @@ +34,3 @@ +/** + * SECTION:uuid + * @title: GUUID So I should have noticed this last time, but the type name should probably be "GUuid"; in general in glib, acronyms longer than 2 letters long get titlecased. (eg, GHmac). Likewise GUuidError and GUuidNamespace. @@ +61,3 @@ + * You can look up well-known namespaces with g_uuid_get_namespace(). + * + * Since: 2.36 all the Since strings need to be changed to 2.38 now (including the one in guuid.h) @@ +83,3 @@ + +/** + * G_UUID_DEFINE: it would probably be good to have G_UUID_DEFINE_STATIC too. eg, that's what you really want with the namespace UUIDs defined below @@ +343,3 @@ + + bytes = uuid->bytes; + ints = (guint32*)bytes; is uuid->bytes guaranteed to be properly aligned for that? (I don't know what the rules would be for a struct containing an array of bytes...) @@ +443,3 @@ + * For new code, you should use g_uuid_generate5(). + * + *Since: 2.36 missing a space there ::: glib/guuid.h @@ +46,3 @@ +} GUUIDError; + +GLIB_AVAILABLE_IN_2_36 2_38 now ::: glib/tests/Makefile.am @@ +257,3 @@ +TEST_PROGS += guuid +guuid_SOURCES = guuid.c This Makefile has been simplified since you wrote the patch. You should just need to add one line to test_programs now. (in the right place alphabetically)
Review of attachment 233972 [details] [review]: ::: glib/guuid.c @@ +34,3 @@ +/** + * SECTION:uuid + * @title: GUUID ok @@ +61,3 @@ + * You can look up well-known namespaces with g_uuid_get_namespace(). + * + * Since: 2.36 done @@ +83,3 @@ + +/** + * G_UUID_DEFINE: Actually, only G_UUID_DEFINE_STATIC is really useful. @@ +343,3 @@ + + bytes = uuid->bytes; + ints = (guint32*)bytes; That's what pahole suggests: struct _GUuid { guint8 bytes[16]; /* 0 16 */ /* size: 16, cachelines: 1, members: 1 */ /* last cacheline: 16 bytes */ }; @@ +443,3 @@ + * For new code, you should use g_uuid_generate5(). + * + *Since: 2.36 added
Created attachment 246057 [details] [review] Add GUUID to GLib Add a new GUUID type representing a UUID described in RFC 4122. The version 3 to 5 of the RFC 4122 mechansims are provided. Many UUID users will just need a random string, which can be generated simply by calling the function g_uuid_random().
Created attachment 246058 [details] [review] Add G_TYPE_UUID
Created attachment 246059 [details] [review] Add GUuid to GLib Add a new GUuid type representing a UUID described in RFC 4122. The version 3 to 5 of the RFC 4122 mechansims are provided. Many UUID users will just need a random string, which can be generated simply by calling the function g_uuid_random().
Created attachment 246060 [details] [review] Add G_TYPE_UUID
I wonder if the macro G_UUID_DEFINE_STATIC could be just named G_UUID_DEFINE (and still static const)
(In reply to comment #28) > I wonder if the macro G_UUID_DEFINE_STATIC could be just named G_UUID_DEFINE > (and still static const) one thing that Mozilla does in the header and IDL files (i.e. the ones that are going to be machine-readable for static analysis and code generation) is embed a UUID, so that header changes are reflected in the API itself, outside of the revision control or build environment. if somebody tries to do the same and wants to use a UUID as part of the API then we're going to need two macros, one with a 'static' and one without.
Review of attachment 246059 [details] [review]: ::: glib/guuid.c @@ +148,3 @@ + **/ +gboolean +g_uuid_equal (const GUuid *uuid1, should we use gconstpointer here, so that you can pass this function to a GHashTable? @@ +294,3 @@ + * f81d4fae-7dec-11d0-a765-00a0c91e6bf6), the URN syntax (ex: + * urn:uuid:f81d4fae-7dec-11d0-a765-00a0c91e6bf6) or with curly braces + * (ex: {f81d4fae-7dec-11d0-a765-00a0c91e6bf6}). the phrasing is a bit awkward; I'd use something like: """ The function accepts the following syntaxes: - simple forms (e.g. f81d4fae-7dec-11d0-a765-00a0c91e6bf6) - simple forms with curly braces (e.g. {urn:uuid:f81d4fae-7dec-11d0-a765-00a0c91e6bf6}) - URN (e.g. urn:uuid:f81d4fae-7dec-11d0-a765-00a0c91e6bf6) """ @@ +297,3 @@ + * + * Returns: %TRUE if @str is a valid UUID, %FALSE otherwise. + **/ missing Since: 2.38 @@ +375,3 @@ + * + * Returns: a UUID, or %NULL on failure. + **/ missing Since: 2.38. @@ +410,3 @@ + + digest_len = g_checksum_type_get_length (checksum_type); + g_return_if_fail (digest_len != -1); this should be an assert. @@ +421,3 @@ + g_checksum_get_digest (checksum, digest, (gsize*)&digest_len); + + g_return_if_fail (digest_len >= 16); I think this should be a g_assert(). @@ +462,3 @@ + * @uuid: a #GUuid + * @namespace: a namespace #GUuid + * @name: a string is there any reason why this is gconstpointer and not a const char *, or const guint8 *? introspection will have some issues with gpointer/gconstpointer buffers. ::: glib/guuid.h @@ +43,3 @@ +typedef enum +{ + G_UUID_ERROR_PARSE, no comma at the end of an enumeration: C++ compilers will warn. @@ +69,3 @@ + G_UUID_NAMESPACE_URL, + G_UUID_NAMESPACE_OID, + G_UUID_NAMESPACE_X500, same as above: no comma at the end of an enumeration. @@ +75,3 @@ + +struct _GUuid { + guint8 bytes[16]; coding style: only two spaces for indentation. @@ +83,3 @@ + { { u0,u1,u2,u3,u4,u5,u6,u7,u8,u9,u10,u11,u12,u13,u14,u15 } }; + +#define G_UUID_NIL { { 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 } } I'd call it G_UUID_INIT_NIL, to match the other init macros we have. should we have a G_UUID_INIT(ns,u0,...) macro as well, for stack-allocated UUIDs?
Review of attachment 246059 [details] [review]: thanks for the reviews ::: glib/guuid.c @@ +148,3 @@ + **/ +gboolean +g_uuid_equal (const GUuid *uuid1, ack @@ +294,3 @@ + * f81d4fae-7dec-11d0-a765-00a0c91e6bf6), the URN syntax (ex: + * urn:uuid:f81d4fae-7dec-11d0-a765-00a0c91e6bf6) or with curly braces + * (ex: {f81d4fae-7dec-11d0-a765-00a0c91e6bf6}). copied, thanks @@ +297,3 @@ + * + * Returns: %TRUE if @str is a valid UUID, %FALSE otherwise. + **/ ok @@ +375,3 @@ + * + * Returns: a UUID, or %NULL on failure. + **/ ok @@ +410,3 @@ + + digest_len = g_checksum_type_get_length (checksum_type); + g_return_if_fail (digest_len != -1); ok @@ +421,3 @@ + g_checksum_get_digest (checksum, digest, (gsize*)&digest_len); + + g_return_if_fail (digest_len >= 16); ok @@ +462,3 @@ + * @uuid: a #GUuid + * @namespace: a namespace #GUuid + * @name: a string I don't remember why I used that, probably to accept easily any kind of pointer. g_checksum_update() uses const guchar*, so I switched to that. ::: glib/guuid.h @@ +44,3 @@ +{ + G_UUID_ERROR_PARSE, +} GUuidError; ah, ok removed. @@ +75,3 @@ + +struct _GUuid { + guint8 bytes[16]; fixed @@ +83,3 @@ + { { u0,u1,u2,u3,u4,u5,u6,u7,u8,u9,u10,u11,u12,u13,u14,u15 } }; + +#define G_UUID_NIL { { 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 } } good idea, added
Created attachment 246076 [details] [review] Add GUuid to GLib Add a new GUuid type representing a UUID described in RFC 4122. The version 3 to 5 of the RFC 4122 mechansims are provided. Many UUID users will just need a random string, which can be generated simply by calling the function g_uuid_random().
Created attachment 246077 [details] [review] Add G_TYPE_UUID
What are some usecases that people would have for wanting to pass around GUuid objects instead of just strings?
(In reply to comment #34) > What are some usecases that people would have for wanting to pass around GUuid > objects instead of just strings? by nature, I think uuid are more machine type than human type. The string representation is useful to serialize in text format, xml and such, but shouldn't be the main representation, also I am not sure the casing is specified. And, verifying validity or extracting the few information you can get from uuid is much easier with raw bytes than string. It's also required to use version 3 and 5. An API that would need to convert string to a raw bytes representation wouldn't be very efficient.. But there are still a lot of cases, ex from command line, where UUID are passed as string, g_uuid_random() could be all you need in those cases.
but... how would i use the GUuid object, typically? Would I expect to see it on the API of libraries? Would it be mostly intended as an internal utility class?
(In reply to comment #36) > but... how would i use the GUuid object, typically? Would I expect to see it > on the API of libraries? Would it be mostly intended as an internal utility > class? Serialization will usually involve string representation. But it's a shame that validation is usually lacking, and doing validation is just a step away from internal representation which is nicer for API than a loosy char* or gpointer. An application such as GNote uses it internally, in native types (UUID on win32, uuid_t on unixes), and store in XML. wix/wixl does generate type 5, and they end up in XML too. I imagine an API/library that uses RFC 4122 to use GUuid (qemu, libvirt, spice-gtk, are examples that come to my mind, looking up in devhelp gives many more usages, but I am not sure they all use RFC 4122, although the type could still fit?). I know recently virt-manager got RFC 4122 related fixes, this kind of mistakes and portability come regularly. in the case of spice-gtk and libvirt, it's useful to associate UUID machines coming from libvirt or external metadata sources (settings, ovirt etc), with a Spice server. I don't see why a string representation should be prefered for this. I can't say for other UUID users, such as tracker, gupnp/rygel etc, but I can imagine the string representation in memory isn't the best.
I can grok that storing UUIDs as a GUuid internally may be vaguely nice, but I don't think it's a huge win over a string (which you'll probably use on all of your interfaces). It's sort of like gunichar* strings vs. just having UTF-8 everwhere -- we could use gunichar, and we do in some very special cases, but for the most part people are happy with utf8 because it involves less converting and is "good enough".
Review of attachment 246076 [details] [review]: In general this looks good, like a useful addition to glib. Have not run it yet. Do you have any patches to gnome apps ready? That would help validate the API. When we did GBytes or GHmac we tried to make sure we did some patches in parallel to make sure we weren't missing anything. There are some things like GUuidNamespace enum, which seem a bit strange, and use of GError. Noted below. Obviously the name "GUuid" looks a bit odd at first glance, but not much we can do about that. I think it's correct. ::: glib/guuid.c @@ +203,3 @@ + +static gboolean +uuid_parse_string (const gchar *str, For a parsing/validation API, it's really useful to have a gssize len argument, which accepts -1, as a way of saying null-terminated. @@ +224,3 @@ + goto end; + + if (expected_len == 38) Comment here would be helpful, or perhaps make the code more obvious. @@ +254,3 @@ + if (success == FALSE) + g_set_error (error, G_UUID_ERROR, G_UUID_ERROR_PARSE, + _("Not a valid UUID string")); Again, don't think we need to be using GError here. @@ +346,3 @@ + + bytes = uuid->bytes; + ints = (guint32*)bytes; Nitpick, missing space in cast? @@ +395,3 @@ + default: + g_return_val_if_reached (NULL); + } So why aren't these just global GUuid constants? Why do we have an enum here? @@ +423,3 @@ + + digest = g_malloc (digest_len); + g_checksum_get_digest (checksum, digest, (gsize*)&digest_len); Nitpick, cast spacing? ::: glib/guuid.h @@ +37,3 @@ +/** + * GUuidError: + * @G_UUID_ERROR_PARSE: UUID string was ill-formed This seems irrelevant. This is not a recoverable error, and I don't think it's appropriate to use GError for failures here. Especially since we have only one error code. @@ +86,3 @@ +#define G_UUID_DEFINE_STATIC(name,u0,u1,u2,u3,u4,u5,u6,u7,u8,u9,u10,u11,u12,u13,u14,u15) \ + static const GUuid name = \ + G_UUID_INIT(u0,u1,u2,u3,u4,u5,u6,u7,u8,u9,u10,u11,u12,u13,u14,u15); What does this macro give us over the static const declaration that it contains? Just seems to hide information and make what's going on less readable. ::: glib/tests/guuid.c @@ +1,1 @@ +/* gdatetime-tests.c Copy pasta :)
Review of attachment 246077 [details] [review]: Makes sense.
Review of attachment 246076 [details] [review]: ::: glib/guuid.c @@ +203,3 @@ + +static gboolean +uuid_parse_string (const gchar *str, Right, it's also useful to have a endptr, but that would make API a bit more complicated: g_uuid_from_string ("uuid", -1, NULL, &uuid, NULL); (with error) I have no strong opinion there, I don't see a clear pattern in glib. @@ +224,3 @@ + goto end; + + if (expected_len == 38) perhaps just replace with if (str[0] == '{') @@ +254,3 @@ + if (success == FALSE) + g_set_error (error, G_UUID_ERROR, G_UUID_ERROR_PARSE, + _("Not a valid UUID string")); We could be more explicit about the kind of error later (length, form, some kind of validation for the different versions). Not sure it is so useful though and we could just go with g_warnings I guess. I'll remove it. @@ +346,3 @@ + + bytes = uuid->bytes; + ints = (guint32*)bytes; what is the official coding style? Looking at glib code it could be either: (guint32 *)bytes; or (guint32 *) bytes; or even (guint32*) bytes @@ +395,3 @@ + default: + g_return_val_if_reached (NULL); + } Avoiding extra symbols, extandable, grouping a familly of symbols behind the same function, simplify doc, make it look like a constant (upper case as opposed to symbols in lower case). ::: glib/guuid.h @@ +37,3 @@ +/** + * GUuidError: + * @G_UUID_ERROR_PARSE: UUID string was ill-formed ok @@ +86,3 @@ +#define G_UUID_DEFINE_STATIC(name,u0,u1,u2,u3,u4,u5,u6,u7,u8,u9,u10,u11,u12,u13,u14,u15) \ + static const GUuid name = \ + G_UUID_INIT(u0,u1,u2,u3,u4,u5,u6,u7,u8,u9,u10,u11,u12,u13,u14,u15); convenience, but I'll get rid of it. ::: glib/tests/guuid.c @@ +1,1 @@ +/* gdatetime-tests.c yep
Review of attachment 246076 [details] [review]: ::: glib/guuid.h @@ +86,3 @@ +#define G_UUID_DEFINE_STATIC(name,u0,u1,u2,u3,u4,u5,u6,u7,u8,u9,u10,u11,u12,u13,u14,u15) \ + static const GUuid name = \ + G_UUID_INIT(u0,u1,u2,u3,u4,u5,u6,u7,u8,u9,u10,u11,u12,u13,u14,u15); Now I remember, it's the only macro documented, and is a strong incentive to use static uuids (non-static shouldn't exist, right?).
Still don't like it :/
(In reply to comment #44) > Still don't like it :/ You are not very explicit. My understanding is that you find it more friendly to call a function expecting UUID with a string, like: get_foo("{3F2504E0-4F89-11D3-9A0C-0305E82C3301}") while this is true when interacting with a console, I think it doesn't make sense from a machine pov. How better is it from using get_foo(uuid_from_string("{3F2504E0-4F89-11D3-9A0C-0305E82C3301}", &uuid)) ? Some issues using the string representation: - should be validated each time it is received - non-unique representation (traditionnal rfc, systemd UUIDs, urn, windows format, lowercase/uppercase, smaller base64/85) - uuid are not human friendly by nature, and more machine types - interpretation and generation is done on byte level, depending on use case, we could avoid unnecessary string conversion - doesn't allow easy access to data fields - variable ~37 vs fixed 16 bytes - non-friendly to copy and allocate on stack
Review of attachment 246076 [details] [review]: ::: glib/guuid.c @@ +254,3 @@ + if (success == FALSE) + g_set_error (error, G_UUID_ERROR, G_UUID_ERROR_PARSE, + _("Not a valid UUID string")); It's useful to raise a GError for higher-level languages. This allows to simply write try { uuid.from_string(str) foo(uuid) } rather than try { if (!uuid.from_string(str)) raise my.uuiderror("uuid parsing error") foo(uuid) } Do you agree?
(In reply to comment #44) > Still don't like it :/ I find myself in need of UUID fonctions again (implementing a small soup webdav server) Ryan, I feel strongly that the internal reprentation type must be 16 bytes. I gave several reasons in comment 45. I think your gunichar comparison is unfair. There is a different internal representation between string and byte format (unlike gchar* and gunichar* afaik) Do you still want the public UUID type to be a mere gchar*? I think it is easy to wrap it in a uuid_from_string("") call instead.
(In reply to comment #2) > Some other references: > > http://www.ietf.org/rfc/rfc4122.txt > http://www.boost.org/doc/libs/1_52_0/libs/uuid/uuid.html > http://doc.qt.digia.com/qt/quuid.html > http://msdn.microsoft.com/en-us/library/system.guid.aspx > http://docs.python.org/3/library/uuid.html > http://git.kernel.org/?p=utils/util-linux/util-linux.git;a=blob;f=libuuid/src/uuid.h > http://msdn.microsoft.com/en-us/library/windows/desktop/aa379205%28v=vs.85%29.aspx Adding rust to the list http://doc.rust-lang.org/uuid/
Just looking at this bug again, Ryan, I still don't get why you would prefer a string as an internal representation. The machine type is 16 bytes, it's the only "real" canonical form (don't trust wikipedia ;). An internal string representation would require more than twice the amount of memory and the worst is the checking and conversions for doing any operation, like comparison. Also, I think a specific type is way better than just a char* for API clarity and type checking. If people just need string representations, they probably don't need a UUID type.
Created attachment 305747 [details] [review] Add GUuid to GLib Add a new GUuid type representing a UUID described in RFC 4122. The version 3 to 5 of the RFC 4122 mechansims are provided. Many UUID users will just need a random string, which can be generated simply by calling the function g_uuid_random().
Created attachment 305748 [details] [review] Add G_TYPE_UUID
Rebased to current master. I've also fixed the build with the C++ compiler (it doesn't like "namespace" as a variable name). There are a couple of gtk-doc warnings during the build: ./glib-sections.txt:3356: warning: No declaration found for G_UUID_NIL. ./glib-sections.txt:3357: warning: No declaration found for G_UUID_DEFINE. I want to use this to generate session IDs for use with AirPlay. Look for X-Apple-Session-ID: http://nto.github.io/AirPlay.html
Review of attachment 305747 [details] [review]: Okay. Seems like enough people want this that I'll get out of the way of blocking it. Let's talk API... ::: glib/guuid.h @@ +70,3 @@ + G_UUID_NAMESPACE_OID, + G_UUID_NAMESPACE_X500 +} GUuidNamespace; Why bother with this enum and the getter function when we could just define these ones directly? ie: Purely in terms of using this API (and ignoring ugly implementation tricks), I think I'd prefer to write this: g_uuid_init_v4 (&uuid, G_UUID_NAMESPACE_DNS, ...); rather than g_uuid_init_v4 (&uuid, g_uuid_get_namespace (G_UUID_NAMESPACE_DNS), ...); There are several ways we could achieve that. I think my preferred choice is to either do something like you have here, but to include the call automatically: #define G_UUID_NAMESPACE_DNS (g_uuid_get_namespace(0)) or to do something like: #define G_UUID_NAMESPACE_DNS (g_uuid_get_namespace_dns()) static inline const GUuid * g_uuid_get_namespace_dns(void) { return ... } @@ +74,3 @@ +typedef struct _GUuid GUuid; + +struct _GUuid { I'm glad you used a struct here and not an array directly. Better to avoid all the implicit weirdness associated with passing array types as arguments to a function... @@ +79,3 @@ + + +#define G_UUID_INIT(u0,u1,u2,u3,u4,u5,u6,u7,u8,u9,u10,u11,u12,u13,u14,u15) \ What if we stepped up the crack level to allow writing something like: G_UUID_INIT(de305d54, 75b4, 431b, adb2, eb6b9e546014) ? This would not be difficult with a bit of math and preprocessor token pasting (ie: add the "0x" to the front automatically). For the long part at the end we can make use of the fact that all glib platforms support 64bit ints (and do some preprocessor math to extract the separate bytes from that long literal). If I encounter that in code it's going to be a whole lot more readable than the other way, with the bytes split out separately. @@ +91,3 @@ +gboolean g_uuid_is_nil (const GUuid *uuid); +GLIB_AVAILABLE_IN_2_46 +gboolean g_uuid_equal (gconstpointer uuid1, Why are these taking gconstpointer? Is it the eventual idea that we may use this as keys in a hashtable, so we want to match the function prototype with the callback type? Do we want to give a _hash() function? If we're going to be using these as keys in a table then do we also want to have functions to heap allocate and free uuid structs, or do we only plan on using them via simple containment in other structs? @@ +104,3 @@ + +GLIB_AVAILABLE_IN_2_46 +gchar * g_uuid_random (void); this has a bad name. maybe g_uuid_string_generate() to go with the pseudo-method g_uuid_string_is_valid() above. @@ +106,3 @@ +gchar * g_uuid_random (void); +GLIB_AVAILABLE_IN_2_46 +void g_uuid_generate4 (GUuid *uuid); Not a fan of this generate3/4/5 business. Reminds me of dup3() and accept4(). Since this is stack-allocated, I guess we would go with the _init() paradigm here. Could even do _init_v3, _init_v4, etc. We could also add a BS _clear() method to set it back to zero (noticing that you have a _is_nil() above, maybe that's useful for some things...)
Review of attachment 305747 [details] [review]: thanks for the review desrt! ::: glib/guuid.h @@ +70,3 @@ + G_UUID_NAMESPACE_OID, + G_UUID_NAMESPACE_X500 +} GUuidNamespace; Sure that looks nicer, either way is fine for me. @@ +79,3 @@ + + +#define G_UUID_INIT(u0,u1,u2,u3,u4,u5,u6,u7,u8,u9,u10,u11,u12,u13,u14,u15) \ Sounds doable indeed. @@ +91,3 @@ +gboolean g_uuid_is_nil (const GUuid *uuid); +GLIB_AVAILABLE_IN_2_46 +gboolean g_uuid_equal (gconstpointer uuid1, Yes, I think I wanted it of GEqualFunc type. Regarding heap/stack, it's fairly common to have it embeded in other structs, and easy to heap copy/allocate it anyway. Sure a hash() function would be nice, but I haven't looked at it. @@ +104,3 @@ + +GLIB_AVAILABLE_IN_2_46 +gchar * g_uuid_random (void); I wouldn't use "generate", but "random" instead, so it's quite obvious it's going to use v4. I am fine with having "string" or not in the name, but I have a slight preference for the shorter version. @@ +106,3 @@ +gchar * g_uuid_random (void); +GLIB_AVAILABLE_IN_2_46 +void g_uuid_generate4 (GUuid *uuid); Or we could use the version method name: g_uuid_generate_md5(), g_uuid_generate_sha1() etc.. I would avoid "init" in the name which doesn't reflect the randomness/uniqueness of the returned value. It should be enough to clear an UUID to do: uuid = G_UUID_INIT_NIL;
Created attachment 312163 [details] [review] Add GUuid to GLib Add a new GUuid type representing a UUID described in RFC 4122. The version 3 to 5 of the RFC 4122 mechansims are provided. Many UUID users will just need a random string, which can be generated simply by calling the function g_uuid_string_random().
Created attachment 312164 [details] [review] Add G_TYPE_UUID
Created attachment 312274 [details] [review] Add GUuid to GLib Add a new GUuid type representing a UUID described in RFC 4122. The version 3 to 5 of the RFC 4122 mechansims are provided. Many UUID users will just need a random string, which can be generated simply by calling the function g_uuid_string_random().
Created attachment 312275 [details] [review] Add G_TYPE_UUID
Summary of changes: - rewrite macro G_UUID_INIT(timelow, timemid, timehigh, clock, node) - remove g_uuid_get_namespace() and enum in favor of inline functions to return static uuid - rename g_uuid_random() -> g_uuid_string_random() - rename generateN() generate_vN() - remove the GError, not so useful (had only parse error) - fix some gtk-doc warnings - update headers - update version annotations
Those patches still apply, and work for me.
I'm happy to add g_uuid_string_random() that generates a string-formatted uuid using the current 'best practices'. I still don't understand the need for any of the rest of this. A brief chat on IRC with some of the users of this confirms that this is all they really want to use anyway.
Created attachment 343378 [details] [review] guuid: Add UUID helper functions to GLib Many UUID users will just need a random string, which can be generated simply by calling the function g_uuid_string_random(). Based on original patch by Marc-André Lureau <marcandre.lureau@gmail.com>
I trimmed Marc-André's patches so that only the "random" and "is_valid" functions were left public. I tried to do minimal changes to the code itself, even keeping documentation but remove the double-star so that gtk-doc didn't pick it up. This means it should be fairly straight forward to re-add the functionality in the original patches should the need arise, with minimal churn.
there is a need for all, the question is do we want a "full uuid support" (we are talking a few hundred lines) in glib or not.
(In reply to Marc-Andre Lureau from comment #64) > there is a need for all, the question is do we want a "full uuid support" > (we are talking a few hundred lines) in glib or not. Which is what was discussed in comment 61. I have no need for the whole support, neither did anyone who was interested in the functionality when we discussed it then, and with no comment from anyone else, I trimmed it so that the patch is hopefully acceptable without further discussion. Re-adding the functionality is certainly do-able.
(In reply to Marc-Andre Lureau from comment #64) > there is a need for all, the question is do we want a "full uuid support" > (we are talking a few hundred lines) in glib or not. Can you explain the use cases for all the other functionality then?
Review of attachment 343378 [details] [review]: I'd be totally good with merging this in terms of public API, although imho the implementation is substantially more complicated than it needs to be to implement that API. ::: glib/guuid.c @@ +149,3 @@ + * - simple forms (e.g. f81d4fae-7dec-11d0-a765-00a0c91e6bf6) + * - simple forms with curly braces (e.g. + * {urn:uuid:f81d4fae-7dec-11d0-a765-00a0c91e6bf6}) I think this is probably an unexpected misfeature. Wouldn't it be more useful to only support simple forms here?
Review of attachment 343378 [details] [review]: ::: glib/guuid.c @@ +149,3 @@ + * - simple forms (e.g. f81d4fae-7dec-11d0-a765-00a0c91e6bf6) + * - simple forms with curly braces (e.g. + * {urn:uuid:f81d4fae-7dec-11d0-a765-00a0c91e6bf6}) Would that be alright to remove in a separate commit? So that the aforementioned possible re-adding of the more complete API is made easier?
I was kinda worried after reading the last patch that this was a "foot in the door" attempt to land a reasonable API, from which more reasonable requests could be made to "moderately expand" it. And then I read this: > Would that be alright to remove in a separate commit? So that the > aforementioned possible re-adding of the more complete API is made easier? ... What's your plan here?
(In reply to Allison Lortie (desrt) from comment #69) > I was kinda worried after reading the last patch that this was a "foot in > the door" attempt to land a reasonable API, from which more reasonable > requests could be made to "moderately expand" it. > > And then I read this: > > > Would that be alright to remove in a separate commit? So that the > > aforementioned possible re-adding of the more complete API is made easier? > > ... > > What's your plan here? Re-adding the functionality from the original patch piece-meal once we have justification about the various API calls. Right now, I only removed functions, and at worse made them static. Adding back portions of the original is easy. If I start modifying the functions, it's just going to make this harder. It's code management, not "let's add everything back as soon as things have landed".
I will stop second-guessing you... If you want to provide an additional patch to back-out the {urn: feature, then we can do it that way. Will be happy to review those in a bit.
Created attachment 344778 [details] [review] guuid: Add UUID helper functions to GLib Many UUID users will just need a random string, which can be generated simply by calling the function g_uuid_string_random(). Based on original patch by Marc-André Lureau <marcandre.lureau@gmail.com>
Created attachment 344779 [details] [review] guuid: Remove support for curly braces and URN UUIDs As we currently cannot generate UUIDs with curly braces, or as URNs, remove those from the possible valid UUIDs. We do this separately to make it easier to re-add later, should we want to enhance the coverage of our UUID functions.
Review of attachment 344778 [details] [review]: ::: glib/guuid.c @@ +36,3 @@ +struct _GUuid { + guint8 bytes[16]; +}; Nitpick: Could combine the typedef and struct definition for compactness. @@ +41,3 @@ + * SECTION:uuid + * @title: GUuid + * @short_description: a universal unique identifier Nitpick: s/univeral/universally/ @@ +46,3 @@ + * identify information in a distributed environment. For the + * definition of UUID, see <ulink + * url="tools.ietf.org/html/rfc4122.html">RFC 4122</ulink>. Nitpick: Use gtk-doc’s Markdown syntax for links now: https://wiki.gnome.org/Projects/GTK%2B/DocumentationSyntax/Markdown [RFC 4122](https://tools.ietf.org/html/rfc4122.html) @@ +59,3 @@ + * + * Since: 2.52 + **/ Nitpick: gtk-doc comments should end with ‘*/’, not ‘**/’ (same in several other places below). @@ +69,3 @@ + * prefix). + * + * Returns: A string that should be freed with g_free(). Nitpick: If you’re going to stick a doc comment on this function, you might as well use annotations like (transfer full) in it. @@ +150,3 @@ + * - simple forms with curly braces (e.g. + * {urn:uuid:f81d4fae-7dec-11d0-a765-00a0c91e6bf6}) + * - URN (e.g. urn:uuid:f81d4fae-7dec-11d0-a765-00a0c91e6bf6) Nitpick: Use backticks (`like this`) to denote the examples in monospaced font. (See https://wiki.gnome.org/Projects/GTK%2B/DocumentationSyntax/Markdown.) @@ +164,3 @@ + +static void +uuid_set_version (GUuid *uuid, int version) unsigned int? @@ +200,3 @@ + + bytes = uuid->bytes; + ints = (guint32*)bytes; Nitpick: Need some spaces in the cast: `ints = (guint32 *) bytes;` @@ +202,3 @@ + ints = (guint32*)bytes; + for (i = 0; i < 4; i++) + ints[i] = g_random_int (); At the risk of increasing the API slightly, I suspect we should be exposing a GRand instance for this somewhere, otherwise this cannot be made deterministic (for, e.g., unit testing). @@ +212,3 @@ + * Generates a random UUID (RFC 4122 version 4) as a string. + * + * Returns: A string that should be freed with g_free(). Needs (transfer full). Some indication of the format (or a link to other documentation which explains the format) would be useful. There should probably also be a mention of the fact it uses GLib’s internal PRNG (g_random_int()), so isn’t securely random. ::: glib/tests/guuid.c @@ +1,3 @@ +/* guuid.c + * + * Copyright (C) 2013-2015 Red Hat, Inc. + 2017? @@ +26,3 @@ + +static void +test_GUuid_string (void) Bit weird to have capitals in this function name. @@ +29,3 @@ +{ + g_assert_false (g_uuid_string_is_valid ("00010203-0405-0607-0809")); + g_assert_false (g_uuid_string_is_valid ("zzzzzzzz-zzzz-zzzz-zzzz-zzzzzzzzzzzz")); What about an otherwise-valid UUID with the hyphens removed? @@ +33,3 @@ + g_assert_true (g_uuid_string_is_valid ("00010203-0405-0607-0809-0a0b0c0d0e0f")); + g_assert_true (g_uuid_string_is_valid ("{urn:uuid:f81d4fae-7dec-11d0-a765-00a0c91e6bf6}")); + g_assert_true (g_uuid_string_is_valid ("urn:uuid:f81d4fae-7dec-11d0-a765-00a0c91e6bf6")); Might want to add the other examples from the RFC to the test suite (e.g. from appendix B). @@ +41,3 @@ + gchar *str; + + str = g_uuid_string_random (); Could also test that two consecutive calls to g_uuid_string_random() give different UUIDs.
Review of attachment 344779 [details] [review]: ::: glib/tests/guuid.c @@ -33,2 @@ g_assert_true (g_uuid_string_is_valid ("00010203-0405-0607-0809-0a0b0c0d0e0f")); - g_assert_true (g_uuid_string_is_valid ("{urn:uuid:f81d4fae-7dec-11d0-a765-00a0c91e6bf6}")); Might want to leave `f81d4fae-7dec-11d0-a765-00a0c91e6bf6` as a valid example. Probably best to add it as a valid example in the previous commit, then leave it untouched in this one.
Review of attachment 344778 [details] [review]: ::: glib/guuid.c @@ +150,3 @@ + * - simple forms with curly braces (e.g. + * {urn:uuid:f81d4fae-7dec-11d0-a765-00a0c91e6bf6}) + * - URN (e.g. urn:uuid:f81d4fae-7dec-11d0-a765-00a0c91e6bf6) Maybe also explicitly mention that hyphens are required in the UUIDs; otherwise people new to UUIDs might assume they’re just for readability.
Created attachment 344856 [details] [review] guuid: Add UUID helper functions to GLib Many UUID users will just need a random string, which can be generated simply by calling the function g_uuid_string_random(). Based on original patch by Marc-André Lureau <marcandre.lureau@gmail.com>
Created attachment 344857 [details] [review] guuid: Remove support for curly braces and URN UUIDs As we currently cannot generate UUIDs with curly braces, or as URNs, remove those from the possible valid UUIDs. We do this separately to make it easier to re-add later, should we want to enhance the coverage of our UUID functions.
Created attachment 344858 [details] [review] guuid: Add UUID helper functions to GLib Many UUID users will just need a random string, which can be generated simply by calling the function g_uuid_string_random(). Based on original patch by Marc-André Lureau <marcandre.lureau@gmail.com>
Fixed all the review comments, apart from making GRand available in the API, as I don't see the need for it.
Review of attachment 344857 [details] [review]: I can’t see any problems with this.
Review of attachment 344858 [details] [review]: ::: glib/guuid.c @@ +66,3 @@ + * prefix). + * + * Returns: A string that should be freed with g_free(). (transfer full)? @@ +207,3 @@ + ints = (guint32 *) bytes; + for (i = 0; i < 4; i++) + ints[i] = g_random_int (); I’m pretty sure we should expose a version that parameterises the GRand here, but this is also probably something where a second opinion would be useful. I’m thinking of the use case where some user-written code generates a big string which has an embedded UUID, and they want to unit test the contents of the string. If the UUID is uncontrollably random, they will need to faff around with a regex or similar; if it’s not, they could use a simple strcmp. ::: glib/tests/guuid.c @@ +41,3 @@ + +static void +test_GUuid_random (void) Drop the capitals from the function name. @@ +46,3 @@ + + str1 = g_uuid_string_random (); + g_assert (strlen (str1) == 36); g_assert_cmpuint() @@ +47,3 @@ + str1 = g_uuid_string_random (); + g_assert (strlen (str1) == 36); + g_assert (g_uuid_string_is_valid (str1) == TRUE); g_assert_true() @@ +66,3 @@ + /* GUuid Tests */ + g_test_add_func ("/GUuid/string", test_guuid_string); + g_test_add_func ("/GUuid/random", test_GUuid_random); I think it would be more conventional to call the test suite ‘uuid’ (seems to be more in-line with other GLib test suite names). So: `/uuid/string` and `/uuid/random`.
Created attachment 344925 [details] [review] guuid: Add UUID helper functions to GLib Many UUID users will just need a random string, which can be generated simply by calling the function g_uuid_string_random(). Based on original patch by Marc-André Lureau <marcandre.lureau@gmail.com>
Review of attachment 344925 [details] [review]: This looks good to me, but I’d still like a second opinion about the GRand exposure in the API, so I’ll mark as reviewed rather than a_c-n.
Allison says: Philip is being unreasonable. Commit it now.
Attachment 344857 [details] pushed as 4b75333 - guuid: Remove support for curly braces and URN UUIDs Attachment 344925 [details] pushed as 215c9b7 - guuid: Add UUID helper functions to GLib
(In reply to Marc-Andre Lureau from comment #64) > there is a need for all, the question is do we want a "full uuid support" > (we are talking a few hundred lines) in glib or not. Please file a new bug and mention this one with the use cases for the additional features that were in your original patch. I'm fine with doing the grunt work to get it in a mergeable state, but I can't make up the use cases I'm afraid :/ Thanks for your work!
It broke phodav because it has a different version of the same class. Please work with Marc-Andre to fix it ASAP. I'm unfortunately going to have to limit glib in GNOME to version 2.51.1 until we have a new phodav tarball.
Er, to version 2.50, because we can't use limits that fine-grained. Hm, this might not work; I wonder how much depends on glib 2.51 already. It would really make my day if we get a new phodav tarball tomorrow morning, so I don't have to rebuild everything with glib 2.50 to find out. :)
(In reply to Michael Catanzaro from comment #88) > It broke phodav because it has a different version of the same class. Please > work with Marc-Andre to fix it ASAP. I'm unfortunately going to have to > limit glib in GNOME to version 2.51.1 until we have a new phodav tarball. phodav is broken, sorry. Really shouldn't use a namespace that it doesn't own. It would have broken whichever way we merged this.