Changeset
hw/i386/xen/xen-hvm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Git apply log
Switched to a new branch '20171011155203.16502-1-ross.lagerwall@citrix.com'
Applying: xen: Log errno rather than return value
To https://github.com/patchew-project/qemu
 + ad80d51a64...610f27f6a2 patchew/20171011155203.16502-1-ross.lagerwall@citrix.com -> patchew/20171011155203.16502-1-ross.lagerwall@citrix.com (forced update)
Test passed: docker

loading

Test passed: s390x

loading

Test passed: checkpatch

loading

[Qemu-devel] [PATCH] xen: Log errno rather than return value
Posted by Ross Lagerwall, 9 weeks ago
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
Posted by Anthony PERARD, 9 weeks ago
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
Posted by Paul Durrant, 8 weeks ago
> -----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
Posted by Paul Durrant, 8 weeks ago
> -----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
> >
>