[PATCH 0/2] libxl: FreeBSD fixes

[PATCH 0/2] libxl: FreeBSD fixes

[Roger Pau Monne]

Hello,

This series contains a couple of small fixes for FreeBSD. The first one is 
regarding the newly introduced libxl__dm_runas_helper function and it's 
usage of sysconf, while the second one fixes a long-stading bug that impacts 
UUID usage on FreeBSD (and NetBSD AFAICT).

Thanks, Roger.

[PATCH 1/2] libxl: fix _SC_GETPW_R_SIZE_MAX usage

[Roger Pau Monne]

According to the FreeBSD sysconf man page [0] if the variable is associated
with functionality that is not supported, -1 is returned and errno is not
modified. Modify libxl__dm_runas_helper so it's able to correctly
deal with this situation by setting the initial buffer value to 2048.

[0] https://www.freebsd.org/cgi/man.cgi?query=sysconf

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libxl/libxl_dm.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 0aaefd9..ec8fb51 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -728,7 +728,14 @@ static int libxl__dm_runas_helper(libxl__gc *gc, const char *username)
     long buf_size;
     int ret;
 
+    errno = 0;
     buf_size = sysconf(_SC_GETPW_R_SIZE_MAX);
+    if (buf_size < 0 && errno == 0) {
+        buf_size = 2048;
+        LOG(DEBUG,
+"sysconf(_SC_GETPW_R_SIZE_MAX) is not supported, using a buffer size of %ld",
+            buf_size);
+    }
     if (buf_size < 0) {
         LOGE(ERROR, "sysconf(_SC_GETPW_R_SIZE_MAX) returned error %ld",
                 buf_size);
-- 
1.9.5 (Apple Git-50.3)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1/2] libxl: fix _SC_GETPW_R_SIZE_MAX usage

[Doug Goldstein]

[-- Attachment #1.1: Type: text/plain, Size: 1938 bytes --]

On 1/12/16 7:14 AM, Roger Pau Monne wrote:
> According to the FreeBSD sysconf man page [0] if the variable is associated
> with functionality that is not supported, -1 is returned and errno is not
> modified. Modify libxl__dm_runas_helper so it's able to correctly
> deal with this situation by setting the initial buffer value to 2048.
> 
> [0] https://www.freebsd.org/cgi/man.cgi?query=sysconf
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  tools/libxl/libxl_dm.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 0aaefd9..ec8fb51 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -728,7 +728,14 @@ static int libxl__dm_runas_helper(libxl__gc *gc, const char *username)
>      long buf_size;
>      int ret;
>  
> +    errno = 0;
>      buf_size = sysconf(_SC_GETPW_R_SIZE_MAX);
> +    if (buf_size < 0 && errno == 0) {
> +        buf_size = 2048;
> +        LOG(DEBUG,
> +"sysconf(_SC_GETPW_R_SIZE_MAX) is not supported, using a buffer size of %ld",
> +            buf_size);
> +    }
>      if (buf_size < 0) {
>          LOGE(ERROR, "sysconf(_SC_GETPW_R_SIZE_MAX) returned error %ld",
>                  buf_size);
> 

So on Linux the behavior is somewhat similar [1]. But I took a peek at
the libvirt code for doing the similar thing and I notice that they just
default if the value is returned as less than 0 [2]. Reading both man
pages it seems like that would be the better bet instead of failing how
the current code is. Your fix probably makes it fail less but it could
still error out senselessly. Just a suggestion for an improvement overall.

[1] http://man7.org/linux/man-pages/man3/sysconf.3.html#RETURN_VALUE
[2]
http://libvirt.org/git/?p=libvirt.git;a=blob;f=src/util/virutil.c;h=bb9604a0c1ffb9c99e454e84878a8c376f773046;hb=HEAD#l935

-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1/2] libxl: fix _SC_GETPW_R_SIZE_MAX usage

[Roger Pau Monné]

El 13/01/16 a les 4.19, Doug Goldstein ha escrit:
> On 1/12/16 7:14 AM, Roger Pau Monne wrote:
>> According to the FreeBSD sysconf man page [0] if the variable is associated
>> with functionality that is not supported, -1 is returned and errno is not
>> modified. Modify libxl__dm_runas_helper so it's able to correctly
>> deal with this situation by setting the initial buffer value to 2048.
>>
>> [0] https://www.freebsd.org/cgi/man.cgi?query=sysconf
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>>  tools/libxl/libxl_dm.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
>> index 0aaefd9..ec8fb51 100644
>> --- a/tools/libxl/libxl_dm.c
>> +++ b/tools/libxl/libxl_dm.c
>> @@ -728,7 +728,14 @@ static int libxl__dm_runas_helper(libxl__gc *gc, const char *username)
>>      long buf_size;
>>      int ret;
>>  
>> +    errno = 0;
>>      buf_size = sysconf(_SC_GETPW_R_SIZE_MAX);
>> +    if (buf_size < 0 && errno == 0) {
>> +        buf_size = 2048;
>> +        LOG(DEBUG,
>> +"sysconf(_SC_GETPW_R_SIZE_MAX) is not supported, using a buffer size of %ld",
>> +            buf_size);
>> +    }
>>      if (buf_size < 0) {
>>          LOGE(ERROR, "sysconf(_SC_GETPW_R_SIZE_MAX) returned error %ld",
>>                  buf_size);
>>
> 
> So on Linux the behavior is somewhat similar [1]. But I took a peek at
> the libvirt code for doing the similar thing and I notice that they just
> default if the value is returned as less than 0 [2]. Reading both man
> pages it seems like that would be the better bet instead of failing how
> the current code is. Your fix probably makes it fail less but it could
> still error out senselessly. Just a suggestion for an improvement overall.

Ack, it doesn't make much sense to error out if we cannot find an
initial value for the buffer, the code below is able to adapt and expand
the buffer as needed anyway. I'm going to resend with that fixed.

Thanks, Roger.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1/2] libxl: fix _SC_GETPW_R_SIZE_MAX usage

[Ian Campbell]

On Wed, 2016-01-13 at 10:08 +0100, Roger Pau Monné wrote:
> El 13/01/16 a les 4.19, Doug Goldstein ha escrit:
> > On 1/12/16 7:14 AM, Roger Pau Monne wrote:
> > > According to the FreeBSD sysconf man page [0] if the variable is
> > > associated
> > > with functionality that is not supported, -1 is returned and errno is
> > > not
> > > modified. Modify libxl__dm_runas_helper so it's able to correctly
> > > deal with this situation by setting the initial buffer value to 2048.
> > > 
> > > [0] https://www.freebsd.org/cgi/man.cgi?query=sysconf

http://pubs.opengroup.org/onlinepubs/9699919799/functions/sysconf.html (whi
ch IMHO is the more canonical documentation) describes a similar error
behaviour, although for a subtly different case (no limit for the given
variable).

