Re: [Patch v2] xl: Fix CHK_ERRNO

[George Dunlap <george.dunlap@eu.citrix.com>]

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;