* [PATCH] xl: Fix CHK_ERRNO
@ 2013-12-09 14:48 Andrew Cooper
2013-12-09 14:55 ` Ian Campbell
0 siblings, 1 reply; 7+ messages in thread
From: Andrew Cooper @ 2013-12-09 14:48 UTC (permalink / raw)
To: Xen-devel; +Cc: George Dunlap, Andrew Cooper, Ian Jackson, Ian Campbell
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] xl: Fix CHK_ERRNO
2013-12-09 14:48 [PATCH] xl: Fix CHK_ERRNO Andrew Cooper
@ 2013-12-09 14:55 ` Ian Campbell
2013-12-09 14:56 ` Andrew Cooper
0 siblings, 1 reply; 7+ messages in thread
From: Ian Campbell @ 2013-12-09 14:55 UTC (permalink / raw)
To: Andrew Cooper; +Cc: George Dunlap, Ian Jackson, Xen-devel
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;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xl: Fix CHK_ERRNO
2013-12-09 14:55 ` Ian Campbell
@ 2013-12-09 14:56 ` Andrew Cooper
2013-12-10 15:13 ` Ian Jackson
0 siblings, 1 reply; 7+ messages in thread
From: Andrew Cooper @ 2013-12-09 14:56 UTC (permalink / raw)
To: Ian Campbell; +Cc: George Dunlap, Ian Jackson, Xen-devel
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;
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xl: Fix CHK_ERRNO
2013-12-09 14:56 ` Andrew Cooper
@ 2013-12-10 15:13 ` Ian Jackson
2013-12-10 15:16 ` Andrew Cooper
0 siblings, 1 reply; 7+ messages in thread
From: Ian Jackson @ 2013-12-10 15:13 UTC (permalink / raw)
To: Andrew Cooper; +Cc: George Dunlap, Ian Campbell, Xen-devel
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xl: Fix CHK_ERRNO
2013-12-10 15:13 ` Ian Jackson
@ 2013-12-10 15:16 ` Andrew Cooper
2013-12-10 15:21 ` Ian Jackson
0 siblings, 1 reply; 7+ messages in thread
From: Andrew Cooper @ 2013-12-10 15:16 UTC (permalink / raw)
To: Ian Jackson; +Cc: George Dunlap, Ian Campbell, Xen-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xl: Fix CHK_ERRNO
2013-12-10 15:16 ` Andrew Cooper
@ 2013-12-10 15:21 ` Ian Jackson
2013-12-10 15:25 ` Ian Campbell
0 siblings, 1 reply; 7+ messages in thread
From: Ian Jackson @ 2013-12-10 15:21 UTC (permalink / raw)
To: Andrew Cooper; +Cc: George Dunlap, Ian Campbell, Xen-devel
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] xl: Fix CHK_ERRNO
2013-12-10 15:21 ` Ian Jackson
@ 2013-12-10 15:25 ` Ian Campbell
0 siblings, 0 replies; 7+ messages in thread
From: Ian Campbell @ 2013-12-10 15:25 UTC (permalink / raw)
To: Ian Jackson; +Cc: George Dunlap, Andrew Cooper, Xen-devel
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-12-10 15:25 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-09 14:48 [PATCH] xl: Fix CHK_ERRNO Andrew Cooper
2013-12-09 14:55 ` Ian Campbell
2013-12-09 14:56 ` Andrew Cooper
2013-12-10 15:13 ` Ian Jackson
2013-12-10 15:16 ` Andrew Cooper
2013-12-10 15:21 ` Ian Jackson
2013-12-10 15:25 ` Ian Campbell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).