> > > 
> > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > ---
> > >  tools/libxl/libxl_dm.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> > > index 0aaefd9..ec8fb51 100644
> > > --- a/tools/libxl/libxl_dm.c
> > > +++ b/tools/libxl/libxl_dm.c
> > > @@ -728,7 +728,14 @@ static int libxl__dm_runas_helper(libxl__gc *gc,
> > > const char *username)
> > >      long buf_size;
> > >      int ret;
> > >  
> > > +    errno = 0;
> > >      buf_size = sysconf(_SC_GETPW_R_SIZE_MAX);
> > > +    if (buf_size < 0 && errno == 0) {
> > > +        buf_size = 2048;
> > > +        LOG(DEBUG,
> > > +"sysconf(_SC_GETPW_R_SIZE_MAX) is not supported, using a buffer size of %ld",

"... an initial buffer size of ..."

> > > +            buf_size);
> > > +    }
> > >      if (buf_size < 0) {
> > >          LOGE(ERROR, "sysconf(_SC_GETPW_R_SIZE_MAX) returned error
> > > %ld",
> > >                  buf_size);
> > > 
> > 
> > So on Linux the behavior is somewhat similar [1]. But I took a peek at
> > the libvirt code for doing the similar thing and I notice that they
> > just
> > default if the value is returned as less than 0 [2]. Reading both man
> > pages it seems like that would be the better bet instead of failing how
> > the current code is. Your fix probably makes it fail less but it could
> > still error out senselessly. Just a suggestion for an improvement
> > overall.
> 
> Ack, it doesn't make much sense to error out if we cannot find an
> initial value for the buffer, the code below is able to adapt and expand
> the buffer as needed anyway. I'm going to resend with that fixed.

Sounds good.

Don't forget to CC all the tools maintainers next time.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 2/2] libxl: fix UUID usage on FreeBSD

[Roger Pau Monne]

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(-)

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__ */
 
-- 
1.9.5 (Apple Git-50.3)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 2/2] libxl: fix UUID usage on FreeBSD

[Ian Campbell]

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*.

Re: [PATCH 2/2] libxl: fix UUID usage on FreeBSD

[Roger Pau Monné]

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.

Re: [PATCH 2/2] libxl: fix UUID usage on FreeBSD

[Ian Campbell]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 2/2] libxl: fix UUID usage on FreeBSD

[Ian Jackson]

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.

Re: [PATCH 0/2] libxl: FreeBSD fixes

[Roger Pau Monné]

El 12/01/16 a les 14.14, Roger Pau Monne ha escrit:
> Hello,
> 
> This series contains a couple of small fixes for FreeBSD. The first one is 
> regarding the newly introduced libxl__dm_runas_helper function and it's 
> usage of sysconf, while the second one fixes a long-stading bug that impacts 
> UUID usage on FreeBSD (and NetBSD AFAICT).
> 
> Thanks, Roger.

Hm, it seems like I haven't Cced any of the maintainers, doing it here
to reduce spam, sorry for the trouble.

Roger.