[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/5] add basic backup support to block driver
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH 2/5] add basic backup support to block driver |
Date: |
Thu, 22 Nov 2012 12:25:57 +0100 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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.
- [Qemu-devel] [PATCH 1/5] RFC: Efficient VM backup for qemu (v1), Dietmar Maurer, 2012/11/21
- [Qemu-devel] [PATCH 4/5] add backup related monitor commands, Dietmar Maurer, 2012/11/21
- [Qemu-devel] [PATCH 2/5] add basic backup support to block driver, Dietmar Maurer, 2012/11/21
- Re: [Qemu-devel] [PATCH 2/5] add basic backup support to block driver,
Stefan Hajnoczi <=
- [Qemu-devel] [PATCH 5/5] add regression tests for backup, Dietmar Maurer, 2012/11/21
- [Qemu-devel] [PATCH 3/5] introduce new vma archive format, Dietmar Maurer, 2012/11/21
- Re: [Qemu-devel] [PATCH 1/5] RFC: Efficient VM backup for qemu (v1), Kevin Wolf, 2012/11/21