Re: [PATCH] xl: Fix CHK_ERRNO

[Andrew Cooper <andrew.cooper3@citrix.com>]

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;
>