From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36370) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZAfFG-0002iE-O2 for qemu-devel@nongnu.org; Thu, 02 Jul 2015 10:13:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZAfFA-00039r-GE for qemu-devel@nongnu.org; Thu, 02 Jul 2015 10:13:42 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:48728) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZAfFA-00039d-9v for qemu-devel@nongnu.org; Thu, 02 Jul 2015 10:13:36 -0400 Date: Thu, 2 Jul 2015 10:13:28 -0400 From: Konrad Rzeszutek Wilk Message-ID: <20150702141328.GA4269@l.oracle.com> References: <1435605913-23826-1-git-send-email-konrad.wilk@oracle.com> <1435605913-23826-5-git-send-email-konrad.wilk@oracle.com> <20150701182209.GA22498@l.oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH RFC 4/6] xen: Print and use errno where applicable. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefano Stabellini Cc: xen-devel@lists.xenproject.org, qemu-devel@nongnu.org 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 > > > > --- > > > > 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 > > > > > >