Re: [Qemu-devel] [PATCH 2/3] target-i386: Add Intel HAX files

[Stefan Hajnoczi <stefanha@gmail.com>]

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

On Tue, Nov 08, 2016 at 04:39:28PM +0100, Vincent Palatin wrote:

Please run scripts/checkpatch.pl to verify that the code follows the
QEMU coding style.

> +hax_fd hax_host_open_vcpu(int vmid, int vcpuid)
> +{
> +    char *devfs_path = NULL;
> +    hax_fd fd;
> +
> +    devfs_path = hax_vcpu_devfs_string(vmid, vcpuid);
> +    if (!devfs_path) {
> +        fprintf(stderr, "Failed to get the devfs\n");
> +        return -EINVAL;
> +    }
> +
> +    fd = open(devfs_path, O_RDWR);
> +    qemu_vfree(devfs_path);

g_malloc(), g_new(), g_strdup(), etc must be matched with g_free(), not
qemu_vfree().  There are probably other instances of this issue in the
patches.

> +//#define DEBUG_HAX_SLOT
> +
> +#ifdef DEBUG_HAX_SLOT
> +#define DPRINTF(fmt, ...) \
> +    do { fprintf(stdout, fmt, ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) \
> +    do { } while (0)
> +#endif

Please consider using tracing instead of debug printfs.  See docs/tracing.txt.

If you really want to keep macros, please use:

#define DEBUG_HAX_SLOT 0
#define DPRINTF(fmt, ...) \
    do { \
        if (DEBUG_HAX_SLOT) { \
            fprintf(stdout, fmt, ## __VA_ARGS__); \
        } \
    } while (0)

This approach prevents bitrot because it allows the compiler to syntax
check the format string and arguments even when the printf is compiled
out.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]