[Qemu-devel] Re: [PATCH] bdrv_flush error handling

[Ian Jackson]

On the 20th of February I wrote:
> bdrv_flush is declared to return void, but this is wrong because it
> means that the implementations have nowhere to report their errors.
> Indeed, the implementations generally ignore errors.
>
> This patch corrects this by making it return int (implicitly, either 0
> or -errno, as for other similar functions).  All of the
> implementations and callers are adjusted too.

There was some discussion which I think concluded that this change was
good but the patch doesn't seem to have been applied.

Ian.

diff -r b5fea3aeb04b tools/ioemu/block-qcow.c
--- a/tools/ioemu/block-qcow.c  Fri Mar 28 14:12:33 2008 +0000
+++ b/tools/ioemu/block-qcow.c  Fri Mar 28 16:58:09 2008 +0000
@@ -934,10 +934,10 @@ static int qcow_write_compressed(BlockDr
     return 0;
 }
 
-static void qcow_flush(BlockDriverState *bs)
-{
-    BDRVQcowState *s = bs->opaque;
-    bdrv_flush(s->hd);
+static int qcow_flush(BlockDriverState *bs)
+{
+    BDRVQcowState *s = bs->opaque;
+    return bdrv_flush(s->hd);
 }
 
 static int qcow_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
diff -r b5fea3aeb04b tools/ioemu/block-qcow2.c
--- a/tools/ioemu/block-qcow2.c Fri Mar 28 14:12:33 2008 +0000
+++ b/tools/ioemu/block-qcow2.c Fri Mar 28 16:58:10 2008 +0000
@@ -1235,10 +1235,10 @@ static int qcow_write_compressed(BlockDr
     return 0;
 }
 
-static void qcow_flush(BlockDriverState *bs)
-{
-    BDRVQcowState *s = bs->opaque;
-    bdrv_flush(s->hd);
+static int qcow_flush(BlockDriverState *bs)
+{
+    BDRVQcowState *s = bs->opaque;
+    return bdrv_flush(s->hd);
 }
 
 static int qcow_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
diff -r b5fea3aeb04b tools/ioemu/block-raw.c
--- a/tools/ioemu/block-raw.c   Fri Mar 28 14:12:33 2008 +0000
+++ b/tools/ioemu/block-raw.c   Fri Mar 28 16:58:12 2008 +0000
@@ -615,10 +615,12 @@ static int raw_create(const char *filena
     return 0;
 }
 
-static void raw_flush(BlockDriverState *bs)
-{
-    BDRVRawState *s = bs->opaque;
-    fsync(s->fd);
+static int raw_flush(BlockDriverState *bs)
+{
+    BDRVRawState *s = bs->opaque;
+    if (fsync(s->fd))
+        return errno;
+    return 0;
 }
 
 BlockDriver bdrv_raw = {
diff -r b5fea3aeb04b tools/ioemu/block-vmdk.c
--- a/tools/ioemu/block-vmdk.c  Fri Mar 28 14:12:33 2008 +0000
+++ b/tools/ioemu/block-vmdk.c  Fri Mar 28 16:58:13 2008 +0000
@@ -734,10 +734,10 @@ static void vmdk_close(BlockDriverState 
     vmdk_parent_close(s->hd);
 }
 
-static void vmdk_flush(BlockDriverState *bs)
-{
-    BDRVVmdkState *s = bs->opaque;
-    bdrv_flush(s->hd);
+static int vmdk_flush(BlockDriverState *bs)
+{
+    BDRVVmdkState *s = bs->opaque;
+    return bdrv_flush(s->hd);
 }
 
 BlockDriver bdrv_vmdk = {
diff -r b5fea3aeb04b tools/ioemu/block.c
--- a/tools/ioemu/block.c       Fri Mar 28 14:12:33 2008 +0000
+++ b/tools/ioemu/block.c       Fri Mar 28 17:00:28 2008 +0000
@@ -889,12 +889,14 @@ const char *bdrv_get_device_name(BlockDr
     return bs->device_name;
 }
 
-void bdrv_flush(BlockDriverState *bs)
-{
-    if (bs->drv->bdrv_flush)
-        bs->drv->bdrv_flush(bs);
-    if (bs->backing_hd)
-        bdrv_flush(bs->backing_hd);
+int bdrv_flush(BlockDriverState *bs)
+{
+    int ret = 0;
+    if (bs->drv->bdrv_flush) 
+        ret = bs->drv->bdrv_flush(bs);
+    if (!ret && bs->backing_hd)
+        ret = bdrv_flush(bs->backing_hd);
+    return ret;
 }
 
 void bdrv_info(void)
@@ -1232,8 +1234,9 @@ static BlockDriverAIOCB *bdrv_aio_flush_
 static BlockDriverAIOCB *bdrv_aio_flush_em(BlockDriverState *bs,
         BlockDriverCompletionFunc *cb, void *opaque)
 {
-    bdrv_flush(bs);
-    cb(opaque, 0);
+    int ret;
+    ret = bdrv_flush(bs);
+    cb(opaque, ret);
     return NULL;
 }
 
diff -r b5fea3aeb04b tools/ioemu/block_int.h
--- a/tools/ioemu/block_int.h   Fri Mar 28 14:12:33 2008 +0000
+++ b/tools/ioemu/block_int.h   Fri Mar 28 16:58:15 2008 +0000
@@ -36,7 +36,7 @@ struct BlockDriver {
     void (*bdrv_close)(BlockDriverState *bs);
     int (*bdrv_create)(const char *filename, int64_t total_sectors, 
                        const char *backing_file, int flags);
-    void (*bdrv_flush)(BlockDriverState *bs);
+    int (*bdrv_flush)(BlockDriverState *bs);
     int (*bdrv_is_allocated)(BlockDriverState *bs, int64_t sector_num,
                              int nb_sectors, int *pnum);
     int (*bdrv_set_key)(BlockDriverState *bs, const char *key);
diff -r b5fea3aeb04b tools/ioemu/hw/ide.c
--- a/tools/ioemu/hw/ide.c      Fri Mar 28 14:12:33 2008 +0000
+++ b/tools/ioemu/hw/ide.c      Fri Mar 28 16:58:15 2008 +0000
@@ -1786,6 +1786,7 @@ static void ide_ioport_write(void *opaqu
     IDEState *s;
     int unit, n;
     int lba48 = 0;
+    int ret;
 
 #ifdef DEBUG_IDE
     printf("IDE: write addr=0x%x val=0x%02x\n", addr, val);
diff -r b5fea3aeb04b tools/ioemu/hw/scsi-disk.c
--- a/tools/ioemu/hw/scsi-disk.c        Fri Mar 28 14:12:33 2008 +0000
+++ b/tools/ioemu/hw/scsi-disk.c        Fri Mar 28 16:59:42 2008 +0000
@@ -291,6 +291,7 @@ int32_t scsi_send_command(SCSIDevice *s,
     uint8_t command;
     uint8_t *outbuf;
     SCSIRequest *r;
+    int ret;
 
     command = buf[0];
     r = scsi_find_request(s, tag);
@@ -496,7 +497,12 @@ int32_t scsi_send_command(SCSIDevice *s,
         break;
     case 0x35:
         DPRINTF("Syncronise cache (sector %d, count %d)\n", lba, len);
-        bdrv_flush(s->bdrv);
+        ret = bdrv_flush(s->bdrv);
+        if (ret) {
+            DPRINTF("IO error on bdrv_flush\n");
+            scsi_command_complete(r, SENSE_HARDWARE_ERROR);
+            return 0;
+        }
         break;
     case 0x43:
         {
diff -r b5fea3aeb04b tools/ioemu/vl.h
--- a/tools/ioemu/vl.h  Fri Mar 28 14:12:33 2008 +0000
+++ b/tools/ioemu/vl.h  Fri Mar 28 16:58:15 2008 +0000
@@ -664,7 +664,7 @@ void qemu_aio_wait_end(void);
 void qemu_aio_wait_end(void);
 
 /* Ensure contents are flushed to disk.  */
-void bdrv_flush(BlockDriverState *bs);
+int bdrv_flush(BlockDriverState *bs);
 
 #define BDRV_TYPE_HD     0
 #define BDRV_TYPE_CDROM  1