[PATCH 1/2] xen: fix usage of ENODATA

[PATCH 1/2] xen: fix usage of ENODATA

[Roger Pau Monne]

ENODATA doesn't exist on FreeBSD, so ENODATA errors returned by the
hypervisor are translated to ENOENT.

Also, the error code is returned in errno if the call returns -1, so
compare the error code with the value in errno instead of the value
returned by the function.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: xen-devel@lists.xenproject.org
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Anthony Perard <anthony.perard@citrix.com>
---
 xen-all.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/xen-all.c b/xen-all.c
index 0b4d934..4026b7b 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -499,11 +499,15 @@ static void xen_sync_dirty_bitmap(XenIOState *state,
                                  start_addr >> TARGET_PAGE_BITS, npages,
                                  bitmap);
     if (rc < 0) {
-        if (rc != -ENODATA) {
+#ifdef ENODATA
+        if (errno == ENODATA) {
+#else
+        if (errno == ENOENT) {
+#endif
             memory_region_set_dirty(framebuffer, 0, size);
             DPRINTF("xen: track_dirty_vram failed (0x" TARGET_FMT_plx
                     ", 0x" TARGET_FMT_plx "): %s\n",
-                    start_addr, start_addr + size, strerror(-rc));
+                    start_addr, start_addr + size, strerror(errno));
         }
         return;
     }
-- 
1.7.7.5 (Apple Git-26)


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

[PATCH 2/2] tap-bsd: implement a FreeBSD only version of tap_open

[Roger Pau Monne]

The current behaviour of tap_open for BSD systems differ greatly from
it's Linux counterpart. Since FreeBSD supports interface renaming and
tap device cloning by opening /dev/tap, implement a FreeBSD specific
version of tap_open that behaves like it's Linux counterpart.

This is specially important for toolstacks that use Qemu (like Xen
libxl), in order to have a unified behaviour across suported
platforms.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: xen-devel@lists.xenproject.org
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Anthony Liguori <aliguori@us.ibm.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
---
 net/tap-bsd.c |   70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 69 insertions(+), 1 deletions(-)

diff --git a/net/tap-bsd.c b/net/tap-bsd.c
index 90f8a02..bf91bd0 100644
--- a/net/tap-bsd.c
+++ b/net/tap-bsd.c
@@ -27,12 +27,13 @@
 #include "sysemu/sysemu.h"
 #include "qemu/error-report.h"
 
-#ifdef __NetBSD__
+#if defined(__NetBSD__) || defined(__FreeBSD__)
 #include <sys/ioctl.h>
 #include <net/if.h>
 #include <net/if_tap.h>
 #endif
 
+#ifndef __FreeBSD__
 int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
              int vnet_hdr_required, int mq_required)
 {
@@ -108,6 +109,73 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
     return fd;
 }
 
