[Qemu-devel] [PATCH] bdrv_flush error handling

[Qemu-devel] [PATCH] bdrv_flush error handling

[Ian Jackson]

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 1160 bytes --]

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.


[-- Attachment #2: check errors in block driver flush --]
[-- Type: text/plain, Size: 5584 bytes --]

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:
         {

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

[Daniel P. Berrange]

On Wed, Feb 20, 2008 at 03:53:46PM +0000, Ian Jackson wrote:
Content-Description: message body text
> 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 ?

Yes, any i/o errors I'd expect to be propagated back up to the guest
OS as the most appropriate IDE / SCSI error code.

> 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.

If by 'qemu console' you mean stderr, then fine, but please don't
spew log messages to the  monitor console, because that'll make it
very hard to interact with reliably from management tools. 

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

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

[Ian Jackson]

Daniel P. Berrange writes ("Re: [Qemu-devel] [PATCH] bdrv_flush error handling"):
> On Wed, Feb 20, 2008 at 03:53:46PM +0000, Ian Jackson wrote:
> > 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.
> 
> If by 'qemu console' you mean stderr, then fine, but please don't
> spew log messages to the  monitor console, because that'll make it
> very hard to interact with reliably from management tools. 

Is it permitted, then, to generally print to qemu's stderr from inside
a device model ?  This seems to be done quite a bit during
initialisation, and quite a bit in various rather less common kinds of
device, but not routinely in the main emulations.

Perhaps it would be better to invent a `logprintf' function to make it
easier to redirect these kind of messages later if that turns out to
be desirable ?

Ian.

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

[Paul Brook]

> > 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.
>
> If by 'qemu console' you mean stderr, then fine, but please don't
> spew log messages to the  monitor console, because that'll make it
> very hard to interact with reliably from management tools.

Actually I think a better default would be for qemu to die right there and 
then.  If the host is getting IO errors then something is badly wrong, and 
you're probably screwed anyway.

Paul

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

[Avi Kivity]

Paul Brook wrote:
>>> 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.
>>>       
>> If by 'qemu console' you mean stderr, then fine, but please don't
>> spew log messages to the  monitor console, because that'll make it
>> very hard to interact with reliably from management tools.
>>     
>
> Actually I think a better default would be for qemu to die right there and 
> then.  If the host is getting IO errors then something is badly wrong, and 
> you're probably screwed anyway.
>   

If you're passing through a real disk or volume, this denies the guest 
the possibility of recover y (for example, if it is running a RAID setup 
over multiple volumes, or if you are testing the guest's ability to 
recover from errors).

For non-raw formats, you can pass through errors on data, but it is 
impossible to recover on metadata errors, so dying on I/O error should 
be fine.

-- 
Any sufficiently difficult bug is indistinguishable from a feature.

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

[Ian Jackson]

Avi Kivity writes ("Re: [Qemu-devel] [PATCH] bdrv_flush error handling"):
> For non-raw formats, you can pass through errors on data, but it is 
> impossible to recover on metadata errors, so dying on I/O error should 
> be fine.

You mean metadata write errors I assume.  I don't see why a metadata
read error is any worse than any other read error.

The guest will often prefer to be told that the volume was broken and
then to be denied the ability to continue accessing it.  Who knows
what the guest thinks of the device ?  Perhaps it's an unimportant
peripheral, or simulated network block device, used only for data
interchange.

Write errors for non-raw formats can easily be caused by a disk full
situation on the host.  Killing the guest hard is unfriendly for that
situation.

Ian.

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

[Paul Brook]

> Write errors for non-raw formats can easily be caused by a disk full
> situation on the host.  Killing the guest hard is unfriendly for that
> situation.

Disk full is a fundamentally unfriendly situation to be in. There is no good 
answer. Reporting errors back to the host has its own set of problems. Many 
guest OS effectively just lock up when this occurs.

Paul

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

[Ian Jackson]

Paul Brook writes ("Re: [Qemu-devel] [PATCH] bdrv_flush error handling"):
> Disk full is a fundamentally unfriendly situation to be in. There is no good 
> answer. Reporting errors back to the host has its own set of problems. Many 
> guest OS effectively just lock up when this occurs.

I think that's fine, surely ?  A locked up guest isn't very nice but
it's better than a guest shot dead.

Ian.

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

[Jamie Lokier]

Ian Jackson wrote:
> Paul Brook writes ("Re: [Qemu-devel] [PATCH] bdrv_flush error handling"):
> > Disk full is a fundamentally unfriendly situation to be in. There is no good 
> > answer. Reporting errors back to the host has its own set of problems. Many 
> > guest OS effectively just lock up when this occurs.
> 
> I think that's fine, surely ?  A locked up guest isn't very nice but
> it's better than a guest shot dead.

Well, a guest which receives an IDE write error might do things like
mark parts of the device bad, to avoiding writing to those parts.  If
the guest is running software RAID, for example, it will radically
change its behaviour in response to those errors.

Sometimes that's what you want, but sometimes it is really unwanted.
If the host runs out of disk space, ideally you might want to suspend
the guest until you can free up host disk space (or move to another
host), then resume the guest, perhaps manually.

Is savevm-upon-disk-full a realistic prospect?

-- Jamie

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

[Paul Brook]

> Is savevm-upon-disk-full a realistic prospect?

Not particularly useful. If you're run out of disk space, chances are that 
savevm will also fail.

Paul

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

[Jamie Lokier]

Paul Brook wrote:
> > Is savevm-upon-disk-full a realistic prospect?
> 
> Not particularly useful. If you're run out of disk space, chances are that 
> savevm will also fail.

I'm imagining (a) that the savevm space is preallocated, or (b) is on
a different disk.

-- Jamie

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

[Daniel P. Berrange]

On Wed, Feb 20, 2008 at 04:31:56PM +0000, Jamie Lokier wrote:
> Ian Jackson wrote:
> > Paul Brook writes ("Re: [Qemu-devel] [PATCH] bdrv_flush error handling"):
> > > Disk full is a fundamentally unfriendly situation to be in. There is no good 
> > > answer. Reporting errors back to the host has its own set of problems. Many 
> > > guest OS effectively just lock up when this occurs.
> > 
> > I think that's fine, surely ?  A locked up guest isn't very nice but
> > it's better than a guest shot dead.
> 
> Well, a guest which receives an IDE write error might do things like
> mark parts of the device bad, to avoiding writing to those parts.  If
> the guest is running software RAID, for example, it will radically
> change its behaviour in response to those errors.
> 
> Sometimes that's what you want, but sometimes it is really unwanted.
> If the host runs out of disk space, ideally you might want to suspend
> the guest until you can free up host disk space (or move to another
> host), then resume the guest, perhaps manually.
> 
> Is savevm-upon-disk-full a realistic prospect?

In the 'out of disk space' scenario you wouldn't need to save the guest - merely
stop its CPU. This gives the host admin the opportunity to hot-add storage
to the host & then resume execution, or to kill the VM, or to free enough
space to save the VM to disk / live migrate it to another host. 

Shooting it dead on any I/O error doesn't give the host admin any choices
at all

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

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

[Anthony Liguori]

Daniel P. Berrange wrote:
> On Wed, Feb 20, 2008 at 04:31:56PM +0000, Jamie Lokier wrote:
>   
>> Ian Jackson wrote:
>>     
>>> Paul Brook writes ("Re: [Qemu-devel] [PATCH] bdrv_flush error handling"):
>>>       
>>>> Disk full is a fundamentally unfriendly situation to be in. There is no good 
>>>> answer. Reporting errors back to the host has its own set of problems. Many 
>>>> guest OS effectively just lock up when this occurs.
>>>>         
>>> I think that's fine, surely ?  A locked up guest isn't very nice but
>>> it's better than a guest shot dead.
>>>       
>> Well, a guest which receives an IDE write error might do things like
>> mark parts of the device bad, to avoiding writing to those parts.  If
>> the guest is running software RAID, for example, it will radically
>> change its behaviour in response to those errors.
>>
>> Sometimes that's what you want, but sometimes it is really unwanted.
>> If the host runs out of disk space, ideally you might want to suspend
>> the guest until you can free up host disk space (or move to another
>> host), then resume the guest, perhaps manually.
>>
>> Is savevm-upon-disk-full a realistic prospect?
>>     
>
> In the 'out of disk space' scenario you wouldn't need to save the guest - merely
> stop its CPU. This gives the host admin the opportunity to hot-add storage
> to the host & then resume execution, or to kill the VM, or to free enough
> space to save the VM to disk / live migrate it to another host. 
>   

I agree.  Stopping the CPUs and spitting out a big fat warning message 
would be the best thing to do.  For the average user, this would give 
the opportunity to free up some space if possible.

Regards,

Anthony Liguori

> Shooting it dead on any I/O error doesn't give the host admin any choices
> at all
>
> Dan.
>   

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

[Rick Vernam]

On Wednesday 20 February 2008 01:01:33 pm Anthony Liguori wrote:
> Daniel P. Berrange wrote:
> > On Wed, Feb 20, 2008 at 04:31:56PM +0000, Jamie Lokier wrote:
> >> Ian Jackson wrote:
> >>> Paul Brook writes ("Re: [Qemu-devel] [PATCH] bdrv_flush error 
handling"):
> >>>> Disk full is a fundamentally unfriendly situation to be in. There is
> >>>> no good answer. Reporting errors back to the host has its own set of
> >>>> problems. Many guest OS effectively just lock up when this occurs.
> >>>
> >>> I think that's fine, surely ?  A locked up guest isn't very nice but
> >>> it's better than a guest shot dead.
> >>
> >> Well, a guest which receives an IDE write error might do things like
> >> mark parts of the device bad, to avoiding writing to those parts.  If
> >> the guest is running software RAID, for example, it will radically
> >> change its behaviour in response to those errors.
> >>
> >> Sometimes that's what you want, but sometimes it is really unwanted.
> >> If the host runs out of disk space, ideally you might want to suspend
> >> the guest until you can free up host disk space (or move to another
> >> host), then resume the guest, perhaps manually.
> >>
> >> Is savevm-upon-disk-full a realistic prospect?
> >
> > In the 'out of disk space' scenario you wouldn't need to save the guest -
> > merely stop its CPU. This gives the host admin the opportunity to hot-add
> > storage to the host & then resume execution, or to kill the VM, or to
> > free enough space to save the VM to disk / live migrate it to another
> > host.
>
> I agree.  Stopping the CPUs and spitting out a big fat warning message
> would be the best thing to do.  For the average user, this would give
> the opportunity to free up some space if possible.

agreed.

>
> Regards,
>
> Anthony Liguori
>
> > Shooting it dead on any I/O error doesn't give the host admin any choices
> > at all
> >
> > Dan.

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

[Thomas Irlet]

[-- Attachment #1: Type: text/plain, Size: 533 bytes --]

Sometimes the guest intentionally seeks the error. Example?

TrueCrypt 5.0 supports encryption of the full system disk. To get the real
size of the disk, the truecrypt driver queries the number of blocks of the
disk, but then tries to read after the last block, until it gets an error.
Qemu hangs in this operation. So, for me, the blockdriver has to give the
errors back to the guest (in this case, reading behind eof). In no cases,
qemu should die because of that.


Tom

PS: I had to patch truecrypt because of this qemu feature.

[-- Attachment #2: Type: text/html, Size: 559 bytes --]

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

[Ben Taylor]

---- "Daniel P. Berrange" <berrange@redhat.com> wrote: 
> On Wed, Feb 20, 2008 at 03:53:46PM +0000, Ian Jackson wrote:
> Content-Description: message body text
> > 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 ?
> 
> Yes, any i/o errors I'd expect to be propagated back up to the guest
> OS as the most appropriate IDE / SCSI error code.
> 
> > 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.
> 
> If by 'qemu console' you mean stderr, then fine, but please don't
> spew log messages to the  monitor console, because that'll make it
> very hard to interact with reliably from management tools. 

Would it make sense to have a log messages screen associated with
the monitor (like Ctrl-Alt-7) to deal with those sorts of things?

Ben

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

[Daniel P. Berrange]

On Thu, Feb 21, 2008 at 12:19:22PM -0500, Ben Taylor wrote:
> 
> ---- "Daniel P. Berrange" <berrange@redhat.com> wrote: 
> > On Wed, Feb 20, 2008 at 03:53:46PM +0000, Ian Jackson wrote:
> > Content-Description: message body text
> > > 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 ?
> > 
> > Yes, any i/o errors I'd expect to be propagated back up to the guest
> > OS as the most appropriate IDE / SCSI error code.
> > 
> > > 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.
> > 
> > If by 'qemu console' you mean stderr, then fine, but please don't
> > spew log messages to the  monitor console, because that'll make it
> > very hard to interact with reliably from management tools. 
> 
> Would it make sense to have a log messages screen associated with
> the monitor (like Ctrl-Alt-7) to deal with those sorts of things?

Why invent a new special QEMU log screen, when stderr works just fine. If an
app wants to capture log messages they just capture stderr and persist it.
We already capture stderr for exactly this reason in libvirt when managing
QEMU instances.

Dan,
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

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

[risc]

On Thu, Feb 21, 2008 at 05:24:10PM +0000, Daniel P. Berrange wrote:
> On Thu, Feb 21, 2008 at 12:19:22PM -0500, Ben Taylor wrote:
> > 
> > ---- "Daniel P. Berrange" <berrange@redhat.com> wrote: 
> > > On Wed, Feb 20, 2008 at 03:53:46PM +0000, Ian Jackson wrote:
> > > Content-Description: message body text
> > > > 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 ?
> > > 
> > > Yes, any i/o errors I'd expect to be propagated back up to the guest
> > > OS as the most appropriate IDE / SCSI error code.
> > > 
> > > > 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.
> > > 
> > > If by 'qemu console' you mean stderr, then fine, but please don't
> > > spew log messages to the  monitor console, because that'll make it
> > > very hard to interact with reliably from management tools. 
> > 
> > Would it make sense to have a log messages screen associated with
> > the monitor (like Ctrl-Alt-7) to deal with those sorts of things?
> 
> Why invent a new special QEMU log screen, when stderr works just fine. If an
> app wants to capture log messages they just capture stderr and persist it.
> We already capture stderr for exactly this reason in libvirt when managing
> QEMU instances.
> 
> Dan,
> -- 
> |=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
> |=-           Perl modules: http://search.cpan.org/~danberr/              -=|
> |=-               Projects: http://freshmeat.net/~danielpb/               -=|
> |=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 
> 

Please excuse my intrusion in this thread, but I'm a user of the new
ncurses user interface. when ssh'd in, running qemu, I don't believe 
having messages pop out of stderr and over the current screen contents is 
the appropriate behavior, as it sounds to me like it would cause redraw 
defects in the normal text console (via ncurses)

Julia Longtin <risc@volumehost.com>

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

[Daniel P. Berrange]

On Thu, Feb 21, 2008 at 11:28:23AM -0600, risc@volumehost.com wrote:
> On Thu, Feb 21, 2008 at 05:24:10PM +0000, Daniel P. Berrange wrote:
> > On Thu, Feb 21, 2008 at 12:19:22PM -0500, Ben Taylor wrote:
> > > > > 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.
> > > > 
> > > > If by 'qemu console' you mean stderr, then fine, but please don't
> > > > spew log messages to the  monitor console, because that'll make it
> > > > very hard to interact with reliably from management tools. 
> > > 
> > > Would it make sense to have a log messages screen associated with
> > > the monitor (like Ctrl-Alt-7) to deal with those sorts of things?
> > 
> > Why invent a new special QEMU log screen, when stderr works just fine. If an
> > app wants to capture log messages they just capture stderr and persist it.
> > We already capture stderr for exactly this reason in libvirt when managing
> > QEMU instances.
> 
> Please excuse my intrusion in this thread, but I'm a user of the new
> ncurses user interface. when ssh'd in, running qemu, I don't believe 
> having messages pop out of stderr and over the current screen contents is 
> the appropriate behavior, as it sounds to me like it would cause redraw 
> defects in the normal text console (via ncurses)

So just redirect stderr to a logfile, or /dev/null, etc, etc

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

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

[Ian Jackson]

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 542 bytes --]

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.


[-- Attachment #2: make bdrv_flush report errors --]
[-- Type: text/plain, Size: 5416 bytes --]

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

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

[andrzej zaborowski]

On 28/03/2008, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> 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.

It doesn't seem to apply against qemu, I could modify it but I don't
know how block-raw-win32.c reports errors.

Regards

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

[Ian Jackson]

andrzej zaborowski writes ("Re: [Qemu-devel] Re: [PATCH] bdrv_flush error handling"):
> It doesn't seem to apply against qemu, I could modify it but I don't
> know how block-raw-win32.c reports errors.

Thanks.  I'll deal with it.  I think it's just gone stale because the
head has moved on.

Ian.