[Qemu-devel] [PATCH] bdrv_flush error handling

[Ian Jackson]

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.


While looking at this I was surprised to see this:

    static void scsi_write_complete(void * opaque, int ret)
    ...
        if (ret) {
            fprintf(stderr, "scsi-disc: IO write error\n");
            exit(1);
        }

Surely that is overkill ?


Also, in block-raw-posix.c, raw_pwrite et al seem to return -1 on
error (the return value from write) whereas the other block read/write
methods return errno values.  This is a mistake, surely ?  -1 would be
-EPERM.  If any of the callers did anything with these return values
you'd get incorrect error indications.


Finally, it would perhaps be best if the block device emulators wrote
to the qemu console to complain if they give write errors.  Otherwise
the errno value and other important information will be lost, which
makes debugging hard.


Thanks,
Ian.

diff --git a/block-qcow.c b/block-qcow.c
index 6ec625d..d53d1d0 100644
--- a/block-qcow.c
+++ b/block-qcow.c
@@ -879,10 +879,10 @@ static int qcow_write_compressed(BlockDriverState *bs, 
int64_t sector_num,
     return 0;
 }
 
-static void qcow_flush(BlockDriverState *bs)
+static int qcow_flush(BlockDriverState *bs)
 {
     BDRVQcowState *s = bs->opaque;
-    bdrv_flush(s->hd);
+    return bdrv_flush(s->hd);
 }
 
 static int qcow_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
diff --git a/block-qcow2.c b/block-qcow2.c
index 577210b..9bad090 100644
--- a/block-qcow2.c
+++ b/block-qcow2.c
@@ -1228,10 +1228,10 @@ static int qcow_write_compressed(BlockDriverState *bs, 
int64_t sector_num,
     return 0;
 }
 
-static void qcow_flush(BlockDriverState *bs)
+static int qcow_flush(BlockDriverState *bs)
 {
     BDRVQcowState *s = bs->opaque;
-    bdrv_flush(s->hd);
+    return bdrv_flush(s->hd);
 }
 
 static int qcow_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
diff --git a/block-raw-posix.c b/block-raw-posix.c
index 28aaf02..45d0a93 100644
--- a/block-raw-posix.c
+++ b/block-raw-posix.c
@@ -539,10 +539,12 @@ static int raw_create(const char *filename, int64_t 
total_size,
     return 0;
 }
 
-static void raw_flush(BlockDriverState *bs)
+static int raw_flush(BlockDriverState *bs)
 {
     BDRVRawState *s = bs->opaque;
-    fsync(s->fd);
+    if (fsync(s->fd))
+        return errno;
+    return 0;
 }
 
 BlockDriver bdrv_raw = {
diff --git a/block-raw-win32.c b/block-raw-win32.c
index 43d3f6c..b86c66e 100644
--- a/block-raw-win32.c
+++ b/block-raw-win32.c
@@ -269,10 +269,14 @@ static void raw_aio_cancel(BlockDriverAIOCB *blockacb)
 }
 #endif /* #if 0 */
 
-static void raw_flush(BlockDriverState *bs)
+static int raw_flush(BlockDriverState *bs)
 {
     BDRVRawState *s = bs->opaque;
-    FlushFileBuffers(s->hfile);
+    int ret;
+    ret = FlushFileBuffers(s->hfile);
+    if (ret)
+       return -EIO;
+    return 0;
 }
 
 static void raw_close(BlockDriverState *bs)
diff --git a/block-vmdk.c b/block-vmdk.c
index b6ec89e..4cacc6a 100644
--- a/block-vmdk.c
+++ b/block-vmdk.c
@@ -815,10 +815,10 @@ static void vmdk_close(BlockDriverState *bs)
     vmdk_parent_close(s->hd);
 }
 
-static void vmdk_flush(BlockDriverState *bs)
+static int vmdk_flush(BlockDriverState *bs)
 {
     BDRVVmdkState *s = bs->opaque;
-    bdrv_flush(s->hd);
+    return bdrv_flush(s->hd);
 }
 
 BlockDriver bdrv_vmdk = {
diff --git a/block.c b/block.c
index 0f8ad7b..b1edd22 100644
--- a/block.c
+++ b/block.c
@@ -865,12 +865,14 @@ const char *bdrv_get_device_name(BlockDriverState *bs)
     return bs->device_name;
 }
 
-void bdrv_flush(BlockDriverState *bs)
+int bdrv_flush(BlockDriverState *bs)
 {
-    if (bs->drv->bdrv_flush)
-        bs->drv->bdrv_flush(bs);
-    if (bs->backing_hd)
-        bdrv_flush(bs->backing_hd);
+    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;
 }
 
 #ifndef QEMU_IMG
diff --git a/block.h b/block.h
index b730505..861a65b 100644
--- a/block.h
+++ b/block.h
@@ -98,7 +98,7 @@ void qemu_aio_wait_end(void);
 int qemu_key_check(BlockDriverState *bs, const char *name);
 
 /* 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
diff --git a/block_int.h b/block_int.h
index 137000e..796ce29 100644
--- a/block_int.h
+++ b/block_int.h
@@ -42,7 +42,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 --git a/hw/ide.c b/hw/ide.c
index 370a412..56a1cda 100644
--- a/hw/ide.c
+++ b/hw/ide.c
@@ -1895,6 +1895,7 @@ static void ide_ioport_write(void *opaque, uint32_t addr, 
uint32_t val)
     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);
@@ -2140,8 +2141,10 @@ static void ide_ioport_write(void *opaque, uint32_t 
addr, uint32_t val)
             break;
         case WIN_FLUSH_CACHE:
         case WIN_FLUSH_CACHE_EXT:
-            if (s->bs)
-                bdrv_flush(s->bs);
+            if (s->bs) {
+                ret = bdrv_flush(s->bs);
+               if (ret) goto abort_cmd;
+           }
            s->status = READY_STAT;
             ide_set_irq(s);
             break;
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 44462d4..30cc906 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -293,6 +293,7 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t 
tag,
     uint8_t command;
     uint8_t *outbuf;
     SCSIRequest *r;
+    int ret;
 
     command = buf[0];
     r = scsi_find_request(s, tag);
@@ -620,7 +621,12 @@ static int32_t scsi_send_command(SCSIDevice *d, uint32_t 
tag,
         break;
     case 0x35:
         DPRINTF("Synchronise 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:
         {