+#else /* __FreeBSD__ */
+
+#define PATH_NET_TAP "/dev/tap"
+
+int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
+             int vnet_hdr_required, int mq_required)
+{
+    int fd, s, ret;
+    struct ifreq ifr;
+
+    TFR(fd = open(PATH_NET_TAP, O_RDWR));
+    if (fd < 0) {
+        error_report("could not open %s: %s", PATH_NET_TAP, strerror(errno));
+        return -1;
+    }
+
+    memset(&ifr, 0, sizeof(ifr));
+
+    ret = ioctl(fd, TAPGIFNAME, (void *)&ifr);
+    if (ret < 0) {
+        error_report("could not get tap interface name");
+        goto error;
+    }
+
+    if (ifname[0] != '\0') {
+        /* User requested the interface to have a specific name */
+        s = socket(AF_LOCAL, SOCK_DGRAM, 0);
+        if (s < 0) {
+            error_report("could not open socket to set interface name");
+            goto error;
+        }
+        ifr.ifr_data = ifname;
+        ret = ioctl(s, SIOCSIFNAME, (void *)&ifr);
+        close(s);
+        if (ret < 0) {
+            error_report("could not set tap interface name");
+            goto error;
+        }
+    } else {
+        pstrcpy(ifname, ifname_size, ifr.ifr_name);
+    }
+
+    if (*vnet_hdr) {
+        /* BSD doesn't have IFF_VNET_HDR */
+        *vnet_hdr = 0;
+
+        if (vnet_hdr_required && !*vnet_hdr) {
+            error_report("vnet_hdr=1 requested, but no kernel "
+                         "support for IFF_VNET_HDR available");
+            goto error;
+        }
+    }
+    if (mq_required) {
+        error_report("mq_required requested, but not kernel support"
+                     "for IFF_MULTI_QUEUE available");
+        goto error;
+    }
+
+    fcntl(fd, F_SETFL, O_NONBLOCK);
+    return fd;
+
+error:
+    close(fd);
+    return -1;
+}
+#endif /* __FreeBSD__ */
+
 int tap_set_sndbuf(int fd, const NetdevTapOptions *tap)
 {
     return 0;
-- 
1.7.7.5 (Apple Git-26)


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

Re: [PATCH 1/2] xen: fix usage of ENODATA

[Egger, Christoph]

On 15.04.14 10:33, Roger Pau Monne wrote:
> ENODATA doesn't exist on FreeBSD, so ENODATA errors returned by the
> hypervisor are translated to ENOENT.
> 
> Also, the error code is returned in errno if the call returns -1, so
> compare the error code with the value in errno instead of the value
> returned by the function.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: xen-devel@lists.xenproject.org
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> ---
>  xen-all.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/xen-all.c b/xen-all.c
> index 0b4d934..4026b7b 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -499,11 +499,15 @@ static void xen_sync_dirty_bitmap(XenIOState *state,
>                                   start_addr >> TARGET_PAGE_BITS, npages,
>                                   bitmap);
>      if (rc < 0) {
> -        if (rc != -ENODATA) {
> +#ifdef ENODATA
> +        if (errno == ENODATA) {
> +#else
> +        if (errno == ENOENT) {
> +#endif

This does not look good to me.
I suggest this:

#ifndef ENODATA
#define ENODATA  ENOENT
#endif

Christoph

>              memory_region_set_dirty(framebuffer, 0, size);
>              DPRINTF("xen: track_dirty_vram failed (0x" TARGET_FMT_plx
>                      ", 0x" TARGET_FMT_plx "): %s\n",
> -                    start_addr, start_addr + size, strerror(-rc));
> +                    start_addr, start_addr + size, strerror(errno));
>          }
>          return;
>      }
> 


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

Re: [PATCH 1/2] xen: fix usage of ENODATA

[Roger Pau Monné]

On 15/04/14 14:33, Egger, Christoph wrote:
> On 15.04.14 10:33, Roger Pau Monne wrote:
>> ENODATA doesn't exist on FreeBSD, so ENODATA errors returned by the
>> hypervisor are translated to ENOENT.
>>
>> Also, the error code is returned in errno if the call returns -1, so
>> compare the error code with the value in errno instead of the value
>> returned by the function.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Cc: xen-devel@lists.xenproject.org
>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> Cc: Anthony Perard <anthony.perard@citrix.com>
>> ---
>>  xen-all.c |    8 ++++++--
>>  1 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen-all.c b/xen-all.c
>> index 0b4d934..4026b7b 100644
>> --- a/xen-all.c
>> +++ b/xen-all.c
>> @@ -499,11 +499,15 @@ static void xen_sync_dirty_bitmap(XenIOState *state,
>>                                   start_addr >> TARGET_PAGE_BITS, npages,
>>                                   bitmap);
>>      if (rc < 0) {
>> -        if (rc != -ENODATA) {
>> +#ifdef ENODATA
>> +        if (errno == ENODATA) {
>> +#else
>> +        if (errno == ENOENT) {
>> +#endif
> 
> This does not look good to me.
> I suggest this:
> 
> #ifndef ENODATA
> #define ENODATA  ENOENT
> #endif

Hello Christoph,

Thanks for the review, I slightly prefer my original proposal over what
you suggest, I think it's more clear that there are different error
codes, while your suggestion simply masks the fact than FreeBSD doesn't
have ENODATA. Anyway, I wouldn't mind changing it if the maintainer
prefers your approach.

Roger.


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

Re: [PATCH 1/2] xen: fix usage of ENODATA

[Anthony PERARD]

On Tue, Apr 15, 2014 at 10:33:08AM +0200, Roger Pau Monne wrote:
> ENODATA doesn't exist on FreeBSD, so ENODATA errors returned by the
> hypervisor are translated to ENOENT.
> 
> Also, the error code is returned in errno if the call returns -1, so
> compare the error code with the value in errno instead of the value
> returned by the function.

But in xenctrl.h, the comment on the prototype of the function says that
it return the error code, there is nothing about errno been updated. So
I think rc should still be check for the error code instead of errno.

> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: xen-devel@lists.xenproject.org
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> ---
>  xen-all.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/xen-all.c b/xen-all.c
> index 0b4d934..4026b7b 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -499,11 +499,15 @@ static void xen_sync_dirty_bitmap(XenIOState *state,
>                                   start_addr >> TARGET_PAGE_BITS, npages,
>                                   bitmap);
>      if (rc < 0) {
> -        if (rc != -ENODATA) {
> +#ifdef ENODATA
> +        if (errno == ENODATA) {
> +#else
> +        if (errno == ENOENT) {
> +#endif
>              memory_region_set_dirty(framebuffer, 0, size);
>              DPRINTF("xen: track_dirty_vram failed (0x" TARGET_FMT_plx
>                      ", 0x" TARGET_FMT_plx "): %s\n",
> -                    start_addr, start_addr + size, strerror(-rc));
> +                    start_addr, start_addr + size, strerror(errno));
>          }
>          return;
>      }
> -- 
> 1.7.7.5 (Apple Git-26)
> 

-- 
Anthony PERARD

Re: [PATCH 1/2] xen: fix usage of ENODATA

[Roger Pau Monné]

On 15/04/14 18:15, Anthony PERARD wrote:
> On Tue, Apr 15, 2014 at 10:33:08AM +0200, Roger Pau Monne wrote:
>> ENODATA doesn't exist on FreeBSD, so ENODATA errors returned by the
>> hypervisor are translated to ENOENT.
>>
>> Also, the error code is returned in errno if the call returns -1, so
>> compare the error code with the value in errno instead of the value
>> returned by the function.
> 
> But in xenctrl.h, the comment on the prototype of the function says that
> it return the error code, there is nothing about errno been updated. So
> I think rc should still be check for the error code instead of errno.

Then I think the comment is wrong (or I'm missing something),
xc_hvm_track_dirty_vram return code comes from do_xen_hypercall, which
is basically a wrapper around ioctl, and according to the ioctl man page
(both Linux and FreeBSD versions):

"On error, -1 is returned, and errno is set appropriately."

Roger.

Re: [PATCH 1/2] xen: fix usage of ENODATA

[Andrew Cooper]

On 15/04/14 17:28, Roger Pau Monné wrote:
> On 15/04/14 18:15, Anthony PERARD wrote:
>> On Tue, Apr 15, 2014 at 10:33:08AM +0200, Roger Pau Monne wrote:
>>> ENODATA doesn't exist on FreeBSD, so ENODATA errors returned by the
>>> hypervisor are translated to ENOENT.
>>>
>>> Also, the error code is returned in errno if the call returns -1, so
>>> compare the error code with the value in errno instead of the value
>>> returned by the function.
>> But in xenctrl.h, the comment on the prototype of the function says that
>> it return the error code, there is nothing about errno been updated. So
>> I think rc should still be check for the error code instead of errno.
> Then I think the comment is wrong (or I'm missing something),
> xc_hvm_track_dirty_vram return code comes from do_xen_hypercall, which
> is basically a wrapper around ioctl, and according to the ioctl man page
> (both Linux and FreeBSD versions):
>
> "On error, -1 is returned, and errno is set appropriately."
>
> Roger.

The only thing you can safely assume about libxc and error values is
that there is no consistency whatsoever (and in some cases a completely
inability to determine success or failure from the return value).  This
is acknowledged as needing to be fixed.

The only way you have of actually working out the error semantics are to
follow the function you care about all the way to the ioctl.

~Andrew