The macro CHK_ERRNO() was being used to check two different error schemes, and
succeeded at neither.
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.
In both cases, ensure that strerror() now gets called with the error integer.
Coverity ID: 1055570 1090374 1130516
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
tools/libxl/xl_cmdimpl.c | 39 ++++++++++++++++++++++++++-------------
1 file changed, 26 insertions(+), 13 deletions(-)
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 4977a53..4709a0f 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -41,15 +41,25 @@
#include "libxlutil.h"
#include "xl.h"
-#define CHK_ERRNO( call ) ({ \
+/* For calls which return positive errno on failure */
+#define CHK_POSERRNO( call ) ({ \
int chk_errno = (call); \
- if (chk_errno < 0) {
\
+ if (chk_errno > 0) { \
fprintf(stderr,"xl: fatal error: %s:%d: %s: %s\n",
\
__FILE__,__LINE__, strerror(chk_errno), #call); \
exit(-ERROR_FAIL); \
} \
})
+/* For calls which return -1 and set errno on failure */
+#define CHK_ERRNO( call ) ({ \
+ if ((call) == -1) { \
+ fprintf(stderr,"xl: fatal error: %s:%d: %s: %s\n",
\
+ __FILE__,__LINE__, strerror(errno), #call); \
+ exit(-ERROR_FAIL); \
+ } \
+ })
+
#define MUST( call ) ({ \
int must_rc = (call); \
if (must_rc < 0) {
\
@@ -1982,8 +1992,9 @@ static uint32_t create_domain(struct domain_create
*dom_info)
if (rc) return rc;
}
- CHK_ERRNO( libxl_read_exactly(ctx, restore_fd, &hdr,
- sizeof(hdr), restore_source, "header") );
+ CHK_POSERRNO( libxl_read_exactly(
+ ctx, restore_fd, &hdr, sizeof(hdr),
+ restore_source, "header") );
if (memcmp(hdr.magic, savefileheader_magic, sizeof(hdr.magic))) {
fprintf(stderr, "File has wrong magic number -"
" corrupt or for a different tool?\n");
@@ -2008,8 +2019,10 @@ static uint32_t create_domain(struct domain_create
*dom_info)
}
if (hdr.optional_data_len) {
optdata_begin = xmalloc(hdr.optional_data_len);
- CHK_ERRNO( libxl_read_exactly(ctx, restore_fd, optdata_begin,
- hdr.optional_data_len, restore_source, "optdata")
);
+ CHK_POSERRNO( libxl_read_exactly(
+ ctx, restore_fd, optdata_begin,
+ hdr.optional_data_len, restore_source,
+ "optdata") );
}
#define OPTDATA_LEFT (hdr.optional_data_len - (optdata_here - optdata_begin))
@@ -3337,9 +3350,9 @@ static void save_domain_core_writeconfig(int fd, const
char *source,
/* that''s the optional data */
- CHK_ERRNO( libxl_write_exactly(ctx, fd,
+ CHK_POSERRNO( libxl_write_exactly(ctx, fd,
&hdr, sizeof(hdr), source, "header") );
- CHK_ERRNO( libxl_write_exactly(ctx, fd,
+ CHK_POSERRNO( libxl_write_exactly(ctx, fd,
optdata_begin, hdr.optional_data_len, source, "header") );
fprintf(stderr, "Saving to %s new xl format (info"
@@ -3695,11 +3708,11 @@ static void migrate_receive(int debug, int daemonize,
int monitor,
fprintf(stderr, "migration target: Ready to receive domain.\n");
- CHK_ERRNO( libxl_write_exactly(ctx, send_fd,
- migrate_receiver_banner,
- sizeof(migrate_receiver_banner)-1,
- "migration ack stream",
- "banner") );
+ CHK_POSERRNO( libxl_write_exactly(ctx, send_fd,
+ migrate_receiver_banner,
+ sizeof(migrate_receiver_banner)-1,
+ "migration ack stream",
+ "banner") );
memset(&dom_info, 0, sizeof(dom_info));
dom_info.debug = debug;
--
1.7.10.4
On Mon, 2013-12-09 at 14:48 +0000, Andrew Cooper wrote:> The macro CHK_ERRNO() was being used to check two different error schemes, and > succeeded at neither. > > 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.Would be better to call POSERRNO LIBXLERR or something, rather than accidentally imply that it was related to "errno" somehow, I think.> In both cases, ensure that strerror() now gets called with the error integer. > > Coverity ID: 1055570 1090374 1130516 > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > CC: Ian Campbell <Ian.Campbell@citrix.com> > CC: Ian Jackson <Ian.Jackson@eu.citrix.com> > CC: George Dunlap <george.dunlap@eu.citrix.com> > --- > tools/libxl/xl_cmdimpl.c | 39 ++++++++++++++++++++++++++------------- > 1 file changed, 26 insertions(+), 13 deletions(-) > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > index 4977a53..4709a0f 100644 > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -41,15 +41,25 @@ > #include "libxlutil.h" > #include "xl.h" > > -#define CHK_ERRNO( call ) ({ \ > +/* For calls which return positive errno on failure */ > +#define CHK_POSERRNO( call ) ({ \ > int chk_errno = (call); \ > - if (chk_errno < 0) { \ > + if (chk_errno > 0) { \ > fprintf(stderr,"xl: fatal error: %s:%d: %s: %s\n", \ > __FILE__,__LINE__, strerror(chk_errno), #call); \ > exit(-ERROR_FAIL); \ > } \ > }) > > +/* For calls which return -1 and set errno on failure */ > +#define CHK_ERRNO( call ) ({ \ > + if ((call) == -1) { \ > + fprintf(stderr,"xl: fatal error: %s:%d: %s: %s\n", \ > + __FILE__,__LINE__, strerror(errno), #call); \ > + exit(-ERROR_FAIL); \ > + } \ > + }) > + > #define MUST( call ) ({ \ > int must_rc = (call); \ > if (must_rc < 0) { \ > @@ -1982,8 +1992,9 @@ static uint32_t create_domain(struct domain_create *dom_info) > if (rc) return rc; > } > > - CHK_ERRNO( libxl_read_exactly(ctx, restore_fd, &hdr, > - sizeof(hdr), restore_source, "header") ); > + CHK_POSERRNO( libxl_read_exactly( > + ctx, restore_fd, &hdr, sizeof(hdr), > + restore_source, "header") ); > if (memcmp(hdr.magic, savefileheader_magic, sizeof(hdr.magic))) { > fprintf(stderr, "File has wrong magic number -" > " corrupt or for a different tool?\n"); > @@ -2008,8 +2019,10 @@ static uint32_t create_domain(struct domain_create *dom_info) > } > if (hdr.optional_data_len) { > optdata_begin = xmalloc(hdr.optional_data_len); > - CHK_ERRNO( libxl_read_exactly(ctx, restore_fd, optdata_begin, > - hdr.optional_data_len, restore_source, "optdata") ); > + CHK_POSERRNO( libxl_read_exactly( > + ctx, restore_fd, optdata_begin, > + hdr.optional_data_len, restore_source, > + "optdata") ); > } > > #define OPTDATA_LEFT (hdr.optional_data_len - (optdata_here - optdata_begin)) > @@ -3337,9 +3350,9 @@ static void save_domain_core_writeconfig(int fd, const char *source, > > /* that''s the optional data */ > > - CHK_ERRNO( libxl_write_exactly(ctx, fd, > + CHK_POSERRNO( libxl_write_exactly(ctx, fd, > &hdr, sizeof(hdr), source, "header") ); > - CHK_ERRNO( libxl_write_exactly(ctx, fd, > + CHK_POSERRNO( libxl_write_exactly(ctx, fd, > optdata_begin, hdr.optional_data_len, source, "header") ); > > fprintf(stderr, "Saving to %s new xl format (info" > @@ -3695,11 +3708,11 @@ static void migrate_receive(int debug, int daemonize, int monitor, > > fprintf(stderr, "migration target: Ready to receive domain.\n"); > > - CHK_ERRNO( libxl_write_exactly(ctx, send_fd, > - migrate_receiver_banner, > - sizeof(migrate_receiver_banner)-1, > - "migration ack stream", > - "banner") ); > + CHK_POSERRNO( libxl_write_exactly(ctx, send_fd, > + migrate_receiver_banner, > + sizeof(migrate_receiver_banner)-1, > + "migration ack stream", > + "banner") ); > > memset(&dom_info, 0, sizeof(dom_info)); > dom_info.debug = debug;
On 09/12/13 14:55, Ian Campbell wrote:> On Mon, 2013-12-09 at 14:48 +0000, Andrew Cooper wrote: >> The macro CHK_ERRNO() was being used to check two different error schemes, and >> succeeded at neither. >> >> 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. > Would be better to call POSERRNO LIBXLERR or something, rather than > accidentally imply that it was related to "errno" somehow, I think.That sounds like a much better name. POSERRNO was chosen as "the best I could think of". ~Andrew> >> In both cases, ensure that strerror() now gets called with the error integer. >> >> Coverity ID: 1055570 1090374 1130516 >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> CC: Ian Campbell <Ian.Campbell@citrix.com> >> CC: Ian Jackson <Ian.Jackson@eu.citrix.com> >> CC: George Dunlap <george.dunlap@eu.citrix.com> >> --- >> tools/libxl/xl_cmdimpl.c | 39 ++++++++++++++++++++++++++------------- >> 1 file changed, 26 insertions(+), 13 deletions(-) >> >> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c >> index 4977a53..4709a0f 100644 >> --- a/tools/libxl/xl_cmdimpl.c >> +++ b/tools/libxl/xl_cmdimpl.c >> @@ -41,15 +41,25 @@ >> #include "libxlutil.h" >> #include "xl.h" >> >> -#define CHK_ERRNO( call ) ({ \ >> +/* For calls which return positive errno on failure */ >> +#define CHK_POSERRNO( call ) ({ \ >> int chk_errno = (call); \ >> - if (chk_errno < 0) { \ >> + if (chk_errno > 0) { \ >> fprintf(stderr,"xl: fatal error: %s:%d: %s: %s\n", \ >> __FILE__,__LINE__, strerror(chk_errno), #call); \ >> exit(-ERROR_FAIL); \ >> } \ >> }) >> >> +/* For calls which return -1 and set errno on failure */ >> +#define CHK_ERRNO( call ) ({ \ >> + if ((call) == -1) { \ >> + fprintf(stderr,"xl: fatal error: %s:%d: %s: %s\n", \ >> + __FILE__,__LINE__, strerror(errno), #call); \ >> + exit(-ERROR_FAIL); \ >> + } \ >> + }) >> + >> #define MUST( call ) ({ \ >> int must_rc = (call); \ >> if (must_rc < 0) { \ >> @@ -1982,8 +1992,9 @@ static uint32_t create_domain(struct domain_create *dom_info) >> if (rc) return rc; >> } >> >> - CHK_ERRNO( libxl_read_exactly(ctx, restore_fd, &hdr, >> - sizeof(hdr), restore_source, "header") ); >> + CHK_POSERRNO( libxl_read_exactly( >> + ctx, restore_fd, &hdr, sizeof(hdr), >> + restore_source, "header") ); >> if (memcmp(hdr.magic, savefileheader_magic, sizeof(hdr.magic))) { >> fprintf(stderr, "File has wrong magic number -" >> " corrupt or for a different tool?\n"); >> @@ -2008,8 +2019,10 @@ static uint32_t create_domain(struct domain_create *dom_info) >> } >> if (hdr.optional_data_len) { >> optdata_begin = xmalloc(hdr.optional_data_len); >> - CHK_ERRNO( libxl_read_exactly(ctx, restore_fd, optdata_begin, >> - hdr.optional_data_len, restore_source, "optdata") ); >> + CHK_POSERRNO( libxl_read_exactly( >> + ctx, restore_fd, optdata_begin, >> + hdr.optional_data_len, restore_source, >> + "optdata") ); >> } >> >> #define OPTDATA_LEFT (hdr.optional_data_len - (optdata_here - optdata_begin)) >> @@ -3337,9 +3350,9 @@ static void save_domain_core_writeconfig(int fd, const char *source, >> >> /* that''s the optional data */ >> >> - CHK_ERRNO( libxl_write_exactly(ctx, fd, >> + CHK_POSERRNO( libxl_write_exactly(ctx, fd, >> &hdr, sizeof(hdr), source, "header") ); >> - CHK_ERRNO( libxl_write_exactly(ctx, fd, >> + CHK_POSERRNO( libxl_write_exactly(ctx, fd, >> optdata_begin, hdr.optional_data_len, source, "header") ); >> >> fprintf(stderr, "Saving to %s new xl format (info" >> @@ -3695,11 +3708,11 @@ static void migrate_receive(int debug, int daemonize, int monitor, >> >> fprintf(stderr, "migration target: Ready to receive domain.\n"); >> >> - CHK_ERRNO( libxl_write_exactly(ctx, send_fd, >> - migrate_receiver_banner, >> - sizeof(migrate_receiver_banner)-1, >> - "migration ack stream", >> - "banner") ); >> + CHK_POSERRNO( libxl_write_exactly(ctx, send_fd, >> + migrate_receiver_banner, >> + sizeof(migrate_receiver_banner)-1, >> + "migration ack stream", >> + "banner") ); >> >> memset(&dom_info, 0, sizeof(dom_info)); >> dom_info.debug = debug; >
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.
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
Andrew Cooper writes ("Re: [PATCH] xl: Fix
CHK_ERRNO"):> On 10/12/13 15:13, Ian Jackson wrote:
> > 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);
\
> > }
\
> > })
...> In v2 of the patch, CHK_POSERRNO was renamed to CHK_LIBXLERR, but I can
> certainly extend it to abort() if negative.
But this has no connection to libxl error values. The error codes
checked for here are actually errno values. So why are you proposing
to call it CHK_LIBXLERR ?
CHK_LIBXLERR would be for something that returned a libxl error value
on error (and a nonnegative integer, perhaps 0, on success).
I think Ian Campbell may have been misled by the calls to
libxl_read_exactly. These functions do not return libxl error codes,
but errno values, and this is stated in their doc comment.
Ian.
On Tue, 2013-12-10 at 15:21 +0000, Ian Jackson wrote:> > I think Ian Campbell may have been misled by the calls to > libxl_read_exactly.Indeed I was.> These functions do not return libxl error codes, > but errno values, and this is stated in their doc comment.Which doesn''t show up in "git grep CHKERRNO" ;-) Ian.