diff mbox

[v2] libxl: replace the usage of uuid_t with a char array

Message ID 1459941065-17573-1-git-send-email-roger.pau@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Roger Pau Monné April 6, 2016, 11:11 a.m. UTC
The internals of the uuid_t struct don't match a big endian octet stream on
*BSD systems, which means that it cannot be directly casted to a
uint8_t[16].

In order to solve that change the type to be an unsigned char[16], which
doesn't imply any other change on Linux. On *BSDs change the helpers so that
the uuid is always stored as a big endian byte stream.

NB: tested on FreeBSD and Linux only.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Discussed-with: Ian Jackson <Ian.Jackson@eu.citrix.com>
Discussed-with: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
NB2: AFAICT the NetBSD version of libxl_uuid_from_string *could* be switched
to the FreeBSD one (because NetBSD also has uuid_from_string), but I don't
have a NetBSD box in order to test it.
---
Changes since v1:
 - Readd a line that was deleted by error.
---
 tools/libxl/libxl_osdeps.h |  3 +++
 tools/libxl/libxl_uuid.c   | 28 ++++++++++++++++++++--------
 tools/libxl/libxl_uuid.h   | 21 ++++++++-------------
 3 files changed, 31 insertions(+), 21 deletions(-)

Comments

Wei Liu April 6, 2016, 11:14 a.m. UTC | #1
On Wed, Apr 06, 2016 at 01:11:05PM +0200, Roger Pau Monne wrote:
> The internals of the uuid_t struct don't match a big endian octet stream on
> *BSD systems, which means that it cannot be directly casted to a
> uint8_t[16].
> 
> In order to solve that change the type to be an unsigned char[16], which
> doesn't imply any other change on Linux. On *BSDs change the helpers so that
> the uuid is always stored as a big endian byte stream.
> 
> NB: tested on FreeBSD and Linux only.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Discussed-with: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Discussed-with: Wei Liu <wei.liu2@citrix.com>
> ---
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
> NB2: AFAICT the NetBSD version of libxl_uuid_from_string *could* be switched
> to the FreeBSD one (because NetBSD also has uuid_from_string), but I don't
> have a NetBSD box in order to test it.

Haven't looked at the code but maybe it's a good idea to CC netbsd
port-xen list?

Wei.
Wei Liu April 7, 2016, 4:12 p.m. UTC | #2
On Wed, Apr 06, 2016 at 01:11:05PM +0200, Roger Pau Monne wrote:
> The internals of the uuid_t struct don't match a big endian octet stream on
> *BSD systems, which means that it cannot be directly casted to a
> uint8_t[16].
> 
> In order to solve that change the type to be an unsigned char[16], which
> doesn't imply any other change on Linux. On *BSDs change the helpers so that
> the uuid is always stored as a big endian byte stream.
> 
> NB: tested on FreeBSD and Linux only.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Discussed-with: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Discussed-with: Wei Liu <wei.liu2@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Wei Liu April 7, 2016, 4:17 p.m. UTC | #3
On Wed, Apr 06, 2016 at 01:11:05PM +0200, Roger Pau Monne wrote:
> The internals of the uuid_t struct don't match a big endian octet stream on
> *BSD systems, which means that it cannot be directly casted to a
> uint8_t[16].
> 
> In order to solve that change the type to be an unsigned char[16], which
> doesn't imply any other change on Linux. On *BSDs change the helpers so that
> the uuid is always stored as a big endian byte stream.
> 
> NB: tested on FreeBSD and Linux only.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Discussed-with: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Discussed-with: Wei Liu <wei.liu2@citrix.com>
> ---
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
> NB2: AFAICT the NetBSD version of libxl_uuid_from_string *could* be switched
> to the FreeBSD one (because NetBSD also has uuid_from_string), but I don't
> have a NetBSD box in order to test it.
> ---
> Changes since v1:
>  - Readd a line that was deleted by error.
> ---
>  tools/libxl/libxl_osdeps.h |  3 +++
>  tools/libxl/libxl_uuid.c   | 28 ++++++++++++++++++++--------
>  tools/libxl/libxl_uuid.h   | 21 ++++++++-------------

A thought: maybe it is worth to have a #define LIBXL_HAVE_UNIFIED_UUID
in libxl.h?

/* If this is defined, libxl_uuid is big endian 16-octet stream on all
 * platform. The libxl_uuid API family will handle transformation
 * between libxl_uuid format and OS specific format.
 */
