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_LIBXLERR() 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>
---
Changes in v2:
* s/CHK_POSERRNO/CHK_LIBXLERR/
---
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..45cb658 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_LIBXLERR( 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_LIBXLERR( 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_LIBXLERR( 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_LIBXLERR( libxl_write_exactly(ctx, fd,
&hdr, sizeof(hdr), source, "header") );
- CHK_ERRNO( libxl_write_exactly(ctx, fd,
+ CHK_LIBXLERR( 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_LIBXLERR( 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 12/09/2013 03:12 PM, 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_LIBXLERR() 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>Clearly a bug fix, if a minor one. It does touch the domain creation as well as save/restore path, but is fairly simple and should also be very well tested between now and the release. Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com>> > --- > > Changes in v2: > * s/CHK_POSERRNO/CHK_LIBXLERR/ > --- > 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..45cb658 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_LIBXLERR( 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_LIBXLERR( 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_LIBXLERR( 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_LIBXLERR( libxl_write_exactly(ctx, fd, > &hdr, sizeof(hdr), source, "header") ); > - CHK_ERRNO( libxl_write_exactly(ctx, fd, > + CHK_LIBXLERR( 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_LIBXLERR( 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;