[Qemu-devel] [PATCH] xen: Log errno rather than return value

[Qemu-devel] [PATCH] xen: Log errno rather than return value

[Ross Lagerwall]

xen_modified_memory() sets errno to communicate what went wrong so log
this rather than the return value which is not interesting.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 hw/i386/xen/xen-hvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index d9ccd5d..8028bed 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1446,7 +1446,7 @@ void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t length)
         if (rc) {
             fprintf(stderr,
                     "%s failed for "RAM_ADDR_FMT" ("RAM_ADDR_FMT"): %i, %s\n",
-                    __func__, start, nb_pages, rc, strerror(-rc));
+                    __func__, start, nb_pages, errno, strerror(errno));
         }
     }
 }
-- 
2.9.5

Re: [Qemu-devel] [PATCH] xen: Log errno rather than return value

[Anthony PERARD]

On Wed, Oct 11, 2017 at 04:52:03PM +0100, Ross Lagerwall wrote:
> xen_modified_memory() sets errno to communicate what went wrong so log
> this rather than the return value which is not interesting.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>  hw/i386/xen/xen-hvm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index d9ccd5d..8028bed 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -1446,7 +1446,7 @@ void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t length)
>          if (rc) {
>              fprintf(stderr,
>                      "%s failed for "RAM_ADDR_FMT" ("RAM_ADDR_FMT"): %i, %s\n",
> -                    __func__, start, nb_pages, rc, strerror(-rc));
> +                    __func__, start, nb_pages, errno, strerror(errno));
>          }
>      }
>  }

There is already a patch, not applied yet for it,
https://lists.nongnu.org/archive/html/qemu-devel/2017-09/msg07541.html
with the only difference is printing the value of rc and not errno (both
patch user errno for strerror).

I guess this patch is better, so
Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH] xen: Log errno rather than return value

[Paul Durrant]

> -----Original Message-----
> From: Qemu-devel [mailto:qemu-devel-
> bounces+paul.durrant=citrix.com@nongnu.org] On Behalf Of Ross Lagerwall
> Sent: 11 October 2017 16:52
> To: qemu-devel@nongnu.org
> Cc: Anthony Perard <anthony.perard@citrix.com>; Ross Lagerwall
> <ross.lagerwall@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>
> Subject: [Qemu-devel] [PATCH] xen: Log errno rather than return value
> 
> xen_modified_memory() sets errno to communicate what went wrong so
> log
> this rather than the return value which is not interesting.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>  hw/i386/xen/xen-hvm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index d9ccd5d..8028bed 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -1446,7 +1446,7 @@ void xen_hvm_modified_memory(ram_addr_t
> start, ram_addr_t length)
>          if (rc) {
>              fprintf(stderr,
>                      "%s failed for "RAM_ADDR_FMT" ("RAM_ADDR_FMT"): %i, %s\n",
> -                    __func__, start, nb_pages, rc, strerror(-rc));
> +                    __func__, start, nb_pages, errno, strerror(errno));

I think this is actually a deeper problem. If QEMU is using compat code, which one way or another will go via xencall, then xen_modified_memory() will return a hypercall errno. However, if it goes via libxendevicemodel and an up-to-date privcmd, then it will return -1 and errno will be set. Thus I think the correct fix is not this patch, but a fix in xen_modified_memory() to return -errno and a fix in libxendevicemodel and the compat code in libxencntrl to behave consistently. It's all rather horrible.

  Paul

>          }
>      }
>  }
> --
> 2.9.5
> 

Re: [Qemu-devel] [PATCH] xen: Log errno rather than return value

[Paul Durrant]

> -----Original Message-----
> From: Qemu-devel [mailto:qemu-devel-
> bounces+paul.durrant=citrix.com@nongnu.org] On Behalf Of Paul Durrant
> Sent: 12 October 2017 10:47
> To: Ross Lagerwall <ross.lagerwall@citrix.com>; qemu-devel@nongnu.org
> Cc: Anthony Perard <anthony.perard@citrix.com>; Ross Lagerwall
> <ross.lagerwall@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>
> Subject: Re: [Qemu-devel] [PATCH] xen: Log errno rather than return value
> 
> > -----Original Message-----
> > From: Qemu-devel [mailto:qemu-devel-
> > bounces+paul.durrant=citrix.com@nongnu.org] On Behalf Of Ross
> Lagerwall
> > Sent: 11 October 2017 16:52
> > To: qemu-devel@nongnu.org
> > Cc: Anthony Perard <anthony.perard@citrix.com>; Ross Lagerwall
> > <ross.lagerwall@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>
> > Subject: [Qemu-devel] [PATCH] xen: Log errno rather than return value
> >
> > xen_modified_memory() sets errno to communicate what went wrong so
> > log
> > this rather than the return value which is not interesting.
> >
> > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> > ---
> >  hw/i386/xen/xen-hvm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> > index d9ccd5d..8028bed 100644
> > --- a/hw/i386/xen/xen-hvm.c
> > +++ b/hw/i386/xen/xen-hvm.c
> > @@ -1446,7 +1446,7 @@ void xen_hvm_modified_memory(ram_addr_t
> > start, ram_addr_t length)
> >          if (rc) {
> >              fprintf(stderr,
> >                      "%s failed for "RAM_ADDR_FMT" ("RAM_ADDR_FMT"): %i,
> %s\n",
> > -                    __func__, start, nb_pages, rc, strerror(-rc));
> > +                    __func__, start, nb_pages, errno, strerror(errno));
> 
> I think this is actually a deeper problem. If QEMU is using compat code, which
> one way or another will go via xencall, then xen_modified_memory() will
> return a hypercall errno.

Having waded through the code now it turns out that (for linux at least) xencall returns the value returned from the underlying ioctl() call so it will actually return -1 with errno set correctly, so the above statement is incorrect. Thus there is no inconsistency and the patch DTRT.

Sorry for the noise.

  Paul

> However, if it goes via libxendevicemodel and an
> up-to-date privcmd, then it will return -1 and errno will be set. Thus I think
> the correct fix is not this patch, but a fix in xen_modified_memory() to return
> -errno and a fix in libxendevicemodel and the compat code in libxencntrl to
> behave consistently. It's all rather horrible.
> 
>   Paul
> 
> >          }
> >      }
> >  }
> > --
> > 2.9.5
> >
>