#define LBIXL_HAVE_UNIFIED_UUID 1

Wei.
Ian Jackson April 7, 2016, 5:41 p.m. UTC | #4
Wei Liu writes ("Re: [PATCH v2] libxl: replace the usage of uuid_t with a char array"):
> A thought: maybe it is worth to have a #define LIBXL_HAVE_UNIFIED_UUID
> in libxl.h?

This is a good idea.

> /* If this is defined, libxl_uuid is big endian 16-octet stream on all
>  * platform. The libxl_uuid API family will handle transformation
>  * between libxl_uuid format and OS specific format.
>  */
> #define LBIXL_HAVE_UNIFIED_UUID 1

But the wording here isn't, quite.  The "transformation between
libxl_uuid format and OS specific format" is entirely hidden within
libxl and exists only to implement the functions provided by libxl.

Ian.
Roger Pau Monné April 8, 2016, 10:29 a.m. UTC | #5
On Thu, 7 Apr 2016, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH v2] libxl: replace the usage of uuid_t with a char array"):
> > A thought: maybe it is worth to have a #define LIBXL_HAVE_UNIFIED_UUID
> > in libxl.h?
> 
> This is a good idea.
> 
> > /* If this is defined, libxl_uuid is big endian 16-octet stream on all
> >  * platform. The libxl_uuid API family will handle transformation
> >  * between libxl_uuid format and OS specific format.
> >  */
> > #define LBIXL_HAVE_UNIFIED_UUID 1
> 
> But the wording here isn't, quite.  The "transformation between
> libxl_uuid format and OS specific format" is entirely hidden within
> libxl and exists only to implement the functions provided by libxl.

What about:

/*
 * LIBXL_HAVE_BYTEARRAY_UUID
 *
 * If this is defined, the internal member of libxl_uuid is defined
 * as a 16 byte array that contains the UUID in big endian format.
 * Also, the same structure layout is used across all OSes.
 */
#define LIBXL_HAVE_BYTEARRAY_UUID 1
Ian Jackson April 8, 2016, 1:42 p.m. UTC | #6
Roger Pau Monné writes ("Re: [PATCH v2] libxl: replace the usage of uuid_t with a char array"):
> What about:
> 
> /*
>  * LIBXL_HAVE_BYTEARRAY_UUID
>  *
>  * If this is defined, the internal member of libxl_uuid is defined
>  * as a 16 byte array that contains the UUID in big endian format.
>  * Also, the same structure layout is used across all OSes.
>  */
> #define LIBXL_HAVE_BYTEARRAY_UUID 1

Yes.

Ian.
diff mbox

Patch

diff --git a/tools/libxl/libxl_osdeps.h b/tools/libxl/libxl_osdeps.h
index 802c762..10ce703 100644
--- a/tools/libxl/libxl_osdeps.h
+++ b/tools/libxl/libxl_osdeps.h
@@ -30,6 +30,7 @@ 
 #define SYSFS_PCIBACK_DRIVER   "/kern/xen/pci"
 #define NETBACK_NIC_NAME       "xvif%ui%d"
 #include <util.h>
+#include <uuid.h>
 #elif defined(__OpenBSD__)
 #include <util.h>
 #elif defined(__linux__)
@@ -39,6 +40,7 @@ 
 #define SYSFS_PCIBACK_DRIVER   "/sys/bus/pci/drivers/pciback"
 #define NETBACK_NIC_NAME       "vif%u.%d"
 #include <pty.h>
+#include <uuid/uuid.h>
 #elif defined(__sun__)
 #include <stropts.h>
 #elif defined(__FreeBSD__)
@@ -49,6 +51,7 @@ 
 #define NETBACK_NIC_NAME       "xnb%u.%d"
 #include <libutil.h>
 #include <sys/endian.h>
+#include <uuid.h>
 #endif
 
 #ifndef SYSFS_USBBACK_DRIVER
