Re: [Qemu-devel] [PATCH] sanitize ->bdrv_get_buffer / ->bdrv_put_buffer calling conventions

[Christoph Hellwig <hch@lst.de>]

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);