From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] xl: Fix CHK_ERRNO Date: Tue, 10 Dec 2013 15:16:39 +0000 Message-ID: <52A73057.8050802@citrix.com> References: <1386600504-10658-1-git-send-email-andrew.cooper3@citrix.com> <1386600901.7812.9.camel@kazak.uk.xensource.com> <52A5DA1C.9090705@citrix.com> <21159.12194.684886.874446@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <21159.12194.684886.874446@mariner.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: George Dunlap , Ian Campbell , Xen-devel List-Id: xen-devel@lists.xenproject.org On 10/12/13 15:13, Ian Jackson wrote: > Andrew Cooper writes ("Re: [PATCH] xl: Fix CHK_ERRNO"): >> On 09/12/13 14:55, Ian Campbell wrote: >>> [Andrew Cooper:] >>>> Split the macro into two; CHK_ERRNO() for calls which return -1 >>>> and set errno on error, and CHK_POSERRNO() for calls which return >>>> a positive errno. > This is a bit confusing. Why do you write "a _positive_ errno" > (emph. mine) ? errno values are always positive. In the libxl LOG* > macros we call a style where an errno value is passed explicitly > "ERRNOVAL". > > You propose: > > #define CHK_POSERRNO( call ) ({ \ > int chk_errno = (call); \ > if (chk_errno > 0) { \ > fprintf(stderr,"xl: fatal error: %s:%d: %s: %s\n", \ > __FILE__,__LINE__, strerror(chk_errno), #call); \ > exit(-ERROR_FAIL); \ > } \ > }) > > This is what I would call CHK_ERRNOVAL. (But I think it should > abort() if the returned value is negative, not treat it as success!) > >>> Would be better to call POSERRNO LIBXLERR or something, rather than >>> accidentally imply that it was related to "errno" somehow, I think. > I think there should be a CHK_LIBXL or something too, but that's not > needed right now because all the CHK_* call sites are either > (return -1, set errno) or (return errno value). > > I think the former macro would better be called CHK_SYSCALL, because > it's the system call return convention. CHK_ERRNO would do. > > Ian. In v2 of the patch, CHK_POSERRNO was renamed to CHK_LIBXLERR, but I can certainly extend it to abort() if negative. I can also rename CHK_ERRNO to CHK_SYSCALL which does make it somewhat more descriptive. ~Andrew