diff --git a/tools/libxl/libxl_uuid.c b/tools/libxl/libxl_uuid.c
index 7d4a032..dadb79b 100644
--- a/tools/libxl/libxl_uuid.c
+++ b/tools/libxl/libxl_uuid.c
@@ -64,27 +64,35 @@  uint8_t *libxl_uuid_bytearray(libxl_uuid *uuid)
 int libxl_uuid_is_nil(const libxl_uuid *uuid)
 {
     uint32_t status;
+    uuid_t nat_uuid;
 
-    return uuid_is_nil(&uuid->uuid, &status);
+    uuid_dec_be(uuid->uuid, &nat_uuid);
+
+    return uuid_is_nil(&nat_uuid, &status);
 }
 
 void libxl_uuid_generate(libxl_uuid *uuid)
 {
     uint32_t status;
+    uuid_t nat_uuid;
 
-    BUILD_BUG_ON(sizeof(libxl_uuid) != sizeof(uuid_t));
-    uuid_create(&uuid->uuid, &status);
+    uuid_create(&nat_uuid, &status);
     assert(status == uuid_s_ok);
+
+    uuid_enc_be(uuid->uuid, &nat_uuid);
 }
 
 #ifdef __FreeBSD__
 int libxl_uuid_from_string(libxl_uuid *uuid, const char *in)
 {
     uint32_t status;
+    uuid_t nat_uuid;
 
-    uuid_from_string(in, &uuid->uuid, &status);
+    uuid_from_string(in, &nat_uuid, &status);
     if (status != uuid_s_ok)
-        return -1;
+        return ERROR_FAIL;
+    uuid_enc_be(uuid->uuid, &nat_uuid);
+
     return 0;
 }
 #else
@@ -115,8 +123,12 @@  void libxl_uuid_clear(libxl_uuid *uuid)
 #ifdef __FreeBSD__
 int libxl_uuid_compare(const libxl_uuid *uuid1, const libxl_uuid *uuid2)
 {
+    uuid_t nat_uuid1, nat_uuid2;
 
-    return uuid_compare(&uuid1->uuid, &uuid2->uuid, NULL);
+    uuid_dec_be(uuid1->uuid, &nat_uuid1);
+    uuid_dec_be(uuid2->uuid, &nat_uuid2);
+
+    return uuid_compare(&nat_uuid1, &nat_uuid2, NULL);
 }
 #else
 int libxl_uuid_compare(const libxl_uuid *uuid1, const libxl_uuid *uuid2)
@@ -128,13 +140,13 @@  int libxl_uuid_compare(const libxl_uuid *uuid1, const libxl_uuid *uuid2)
 const uint8_t *libxl_uuid_bytearray_const(const libxl_uuid *uuid)
 {
 
-    return uuid->uuid_raw;
+    return uuid->uuid;
 }
 
 uint8_t *libxl_uuid_bytearray(libxl_uuid *uuid)
 {
 
-    return uuid->uuid_raw;
+    return uuid->uuid;
 }
 #else
 
diff --git a/tools/libxl/libxl_uuid.h b/tools/libxl/libxl_uuid.h
index c5041c7..994320d 100644
--- a/tools/libxl/libxl_uuid.h
+++ b/tools/libxl/libxl_uuid.h
@@ -21,18 +21,19 @@ 
                                 uuid[4], uuid[5], uuid[6], uuid[7], \
                                 uuid[8], uuid[9], uuid[10], uuid[11], \
                                 uuid[12], uuid[13], uuid[14], uuid[15]
+#define LIBXL_UUID_BYTES(arg) LIBXL__UUID_BYTES((arg).uuid)
 
+typedef struct {
+    /* UUID as an octet stream in big-endian byte-order. */
+    unsigned char uuid[16];
+} libxl_uuid;
+
+#if defined(LIBXL_API_VERSION) && LIBXL_API_VERSION < 0x040700
 #if defined(__linux__)
 
 #include <uuid/uuid.h>
 #include <stdint.h>
 
-typedef struct {
-    uuid_t uuid;
-} libxl_uuid;
-
-#define LIBXL_UUID_BYTES(arg) LIBXL__UUID_BYTES(((uint8_t *)arg.uuid))
-
 #elif defined(__FreeBSD__) || defined(__NetBSD__)
 
 #include <uuid.h>
@@ -42,18 +43,12 @@  typedef struct {
 #include <stdio.h>
 #include <assert.h>
 
-typedef union {
-    uuid_t uuid;
-    uint8_t uuid_raw[16];
-} libxl_uuid;
-
-#define LIBXL_UUID_BYTES(arg) LIBXL__UUID_BYTES(arg.uuid_raw)
-
 #else
 
 #error "Please update libxl_uuid.h for your OS"
 
 #endif
+#endif
 
 int libxl_uuid_is_nil(const libxl_uuid *uuid);
 void libxl_uuid_generate(libxl_uuid *uuid);