Re: [Xen-devel] [PATCH 17/28] libxl: gettimeofday doesn't return an errno on failure

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2013-09-26 at 11:11 +1200, Matthew Daley wrote:
> On Thu, Sep 26, 2013 at 12:04 AM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> 
> wrote:
> > On Wed, 2013-09-18 at 15:37 +1200, Matthew Daley wrote:
> >
> > http://pubs.opengroup.org/onlinepubs/9699919799/functions/gettimeofday.html
> > agrees with this but the Linux manpages gettimeofday(2) disagrees:
> >        gettimeofday() and settimeofday() return 0 for success,  or  -1  for
> >        failure (in which case errno is set appropriately).
> >
> > They may just have confused themselves by lumping get in with set, but
> > at least one error code (EFAULT) seems like it could apply to get too.
> 
> I think my lack of description and my use of the "MUST" macro is
> confusing you here. I agree that they can return values signifying
> failure, but indeed they don't return error codes directly, instead
> they return -1 and set errno.
> 
> >
> > Since the spec says it cannot fail I think there's no harm in reporting
> > errno if it does fail.
> >
> > is the CHK_ERRNO macro correct?
> > #define CHK_ERRNO( 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);                                          \
> >         }                                                               \
> >     })
> >
> > It seems to report the reutrn code and not errno, so it will always say
> > -1 won't it?
> >
> > I think the actual coverity error "chk_errno" is passed to a parameter
> > that cannot be negative." stems from this not the lack of errno, because
> > coverity seems to know that strerror cannot take a negative number.
> 
> Right, and this is what I was trying to avoid by changing to using the
> MUST macro, but I suppose the lack of errno reportage is unacceptable
> (and I guess that MUST is meant for internal xl functions that return
> a exit()able return code, not just -1. Really, I think another macro
> is needed that gets errno not from the call's return value, but errno
> directly?

I think the problem is that CHK_ERRNO is actually used to check both
libc functions (which return -1 and set errno) and libxl function (which
return -LIBXL_ERROR), and does neither of them properly:
xl_cmdimpl.c:        CHK_ERRNO( libxl_read_exactly(ctx, restore_f
xl_cmdimpl.c:            CHK_ERRNO( libxl_read_exactly(ctx, resto
xl_cmdimpl.c:        CHK_ERRNO(( logfile = open(fullname, O_WRONL
xl_cmdimpl.c:        CHK_ERRNO(( nullfd = open("/dev/null", O_RDO
xl_cmdimpl.c:        CHK_ERRNO(daemon(0, 1) < 0);
xl_cmdimpl.c:        CHK_ERRNO(asprintf(&config_source, "<domid %
xl_cmdimpl.c:    CHK_ERRNO( libxl_write_exactly(ctx, fd,
xl_cmdimpl.c:    CHK_ERRNO( libxl_write_exactly(ctx, fd,
xl_cmdimpl.c:    CHK_ERRNO( gettimeofday(&waituntil, 0) );
xl_cmdimpl.c:        CHK_ERRNO( gettimeofday(&now, 0) );
xl_cmdimpl.c:    CHK_ERRNO( libxl_write_exactly(ctx, send_fd,

When used with libc functions it takes the return code (always -1 on
error) and calls strerror on it to get whatever errno -1 happens to be,
which is bogus.

When used with libxl functions it takes the libxl error code (which is
not an errno) and passes it to strerror, which is also bogus.

I think we need to change CHK_ERRNO to strerror(errno) and introduce
CHK_LIBXL which does the right thing for libxl error codes and then use
CHK_ERRNO for libc functions and CHK_LIBXL for libxl ones...

Or maybe MUST is already the CHK_LIBXL we want, but in that case using
MUST on libc functions is wrong...

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel