From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MOXiA-0003jD-Em for qemu-devel@nongnu.org; Wed, 08 Jul 2009 10:01:26 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MOXi5-0003dp-PC for qemu-devel@nongnu.org; Wed, 08 Jul 2009 10:01:25 -0400 Received: from [199.232.76.173] (port=50693 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MOXi4-0003da-Sw for qemu-devel@nongnu.org; Wed, 08 Jul 2009 10:01:20 -0400 Received: from verein.lst.de ([213.95.11.210]:49064) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_3DES_EDE_CBC_SHA1:24) (Exim 4.60) (envelope-from ) id 1MOXi4-0006Qy-2c for qemu-devel@nongnu.org; Wed, 08 Jul 2009 10:01:20 -0400 Date: Wed, 8 Jul 2009 16:01:17 +0200 From: Christoph Hellwig Subject: Re: [Qemu-devel] [PATCH] sanitize ->bdrv_get_buffer / ->bdrv_put_buffer calling conventions Message-ID: <20090708140117.GA24544@lst.de> References: <20090707173401.GA5145@lst.de> <4A54A327.9020501@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4A54A327.9020501@redhat.com> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Christoph Hellwig , qemu-devel@nongnu.org On Wed, Jul 08, 2009 at 03:46:15PM +0200, Kevin Wolf wrote: > Christoph Hellwig schrieb: > > The VM state offset is an concept internal to the image format, it > > doesn't even have to be a traditional linear offset. So change the > > ->bdrv_get_buffer / ->bdrv_put_buffer calling convention to always pass > > a relative offset and add the VM state offset to it internally inside > > the qcow2 code. > > Looks good to me and makes the interface nicer. Maybe we could use the > opportunity to change the function names to something like > bdrv_vmstate_read/write? get/put_buffer is quite unspecific, it could > mean anything. Does this one looks good? Index: qemu/savevm.c =================================================================== --- qemu.orig/savevm.c 2009-07-08 14:01:26.247364595 +0200 +++ qemu/savevm.c 2009-07-08 15:55:24.189365614 +0200 @@ -337,46 +337,28 @@ fail: return NULL; } -typedef struct QEMUFileBdrv -{ - BlockDriverState *bs; - int64_t base_offset; -} QEMUFileBdrv; - static int block_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, int size) { - QEMUFileBdrv *s = opaque; - bdrv_put_buffer(s->bs, buf, s->base_offset + pos, size); + bdrv_save_vmstate(opaque, buf, pos, size); return size; } static int block_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size) { - QEMUFileBdrv *s = opaque; - return bdrv_get_buffer(s->bs, buf, s->base_offset + pos, size); + return bdrv_load_vmstate(opaque, buf, pos, size); } static int bdrv_fclose(void *opaque) { - QEMUFileBdrv *s = opaque; - qemu_free(s); return 0; } -static QEMUFile *qemu_fopen_bdrv(BlockDriverState *bs, int64_t offset, int is_writable) +static QEMUFile *qemu_fopen_bdrv(BlockDriverState *bs, int is_writable) { - QEMUFileBdrv *s; - - s = qemu_mallocz(sizeof(QEMUFileBdrv)); - - s->bs = bs; - s->base_offset = offset; - if (is_writable) - return qemu_fopen_ops(s, block_put_buffer, NULL, bdrv_fclose, NULL, NULL); - - return qemu_fopen_ops(s, NULL, block_get_buffer, bdrv_fclose, NULL, NULL); + return qemu_fopen_ops(bs, block_put_buffer, NULL, bdrv_fclose, NULL, NULL); + return qemu_fopen_ops(bs, NULL, block_get_buffer, bdrv_fclose, NULL, NULL); } QEMUFile *qemu_fopen_ops(void *opaque, QEMUFilePutBufferFunc *put_buffer, @@ -1069,7 +1051,6 @@ void do_savevm(Monitor *mon, const char BlockDriverState *bs, *bs1; QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1; int must_delete, ret, i; - BlockDriverInfo bdi1, *bdi = &bdi1; QEMUFile *f; int saved_vm_running; uint32_t vm_state_size; @@ -1119,14 +1100,8 @@ void do_savevm(Monitor *mon, const char #endif sn->vm_clock_nsec = qemu_get_clock(vm_clock); - if (bdrv_get_info(bs, bdi) < 0 || bdi->vm_state_offset <= 0) { - monitor_printf(mon, "Device %s does not support VM state snapshots\n", - bdrv_get_device_name(bs)); - goto the_end; - } - /* save the VM state */ - f = qemu_fopen_bdrv(bs, bdi->vm_state_offset, 1); + f = qemu_fopen_bdrv(bs, 1); if (!f) { monitor_printf(mon, "Could not open VM state file\n"); goto the_end; @@ -1170,7 +1145,6 @@ void do_savevm(Monitor *mon, const char void do_loadvm(Monitor *mon, const char *name) { BlockDriverState *bs, *bs1; - BlockDriverInfo bdi1, *bdi = &bdi1; QEMUSnapshotInfo sn; QEMUFile *f; int i, ret; @@ -1218,19 +1192,13 @@ void do_loadvm(Monitor *mon, const char } } - if (bdrv_get_info(bs, bdi) < 0 || bdi->vm_state_offset <= 0) { - monitor_printf(mon, "Device %s does not support VM state snapshots\n", - bdrv_get_device_name(bs)); - return; - } - /* Don't even try to load empty VM states */ ret = bdrv_snapshot_find(bs, &sn, name); if ((ret >= 0) && (sn.vm_state_size == 0)) goto the_end; /* restore the VM state */ - f = qemu_fopen_bdrv(bs, bdi->vm_state_offset, 0); + f = qemu_fopen_bdrv(bs, 0); if (!f) { monitor_printf(mon, "Could not open VM state file\n"); goto the_end; Index: qemu/block/qcow2.c =================================================================== --- qemu.orig/block/qcow2.c 2009-07-08 14:01:26.254364718 +0200 +++ qemu/block/qcow2.c 2009-07-08 15:56:01.493321620 +0200 @@ -897,12 +897,16 @@ static void qcow_flush(BlockDriverState bdrv_flush(s->hd); } +static int64_t qcow_vm_state_offset(BDRVQcowState *s) +{ + return (int64_t)s->l1_vm_state_index << (s->cluster_bits + s->l2_bits); +} + static int qcow_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) { BDRVQcowState *s = bs->opaque; bdi->cluster_size = s->cluster_size; - bdi->vm_state_offset = (int64_t)s->l1_vm_state_index << - (s->cluster_bits + s->l2_bits); + bdi->vm_state_offset = qcow_vm_state_offset(s); return 0; } @@ -932,26 +936,28 @@ static void dump_refcounts(BlockDriverSt } #endif -static int qcow_put_buffer(BlockDriverState *bs, const uint8_t *buf, +static int qcow_save_vmstate(BlockDriverState *bs, const uint8_t *buf, int64_t pos, int size) { + BDRVQcowState *s = bs->opaque; int growable = bs->growable; bs->growable = 1; - bdrv_pwrite(bs, pos, buf, size); + bdrv_pwrite(bs, qcow_vm_state_offset(s) + pos, buf, size); bs->growable = growable; return size; } -static int qcow_get_buffer(BlockDriverState *bs, uint8_t *buf, +static int qcow_load_vmstate(BlockDriverState *bs, uint8_t *buf, int64_t pos, int size) { + BDRVQcowState *s = bs->opaque; int growable = bs->growable; int ret; bs->growable = 1; - ret = bdrv_pread(bs, pos, buf, size); + ret = bdrv_pread(bs, qcow_vm_state_offset(s) + pos, buf, size); bs->growable = growable; return ret; @@ -1008,8 +1014,8 @@ static BlockDriver bdrv_qcow2 = { .bdrv_snapshot_list = qcow2_snapshot_list, .bdrv_get_info = qcow_get_info, - .bdrv_put_buffer = qcow_put_buffer, - .bdrv_get_buffer = qcow_get_buffer, + .bdrv_save_vmstate = qcow_save_vmstate, + .bdrv_load_vmstate = qcow_load_vmstate, .create_options = qcow_create_options, .bdrv_check = qcow_check, Index: qemu/block.c =================================================================== --- qemu.orig/block.c 2009-07-08 15:53:02.225364220 +0200 +++ qemu/block.c 2009-07-08 15:55:33.728367200 +0200 @@ -1165,24 +1165,26 @@ int bdrv_get_info(BlockDriverState *bs, return drv->bdrv_get_info(bs, bdi); } -int bdrv_put_buffer(BlockDriverState *bs, const uint8_t *buf, int64_t pos, int size) +int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf, + int64_t pos, int size) { BlockDriver *drv = bs->drv; if (!drv) return -ENOMEDIUM; - if (!drv->bdrv_put_buffer) + if (!drv->bdrv_save_vmstate) return -ENOTSUP; - return drv->bdrv_put_buffer(bs, buf, pos, size); + return drv->bdrv_save_vmstate(bs, buf, pos, size); } -int bdrv_get_buffer(BlockDriverState *bs, uint8_t *buf, int64_t pos, int size) +int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf, + int64_t pos, int size) { BlockDriver *drv = bs->drv; if (!drv) return -ENOMEDIUM; - if (!drv->bdrv_get_buffer) + if (!drv->bdrv_load_vmstate) return -ENOTSUP; - return drv->bdrv_get_buffer(bs, buf, pos, size); + return drv->bdrv_load_vmstate(bs, buf, pos, size); } /**************************************************************/ Index: qemu/block.h =================================================================== --- qemu.orig/block.h 2009-07-08 15:53:02.239364323 +0200 +++ qemu/block.h 2009-07-08 15:54:51.983241775 +0200 @@ -162,9 +162,10 @@ void path_combine(char *dest, int dest_s const char *base_path, const char *filename); -int bdrv_put_buffer(BlockDriverState *bs, const uint8_t *buf, - int64_t pos, int size); +int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf, + int64_t pos, int size); -int bdrv_get_buffer(BlockDriverState *bs, uint8_t *buf, int64_t pos, int size); +int bdrv_load_vmstate(BlockDriverState *bs, uint8_t *buf, + int64_t pos, int size); #endif Index: qemu/block_int.h =================================================================== --- qemu.orig/block_int.h 2009-07-08 15:53:02.254364553 +0200 +++ qemu/block_int.h 2009-07-08 15:55:14.827454532 +0200 @@ -84,10 +84,10 @@ struct BlockDriver { QEMUSnapshotInfo **psn_info); int (*bdrv_get_info)(BlockDriverState *bs, BlockDriverInfo *bdi); - int (*bdrv_put_buffer)(BlockDriverState *bs, const uint8_t *buf, - int64_t pos, int size); - int (*bdrv_get_buffer)(BlockDriverState *bs, uint8_t *buf, - int64_t pos, int size); + int (*bdrv_save_vmstate)(BlockDriverState *bs, const uint8_t *buf, + int64_t pos, int size); + int (*bdrv_load_vmstate)(BlockDriverState *bs, uint8_t *buf, + int64_t pos, int size); /* removable device specific */ int (*bdrv_is_inserted)(BlockDriverState *bs);