Message ID | 1435605913-23826-5-git-send-email-konrad.wilk@oracle.com |
---|---|
State | New |
Headers | show |
On Mon, 29 Jun 2015, Konrad Rzeszutek Wilk wrote: > In Xen 4.6 commit cd2f100f0f61b3f333d52d1737dd73f02daee592 > "libxc: Fix do_memory_op to return negative value on errors" > made the libxc API less odd-ball: On errors, return value is > -1 and error code is in errno. On success the return value > is either 0 or an positive value. > > Since we could be running with an old toolstack in which the > Exx value is in rc or the newer, we print both and return > the -EXX depending on rc == -1 condition. > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > xen-hvm.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/xen-hvm.c b/xen-hvm.c > index 0408462..a92bc14 100644 > --- a/xen-hvm.c > +++ b/xen-hvm.c > @@ -345,11 +345,12 @@ go_physmap: > unsigned long idx = pfn + i; > xen_pfn_t gpfn = start_gpfn + i; > > + /* In Xen 4.6 rc is -1 and errno contains the error value. */ > rc = xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn); > if (rc) { > DPRINTF("add_to_physmap MFN %"PRI_xen_pfn" to PFN %" > - PRI_xen_pfn" failed: %d\n", idx, gpfn, rc); > - return -rc; > + PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, errno); > + return rc == -1 ? -errno : -rc; Printing both rc and errno is the right thing to do, but I am not sure changing return value depending on the libxc version is a good idea. Maybe we should be consistent and always return rc? > } > } > > @@ -422,11 +423,12 @@ static int xen_remove_from_physmap(XenIOState *state, > xen_pfn_t idx = start_addr + i; > xen_pfn_t gpfn = phys_offset + i; > > + /* In Xen 4.6 rc is -1 and errno contains the error value. */ > rc = xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn); > if (rc) { > fprintf(stderr, "add_to_physmap MFN %"PRI_xen_pfn" to PFN %" > - PRI_xen_pfn" failed: %d\n", idx, gpfn, rc); > - return -rc; > + PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, errno); > + return rc == -1 ? -errno : -rc; > } > } > > -- > 2.1.0 >
On Wed, Jul 01, 2015 at 02:01:07PM +0100, Stefano Stabellini wrote: > On Mon, 29 Jun 2015, Konrad Rzeszutek Wilk wrote: > > In Xen 4.6 commit cd2f100f0f61b3f333d52d1737dd73f02daee592 > > "libxc: Fix do_memory_op to return negative value on errors" > > made the libxc API less odd-ball: On errors, return value is > > -1 and error code is in errno. On success the return value > > is either 0 or an positive value. > > > > Since we could be running with an old toolstack in which the > > Exx value is in rc or the newer, we print both and return > > the -EXX depending on rc == -1 condition. > > > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > --- > > xen-hvm.c | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/xen-hvm.c b/xen-hvm.c > > index 0408462..a92bc14 100644 > > --- a/xen-hvm.c > > +++ b/xen-hvm.c > > @@ -345,11 +345,12 @@ go_physmap: > > unsigned long idx = pfn + i; > > xen_pfn_t gpfn = start_gpfn + i; > > > > + /* In Xen 4.6 rc is -1 and errno contains the error value. */ > > rc = xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn); > > if (rc) { > > DPRINTF("add_to_physmap MFN %"PRI_xen_pfn" to PFN %" > > - PRI_xen_pfn" failed: %d\n", idx, gpfn, rc); > > - return -rc; > > + PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, errno); > > + return rc == -1 ? -errno : -rc; > > Printing both rc and errno is the right thing to do, but I am not sure > changing return value depending on the libxc version is a good idea. > Maybe we should be consistent and always return rc? In Xen 4.5 and earlier this function would return -EINVAL (say rc=EINVAL). With Xen 4.6 it would always return 1 on errors (rc is -1, and with --1 we get 1), while the errno would have EINVAL. To be consistent and have this function return an proper -Exx value we need that check to use errno in case rc == -1. I am uncomfortable with returning positive values as errors, which reminds me - I need to update the commit to mention the return 1 issue. > > > > } > > } > > > > @@ -422,11 +423,12 @@ static int xen_remove_from_physmap(XenIOState *state, > > xen_pfn_t idx = start_addr + i; > > xen_pfn_t gpfn = phys_offset + i; > > > > + /* In Xen 4.6 rc is -1 and errno contains the error value. */ > > rc = xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn); > > if (rc) { > > fprintf(stderr, "add_to_physmap MFN %"PRI_xen_pfn" to PFN %" > > - PRI_xen_pfn" failed: %d\n", idx, gpfn, rc); > > - return -rc; > > + PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, errno); > > + return rc == -1 ? -errno : -rc; > > } > > } > > > > -- > > 2.1.0 > >
On Wed, 1 Jul 2015, Konrad Rzeszutek Wilk wrote: > On Wed, Jul 01, 2015 at 02:01:07PM +0100, Stefano Stabellini wrote: > > On Mon, 29 Jun 2015, Konrad Rzeszutek Wilk wrote: > > > In Xen 4.6 commit cd2f100f0f61b3f333d52d1737dd73f02daee592 > > > "libxc: Fix do_memory_op to return negative value on errors" > > > made the libxc API less odd-ball: On errors, return value is > > > -1 and error code is in errno. On success the return value > > > is either 0 or an positive value. > > > > > > Since we could be running with an old toolstack in which the > > > Exx value is in rc or the newer, we print both and return > > > the -EXX depending on rc == -1 condition. > > > > > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > > --- > > > xen-hvm.c | 10 ++++++---- > > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > > > diff --git a/xen-hvm.c b/xen-hvm.c > > > index 0408462..a92bc14 100644 > > > --- a/xen-hvm.c > > > +++ b/xen-hvm.c > > > @@ -345,11 +345,12 @@ go_physmap: > > > unsigned long idx = pfn + i; > > > xen_pfn_t gpfn = start_gpfn + i; > > > > > > + /* In Xen 4.6 rc is -1 and errno contains the error value. */ > > > rc = xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn); > > > if (rc) { > > > DPRINTF("add_to_physmap MFN %"PRI_xen_pfn" to PFN %" > > > - PRI_xen_pfn" failed: %d\n", idx, gpfn, rc); > > > - return -rc; > > > + PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, errno); > > > + return rc == -1 ? -errno : -rc; > > > > Printing both rc and errno is the right thing to do, but I am not sure > > changing return value depending on the libxc version is a good idea. > > Maybe we should be consistent and always return rc? > > In Xen 4.5 and earlier this function would return -EINVAL (say rc=EINVAL). > With Xen 4.6 it would always return 1 on errors (rc is -1, and with --1 we get 1), while > the errno would have EINVAL. > > To be consistent and have this function return an proper -Exx value we > need that check to use errno in case rc == -1. Maybe the best thing to do is to introduce a versioned xen_xc_domain_add_to_physmap to include/hw/xen/xen_common.h > I am uncomfortable with returning positive values as errors, which reminds me - > I need to update the commit to mention the return 1 issue. Agreed > > > > > > > } > > > } > > > > > > @@ -422,11 +423,12 @@ static int xen_remove_from_physmap(XenIOState *state, > > > xen_pfn_t idx = start_addr + i; > > > xen_pfn_t gpfn = phys_offset + i; > > > > > > + /* In Xen 4.6 rc is -1 and errno contains the error value. */ > > > rc = xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn); > > > if (rc) { > > > fprintf(stderr, "add_to_physmap MFN %"PRI_xen_pfn" to PFN %" > > > - PRI_xen_pfn" failed: %d\n", idx, gpfn, rc); > > > - return -rc; > > > + PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, errno); > > > + return rc == -1 ? -errno : -rc; > > > } > > > } > > > > > > -- > > > 2.1.0 > > > >
On Thu, Jul 02, 2015 at 12:00:29PM +0100, Stefano Stabellini wrote: > On Wed, 1 Jul 2015, Konrad Rzeszutek Wilk wrote: > > On Wed, Jul 01, 2015 at 02:01:07PM +0100, Stefano Stabellini wrote: > > > On Mon, 29 Jun 2015, Konrad Rzeszutek Wilk wrote: > > > > In Xen 4.6 commit cd2f100f0f61b3f333d52d1737dd73f02daee592 > > > > "libxc: Fix do_memory_op to return negative value on errors" > > > > made the libxc API less odd-ball: On errors, return value is > > > > -1 and error code is in errno. On success the return value > > > > is either 0 or an positive value. > > > > > > > > Since we could be running with an old toolstack in which the > > > > Exx value is in rc or the newer, we print both and return > > > > the -EXX depending on rc == -1 condition. > > > > > > > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > > > --- > > > > xen-hvm.c | 10 ++++++---- > > > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/xen-hvm.c b/xen-hvm.c > > > > index 0408462..a92bc14 100644 > > > > --- a/xen-hvm.c > > > > +++ b/xen-hvm.c > > > > @@ -345,11 +345,12 @@ go_physmap: > > > > unsigned long idx = pfn + i; > > > > xen_pfn_t gpfn = start_gpfn + i; > > > > > > > > + /* In Xen 4.6 rc is -1 and errno contains the error value. */ > > > > rc = xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn); > > > > if (rc) { > > > > DPRINTF("add_to_physmap MFN %"PRI_xen_pfn" to PFN %" > > > > - PRI_xen_pfn" failed: %d\n", idx, gpfn, rc); > > > > - return -rc; > > > > + PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, errno); > > > > + return rc == -1 ? -errno : -rc; > > > > > > Printing both rc and errno is the right thing to do, but I am not sure > > > changing return value depending on the libxc version is a good idea. > > > Maybe we should be consistent and always return rc? > > > > In Xen 4.5 and earlier this function would return -EINVAL (say rc=EINVAL). > > With Xen 4.6 it would always return 1 on errors (rc is -1, and with --1 we get 1), while > > the errno would have EINVAL. > > > > To be consistent and have this function return an proper -Exx value we > > need that check to use errno in case rc == -1. > > Maybe the best thing to do is to introduce a versioned > xen_xc_domain_add_to_physmap to include/hw/xen/xen_common.h Aah, hadn't seen that before. Yes will do that. > > > > I am uncomfortable with returning positive values as errors, which reminds me - > > I need to update the commit to mention the return 1 issue. > > Agreed > > > > > > > > > > > > } > > > > } > > > > > > > > @@ -422,11 +423,12 @@ static int xen_remove_from_physmap(XenIOState *state, > > > > xen_pfn_t idx = start_addr + i; > > > > xen_pfn_t gpfn = phys_offset + i; > > > > > > > > + /* In Xen 4.6 rc is -1 and errno contains the error value. */ > > > > rc = xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn); > > > > if (rc) { > > > > fprintf(stderr, "add_to_physmap MFN %"PRI_xen_pfn" to PFN %" > > > > - PRI_xen_pfn" failed: %d\n", idx, gpfn, rc); > > > > - return -rc; > > > > + PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, errno); > > > > + return rc == -1 ? -errno : -rc; > > > > } > > > > } > > > > > > > > -- > > > > 2.1.0 > > > > > >
diff --git a/xen-hvm.c b/xen-hvm.c index 0408462..a92bc14 100644 --- a/xen-hvm.c +++ b/xen-hvm.c @@ -345,11 +345,12 @@ go_physmap: unsigned long idx = pfn + i; xen_pfn_t gpfn = start_gpfn + i; + /* In Xen 4.6 rc is -1 and errno contains the error value. */ rc = xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn); if (rc) { DPRINTF("add_to_physmap MFN %"PRI_xen_pfn" to PFN %" - PRI_xen_pfn" failed: %d\n", idx, gpfn, rc); - return -rc; + PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, errno); + return rc == -1 ? -errno : -rc; } } @@ -422,11 +423,12 @@ static int xen_remove_from_physmap(XenIOState *state, xen_pfn_t idx = start_addr + i; xen_pfn_t gpfn = phys_offset + i; + /* In Xen 4.6 rc is -1 and errno contains the error value. */ rc = xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn); if (rc) { fprintf(stderr, "add_to_physmap MFN %"PRI_xen_pfn" to PFN %" - PRI_xen_pfn" failed: %d\n", idx, gpfn, rc); - return -rc; + PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, errno); + return rc == -1 ? -errno : -rc; } }
In Xen 4.6 commit cd2f100f0f61b3f333d52d1737dd73f02daee592 "libxc: Fix do_memory_op to return negative value on errors" made the libxc API less odd-ball: On errors, return value is -1 and error code is in errno. On success the return value is either 0 or an positive value. Since we could be running with an old toolstack in which the Exx value is in rc or the newer, we print both and return the -EXX depending on rc == -1 condition. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- xen-hvm.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)