GNOME Bugzilla – Bug 740865
[review] create RFC4122 compliant UUIDs [th/uuid-variant3-bgo740865]
Last modified: 2014-12-04 16:08:03 UTC
The UUIDs we created in nm_utils_uuid_generate_from_string() were not according to RFC. Extend the function to create valid variant3 UUIDs in addition to our legacy format. Also, update the call sites to use the new format. The last commit is controversial, because it changes the behavior since the last stable release At least the first parts should be included to 1.0, because they change API in libnm-core.
branch th/uuid-variant3-bgo740865
I definitely don't think we should do the last commit. While it is inelegant that we generate non-RFC-compliant UUIDs, this has never been an issue in practice, because there's really no reason anyone would ever want to unparse a connection's UUID. So being compatible is better than being correct in this case. The only intended use for this API is generating NMSettingConnection:uuid values, and there's no reason to make the user do this by hand at this point, since we can just have nm_connection_normalize() do it for them. So how about we move the existing code into NetworkManagerUtils, for use by ifcfg-rh, ifnet, and ifupdown, for compatibility, and add new RFC-compliant UUID generation code as part of nm_connection_normalize(), but not exposed as API.
(In reply to comment #2) > I definitely don't think we should do the last commit. While it is inelegant > that we generate non-RFC-compliant UUIDs, this has never been an issue in > practice, because there's really no reason anyone would ever want to unparse a > connection's UUID. So being compatible is better than being correct in this > case. ACK. For now I left the patch as part of the branch (for others to see). I'll drop it before merging. > The only intended use for this API is generating NMSettingConnection:uuid > values, and there's no reason to make the user do this by hand at this point, > since we can just have nm_connection_normalize() do it for them. So how about > we move the existing code into NetworkManagerUtils, for use by ifcfg-rh, ifnet, > and ifupdown, for compatibility, and add new RFC-compliant UUID generation code > as part of nm_connection_normalize(), but not exposed as API. nm_connection_normalize() does currently not normalize missing UUIDs. I had that once part of a branch, but dropped it again. Anyway, nm_connection_normalize() could only set random UUIDs, not name-based ones because there is no name at hand. I would not remove this functionality from libnm. It is small enough after all. It can be useful to generate UUIDs. Note that the client application might not have another library at hand to generate UUIDs. So, if the client wants to create UUIDs, it could no longer leverage libuuid via libnm. Especially since the client API is expected to provide a UUID in AddConnection(). See bug 740813. I made a few changes to the existing branch and added 3 new commits: libnm: coerce NMSettingConnection:uuid to lower case libnm: remove support for old UUID format libnm: tighten nm_utils_is_uuid() to check exact UUID format the last patch is again a controversial change. It could potentially break old clients that create upper case UUIDs. But well... why would they do such a thing? And it would only break them during AddConnection() when they pass on an upper case UUID *AND* cannot cope with the coerced result.
(In reply to comment #3) > nm_connection_normalize() does currently not normalize missing UUIDs. I had > that once part of a branch, but dropped it again. Anyway, > nm_connection_normalize() could only set random UUIDs, not name-based ones > because there is no name at hand. Well, there's the connection ID. But anyway, it doesn't need to do name-based ones. The name-based UUID generation is only needed to deal with the case of adding a UUID to a hand-written ifcfg file. > It can be useful to generate UUIDs. Note that the client application might not > have another library at hand to generate UUIDs. Since libnm requires libuuid, the client application must also have access to libuuid. And when is it useful (in a NetworkManager context) to generate UUIDs other than filling in connection.uuid, where (other than in the ifcfg-rh case) you don't actually care what the UUID is?
I am not sure what your concrete suggestion is. Do you want to hide nm_utils_uuid_generate_from_strings() from libnm API? Note that nm_utils_is_uuid() and nm_utils_uuid_generate() is needed in libnm-core. So it cannot move to src/, it can only be hidden from public API. Added another commit th/uuid-variant3-bgo740865 > libnm: normalize missing connection UUID This last commit makes part of bug 740813 obsolete.
(In reply to comment #5) > I am not sure what your concrete suggestion is. Do you want to hide > nm_utils_uuid_generate_from_strings() from libnm API? Yes. I don't see any reason a client should need that function. > Note that nm_utils_is_uuid() and nm_utils_uuid_generate() is needed in > libnm-core. So it cannot move to src/, it can only be hidden from public API. nm_utils_is_uuid() is used outside of libnm-core/libnm/daemon anyway. Eg, nmtui uses it to decide whether a caller-provided string is a UUID or an id. nm_utils_uuid_generate() is useful if callers are required to set connection.uuid themselves when creating a new connection, but if normalize()/AddConnection/whatever is going to do it for them, then I'm not sure there's any need for a public method for it too.
I agree we shouldn't change ifcfg-rh behavior at all, since people are likely depending on the UUIDs we've created there. I think it's fine to create RFC-based UUIDs for new connections though. So yeah, if we normalize/complete a connection with a missing UUID, I don't think there's a great reason to expose nm_utils_uuid_generate() since clients would never need it. Also, if we normalize in the daemon we can re-try a conflicting UUID and not return an error to the client, which would be nice. Note that clients ported to libnm (including nmcli/nmtui) will need to be updated to ensure their verify-type functions for the UI pages don't require a UUID anymore (but validate it if present).
(In reply to comment #7) > I agree we shouldn't change ifcfg-rh behavior at all, since people are likely > depending on the UUIDs we've created there. I think it's fine to create > RFC-based UUIDs for new connections though. Ok, I drop that. > So yeah, if we normalize/complete a connection with a missing UUID, I don't > think there's a great reason to expose nm_utils_uuid_generate() since clients > would never need it. Also, if we normalize in the daemon we can re-try a > conflicting UUID and not return an error to the client, which would be nice. > > Note that clients ported to libnm (including nmcli/nmtui) will need to be > updated to ensure their verify-type functions for the UI pages don't require a > UUID anymore (but validate it if present). I think retry is not necessary, because conflict of nm_u_uuid_generate() is externally unlikely. Not sure, what is the conclusion now? What should I change? Even hide nm_utils_uuid_generate()? Seems a bit exaggerated. What do we gain?
s/externally/extremly/ -- my spell-checker... :)
(In reply to comment #8) > > So yeah, if we normalize/complete a connection with a missing UUID, I don't > > think there's a great reason to expose nm_utils_uuid_generate() since clients > > would never need it. Also, if we normalize in the daemon we can re-try a > > conflicting UUID and not return an error to the client, which would be nice. > > > > Note that clients ported to libnm (including nmcli/nmtui) will need to be > > updated to ensure their verify-type functions for the UI pages don't require a > > UUID anymore (but validate it if present). > > I think retry is not necessary, because conflict of nm_u_uuid_generate() is > externally unlikely. Ok. > Not sure, what is the conclusion now? What should I change? > Even hide nm_utils_uuid_generate()? > Seems a bit exaggerated. What do we gain? My suggestion was to drop nm_utils_uuid_generate() completely from libnm public API, because it would no longer be really required. But I looked through nm-applet and GNOME control center sources, and it would require some restructuring to do correctly. So I think we should keep nm_utils_uuid_generate(), but make that only generate RFC-compliant UUIDs. We can hide nm_utils_uuid_generate_from_string() though, since nothing outside of NM itself actually uses that. > libnm: tighten nm_utils_is_uuid() to check exact UUID format We can't do this for libnm-util since existing connections might have UUIDs of the old format. That also means we can't do it for libnm, because all our tools and the NM core use this function too. > libnm: remove support for 40-character UUID format > libnm: coerce NMSettingConnection:uuid to lower case Likewise we cannot do this either, which is explained in email messages you reference in the revert commit message. NetworkManager didn't used to verify the UUID much, and thus accepted a lot more UUID formats previously, and we can't break those existing UUIDs. We also can't coerce them to lowercase either since that changes them. What we should be doing is generating RFC compliant UUIDs for new connections, but allowing existing UUIDs that minimally verify. The testcase that this patch reverts should stay, and libnm-util/libnm-core still need to pass that testcase. It's not like the UUID actually means anything. It just matters that it's unique enough to separate connections, and it's really just an opaque string. Having it be RFC compliant is nice, but not necessarily required.
Rebased and repushed. With new commit >> libnm: hide nm_utils_uuid_generate_from_string() from public API
> libnm: add a type argument to nm_utils_uuid_generate_from_string() Why is type an 'int'? I don't think we'll ever have < 0 types, especially since it's internal now. And since it's internal, I guess we don't need an enum for it really, though I guess that would be nice. (and yeah, enum is signed, I know... but at least with enum the compiler has a chance of warning you about stuff) > libnm: implement variant3 UUIDs according to rfc4122 Indentation in the switch(); it should be: switch (foo) { case 1: break; case 2: break; } > libnm: normalize missing connection UUID Don't you want to use the new RFC-compliant method here? nm_utils_generate_uuid() won't do that.
(In reply to comment #12) > > libnm: add a type argument to nm_utils_uuid_generate_from_string() > > Why is type an 'int'? I don't think we'll ever have < 0 types, especially > since it's internal now. And since it's internal, I guess we don't need an > enum for it really, though I guess that would be nice. (and yeah, enum is > signed, I know... but at least with enum the compiler has a chance of warning > you about stuff) It should be an enum. But first it was public API and I think it's not a good idea to have enum types in structures or function arguments (because user space might compile the type to a different size with -fshort-enums). Now being it internal only, I could change it. But we might publish it later again... so... > > libnm: implement variant3 UUIDs according to rfc4122 > > Indentation in the switch(); it should be: > > switch (foo) { > case 1: > break; > case 2: > break; > } fixed > > libnm: normalize missing connection UUID > > Don't you want to use the new RFC-compliant method here? > nm_utils_generate_uuid() won't do that. nm_utils_generate_uuid() is RFC compliant since when it was introduced first time (as it uses libuuid). It creates random based UUIDs (variant4). nm_utils_generate_uuid_from_strings() creates variant3 UUIDs based on MD5 hash.
merged as: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=da8b201095e90fc3da9ff3a1638cc483447fed7f