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

[Vincent Palatin]

On Wed, Nov 9, 2016 at 1:30 PM, Stefan Hajnoczi <address@hidden> wrote:
> 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.

My original plan was to import those files unmodified but this ship
has probably long-sailed.
Fixed in the v2 of this patch.

>
>> +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.

Found 2 instances by grepping and fixed them.

>
>> +//#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.

For the v2 of this patch, I will keep the macros and do your approach.
Then I will review all the debug print statements, see what I should
do with them in another patch (convert to tracing / remove).