Re: [Qemu-devel] [PATCH v2 16/16] common: Convert conditional compilation of debug printfs to regular ifs

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2534 bytes --]

On 05/13/2014 01:02 AM, Marc Marí wrote:
> Modify debug macros to have the same format through the codebase and use regular
> ifs instead of ifdef.
> 
> As the debug printf is always put in code, some casting had to be added to avoid
> warnings treated as errors at compile time.
> 
> Signed-off-by: Marc Marí <marc.mari.barcelo@gmail.com>
> ---
>  include/qemu-common.h |    7 +++++++
>  migration-rdma.c      |   32 ++++++++++++++------------------
>  page_cache.c          |   10 ++++++----
>  3 files changed, 27 insertions(+), 22 deletions(-)
> 
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index 3f3fd60..3593bdc 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -463,3 +463,10 @@ int parse_debug_env(const char *name, int max, int initial);
>  const char *qemu_ether_ntoa(const MACAddr *mac);
>  
>  #endif
> +
> +#define QEMU_DPRINTF(cond,pfx,fmt,...) \
> +  do { \
> +    if (cond) { \
> +      fprintf(stderr, pfx": %s:"fmt, __func__, ## __VA_ARGS__); \

Another instance of using the gcc extension ##__VA_ARGS__, when you
could be portable to all C99 compilers by just subsuming fmt into the '...'.

Spacing doesn't match normal style; need space after comma, and use
indentation of 4 spaces not 2.  It should be:

#define QEMU_DPRINTF(cond, pfx, fmt, ...) \
    do { \
        if (cond) { \
......

I'm not sure whether the code base has a definite preference towards
lining up \ continuations in the same column.

I agree with the other review comments that this hunk needs to be first
in the series - each patch must pass 'make' on its own, but your series
as sent fails to compile earlier patches until this hunk is in.


>   */
>  #define ERROR(errp, fmt, ...) \
> -    do { \
> -        fprintf(stderr, "RDMA ERROR: " fmt "\n", ## __VA_ARGS__); \
> -        if (errp && (*(errp) == NULL)) { \
> -            error_setg(errp, "RDMA ERROR: " fmt, ## __VA_ARGS__); \
> -        } \
> -    } while (0)
> +    QEMU_DPRINTF(1, "RDMA ERROR", fmt"\n", ## __VA_ARGS__); \
> +    do { if (errp && (*(errp) == NULL)) { error_setg(errp, "RDMA ERROR: " fmt, ## __VA_ARGS__); } } while (0)

Once again, you broke a one-statement macro into multiple statements.
Fix the placement of "do {".

Pre-existing, but you might as well simplify the code to drop redundant
() and comparison to NULL:

if (errp && !*errp) {

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]