| Message ID | 1452604467-65746-3-git-send-email-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2016-01-12 at 14:14 +0100, Roger Pau Monne wrote: > libxl makes the assumtion that libxl_uuid == uuid_t, ("assumption") > and that uuid_t can be > freely used as a byte array. This is not true on FreeBSD (and NetBSD > too, not sure about other BSD UUID implementations), where the internals > of > uuid don't match what libxl expects as a byte array because of endianness > issues. > > Fix this by converting the libxl_uuid type to a struct with an internal > uuid_t field and a byte-array. Also introduce a new function that should > be > used in order to load a byte array into a uuid_t struct. Do we really need to keep both the uuid_t and the byte-array representation around? It looks to me as if we only really need the byte-array form, which might then involve changing various uses of uuid_* internally to just be mem*.
El 15/01/16 a les 11.26, Ian Campbell ha escrit: > On Tue, 2016-01-12 at 14:14 +0100, Roger Pau Monne wrote: >> libxl makes the assumtion that libxl_uuid == uuid_t, > > ("assumption") > >> and that uuid_t can be >> freely used as a byte array. This is not true on FreeBSD (and NetBSD >> too, not sure about other BSD UUID implementations), where the internals >> of >> uuid don't match what libxl expects as a byte array because of endianness >> issues. >> >> Fix this by converting the libxl_uuid type to a struct with an internal >> uuid_t field and a byte-array. Also introduce a new function that should >> be >> used in order to load a byte array into a uuid_t struct. > > Do we really need to keep both the uuid_t and the byte-array representation > around? It looks to me as if we only really need the byte-array form, which > might then involve changing various uses of uuid_* internally to just be > mem*. Yes, we can remove the uuid_t from libxl_uuid, but this is AFAICT a structure that belongs to the stable API. My current change keeps the same layout by turning the union into a struct, but without changing the fields. Roger.
On Fri, 2016-01-15 at 16:11 +0100, Roger Pau Monné wrote: > El 15/01/16 a les 11.26, Ian Campbell ha escrit: > > On Tue, 2016-01-12 at 14:14 +0100, Roger Pau Monne wrote: > > > libxl makes the assumtion that libxl_uuid == uuid_t, > > > > ("assumption") > > > > > and that uuid_t can be > > > freely used as a byte array. This is not true on FreeBSD (and NetBSD > > > too, not sure about other BSD UUID implementations), where the > > > internals > > > of > > > uuid don't match what libxl expects as a byte array because of > > > endianness > > > issues. > > > > > > Fix this by converting the libxl_uuid type to a struct with an > > > internal > > > uuid_t field and a byte-array. Also introduce a new function that > > > should > > > be > > > used in order to load a byte array into a uuid_t struct. > > > > Do we really need to keep both the uuid_t and the byte-array > > representation > > around? It looks to me as if we only really need the byte-array form, > > which > > might then involve changing various uses of uuid_* internally to just > > be > > mem*. > > Yes, we can remove the uuid_t from libxl_uuid, but this is AFAICT a > structure that belongs to the stable API. Oh b*m, so it is. > My current change keeps the > same layout by turning the union into a struct, but without changing the > fields. The danger with your change is that the two halves can now get out of sync. Neither xl nor libvirt actually poke into the contents of the struct at all. So I wonder if we can get away with deprecating it? Or can we get rid of uuid_raw (which is not touched, and is less likely to be given that it is only on a subset of platforms) and have libxl _internal_ stuff convert to a byte array. The problem there I guess is that would involve changing the semantics of libxl_uuid_bytearray{,_const} (since the result would now need to be freed, since both returns a static buffer, so who knows what the const distinction was supposed to be for!). Hrm, we've certainly painted ourselves into a corner with this one :-/ If we do go with keeping both the uuid_t and the raw array then we should clearly mark the uuid_t one as the canonical copy (the raw one essentially becomes a scratch space used to facilitate the provision of the broken libxl_uuid_bytearray* interfaces). Ian.
Ian Campbell writes ("Re: [Xen-devel] [PATCH 2/2] libxl: fix UUID usage on FreeBSD"):
> On Fri, 2016-01-15 at 16:11 +0100, Roger Pau Monné wrote:
> > Yes, we can remove the uuid_t from libxl_uuid, but this is AFAICT a
> > structure that belongs to the stable API.
>
> Oh b*m, so it is.
I think we may have to bite this bullet. Perhaps we could make an
#ifdef that softened the blow on systems with sane uuid_t.
Given that we have discovered that uuid_t is sometimes insane, I think
we really really want to avoid it.
> Or can we get rid of uuid_raw (which is not touched, and is less likely to
> be given that it is only on a subset of platforms) and have libxl
> _internal_ stuff convert to a byte array.
We don't want to do any byteswapping. That way lies utter madness.
At the libxl API/ABI uuids should be represented as sequences of 16
octets in network byte order (BE).
Ian.
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index 9207621..ae08b2f 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -616,7 +616,7 @@ static void xcinfo2xlinfo(libxl_ctx *ctx, { size_t size; - memcpy(&(xlinfo->uuid), xcinfo->handle, sizeof(xen_domain_handle_t)); + libxl_uuid_from_bytearray(&xlinfo->uuid, xcinfo->handle); xlinfo->domid = xcinfo->domain; xlinfo->ssidref = xcinfo->ssidref; if (libxl_flask_sid_to_context(ctx, xlinfo->ssidref, diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h index 05606a7..876fca8 100644 --- a/tools/libxl/libxl.h +++ b/tools/libxl/libxl.h @@ -867,6 +867,15 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, libxl_mac *src); */ #define LIBXL_HAVE_DEVICE_MODEL_VERSION_NONE 1 +/* + * LIBXL_HAVE_UUID_FROM_BYTEARRAY + * + * In the case that LIBXL_HAVE_UUID_FROM_BYTEARRAY is set libxl + * provides a function (libxl_uuid_from_bytearray) to convert an + * octet stream into a UUID. + */ +#define LIBXL_HAVE_UUID_FROM_BYTEARRAY 1 + typedef char **libxl_string_list; void libxl_string_list_dispose(libxl_string_list *sl); int libxl_string_list_length(const libxl_string_list *sl); diff --git a/tools/libxl/libxl_uuid.c b/tools/libxl/libxl_uuid.c index 7d4a032..f566f50 100644 --- a/tools/libxl/libxl_uuid.c +++ b/tools/libxl/libxl_uuid.c @@ -33,6 +33,12 @@ int libxl_uuid_from_string(libxl_uuid *uuid, const char *in) return uuid_parse(in, uuid->uuid); } +int libxl_uuid_from_bytearray(libxl_uuid *uuid, const uint8_t *raw) +{ + memcpy(uuid, raw, sizeof(*uuid)); + return 0; +} + void libxl_uuid_copy(libxl_ctx *ctx_opt, libxl_uuid *dst, const libxl_uuid *src) { @@ -72,9 +78,9 @@ void libxl_uuid_generate(libxl_uuid *uuid) { uint32_t status; - BUILD_BUG_ON(sizeof(libxl_uuid) != sizeof(uuid_t)); uuid_create(&uuid->uuid, &status); assert(status == uuid_s_ok); + uuid_enc_be(uuid->uuid_raw, &uuid->uuid); } #ifdef __FreeBSD__ @@ -85,6 +91,8 @@ int libxl_uuid_from_string(libxl_uuid *uuid, const char *in) uuid_from_string(in, &uuid->uuid, &status); if (status != uuid_s_ok) return -1; + uuid_enc_be(uuid->uuid_raw, &uuid->uuid); + return 0; } #else @@ -101,15 +109,23 @@ int libxl_uuid_from_string(libxl_uuid *uuid, const char *in) #undef LIBXL__UUID_PTRS #endif +int libxl_uuid_from_bytearray(libxl_uuid *uuid, const uint8_t *raw) +{ + uuid_dec_le(raw, &uuid->uuid); + uuid_enc_be(uuid->uuid_raw, &uuid->uuid); + + return 0; +} + void libxl_uuid_copy(libxl_ctx *ctx_opt, libxl_uuid *dst, const libxl_uuid *src) { - memcpy(&dst->uuid, &src->uuid, sizeof(dst->uuid)); + memcpy(dst, src, sizeof(*dst)); } void libxl_uuid_clear(libxl_uuid *uuid) { - memset(&uuid->uuid, 0, sizeof(uuid->uuid)); + memset(uuid, 0, sizeof(*uuid)); } #ifdef __FreeBSD__ diff --git a/tools/libxl/libxl_uuid.h b/tools/libxl/libxl_uuid.h index c5041c7..d84e3d1 100644 --- a/tools/libxl/libxl_uuid.h +++ b/tools/libxl/libxl_uuid.h @@ -42,7 +42,7 @@ typedef struct { #include <stdio.h> #include <assert.h> -typedef union { +typedef struct { uuid_t uuid; uint8_t uuid_raw[16]; } libxl_uuid; @@ -73,6 +73,7 @@ void libxl_uuid_clear(libxl_uuid *uuid); int libxl_uuid_compare(const libxl_uuid *uuid1, const libxl_uuid *uuid2); const uint8_t *libxl_uuid_bytearray_const(const libxl_uuid *uuid); uint8_t *libxl_uuid_bytearray(libxl_uuid *uuid); +int libxl_uuid_from_bytearray(libxl_uuid *uuid, const uint8_t *raw); #endif /* __LIBXL_UUID_H__ */
libxl makes the assumtion that libxl_uuid == uuid_t, and that uuid_t can be freely used as a byte array. This is not true on FreeBSD (and NetBSD too, not sure about other BSD UUID implementations), where the internals of uuid don't match what libxl expects as a byte array because of endianness issues. Fix this by converting the libxl_uuid type to a struct with an internal uuid_t field and a byte-array. Also introduce a new function that should be used in order to load a byte array into a uuid_t struct. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- tools/libxl/libxl.c | 2 +- tools/libxl/libxl.h | 9 +++++++++ tools/libxl/libxl_uuid.c | 22 +++++++++++++++++++--- tools/libxl/libxl_uuid.h | 3 ++- 4 files changed, 31 insertions(+), 5 deletions(-)