Re: [Qemu-devel] [PATCH 2/5] add basic backup support to block driver

[Stefan Hajnoczi <stefanha@gmail.com>]

On Wed, Nov 21, 2012 at 10:01:01AM +0100, Dietmar Maurer wrote:
Two comments from skimming the code, not a full review.

> +/* #define DEBUG_BACKUP */
> +
> +#ifdef DEBUG_BACKUP
> +#define DPRINTF(fmt, ...) \
> +    do { printf("backup: " fmt, ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) \
> +    do { } while (0)
> +#endif

Please don't #define debug macros like this.  It hides build breakage
because the debug code is #ifdef'd out.

Either use docs/tracing.txt or do the following so the code always gets
parsed by the compiler:

#define DEBUG_BACKUP 0
#define DPRINTF(fmt, ...) \
    do { \
        if (DEBUG_BACKUP) { \
            printf("backup: " fmt, ## __VA_ARGS__); \
	} \
    } while (0)

> +BackupInfo *
> +bdrv_backup_init(BlockDriverState *bs, BackupDumpFunc *backup_dump_cb,
> +                 BlockDriverCompletionFunc *backup_complete_cb,
> +                 void *opaque)
> +{
> +    assert(bs);
> +    assert(backup_dump_cb);
> +    assert(backup_complete_cb);
> +
> +    if (bs->backup_info) {
> +        DPRINTF("bdrv_backup_init already initialized %s\n",
> +                bdrv_get_device_name(bs));
> +        return NULL;
> +    }
> +
> +    BackupInfo *info = g_malloc0(sizeof(BackupInfo));
> +    int64_t bitmap_size;
> +    const char *devname = bdrv_get_device_name(bs);
> +
> +    if (!devname || !devname[0]) {
> +        return NULL;
> +    }
> +
> +    DPRINTF("bdrv_backup_init %s\n", bdrv_get_device_name(bs));
> +
> +    bitmap_size = bs->total_sectors +
> +        BACKUP_BLOCKS_PER_CLUSTER * BITS_PER_LONG - 1;
> +    bitmap_size /= BACKUP_BLOCKS_PER_CLUSTER * BITS_PER_LONG;
> +
> +    info->backup_dump_cb = backup_dump_cb;
> +    info->backup_complete_cb = backup_complete_cb;
> +    info->opaque = opaque;
> +    info->bitmap_size = bitmap_size;
> +    info->bitmap = g_new0(unsigned long, bitmap_size);
> +
> +    Error *errp;
> +    BackupBlockJob *job = block_job_create(&backup_job_type, bs, 0,
> +                                           backup_job_cleanup_cb, bs, &errp);

Is there a 1:1 relationship between BackupInfo and BackupBlockJob?  Then
it would be nicer to move BlockupInfo fields into BackupBlockJob (which
is empty right now).  Then you also don't need to add fields to
BlockDriverState because you know that if your blockjob is running you
can access it via bs->job, if necessary.  You can then drop BackupInfo
and any memory management code for it.