From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KQYjm-0000aH-Nh for qemu-devel@nongnu.org; Tue, 05 Aug 2008 22:26:54 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KQYjl-0000X3-Jm for qemu-devel@nongnu.org; Tue, 05 Aug 2008 22:26:53 -0400 Received: from [199.232.76.173] (port=52652 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KQYjl-0000Wp-CN for qemu-devel@nongnu.org; Tue, 05 Aug 2008 22:26:53 -0400 Received: from py-out-1112.google.com ([64.233.166.179]:48809) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1KQYjl-0000a4-Dd for qemu-devel@nongnu.org; Tue, 05 Aug 2008 22:26:53 -0400 Received: by py-out-1112.google.com with SMTP id p76so1519635pyb.10 for ; Tue, 05 Aug 2008 19:26:51 -0700 (PDT) Message-ID: <48990BC6.1050503@codemonkey.ws> Date: Tue, 05 Aug 2008 21:26:14 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] report read/write errors to IDE guest driver as ECC errors References: <20080805115506.GR4478@implementation.uk.xensource.com> In-Reply-To: <20080805115506.GR4478@implementation.uk.xensource.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 8bit Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Samuel Thibault wrote: > report read/write errors to IDE guest driver as ECC errors > so that the guest knows that e.g. writes on read-only backends have > failed. > > Signed-off-by: Samuel Thibault > I'm glad you sent this patch as I think it's something we really need to improve in QEMU. A patch like this has appeared on the list a number of times, and each time it meets with a fair bit of criticism. The most cited issue is that indiscriminately passing IO errors to the guest is not really correct. If you're passing through a drive, then EIO errors are pretty reasonable to pass through as an ECC error. If you're dealing with a file, generating an ECC error on ENOSPC is not really accurate. The guest cannot really do anything about that either and is likely to just further corrupt itself. I think a patch like this is good in theory but it needs to do a better job only handling the case where an error occurs that ECC really makes sense. For things like ENOSPC, there are better error handling strategies. For instance, vm_stop()'ing the guest and printing out an error message. This would allow the admin to free up some space, and then resume the guest. Even if you just stubbed these things out with FIXMEs, it would be better than pretending that these cases didn't exist :-) Regards, Anthony Liguori > Index: hw/ide.c > =================================================================== > --- hw/ide.c (révision 4985) > +++ hw/ide.c (copie de travail) > @@ -891,7 +891,6 @@ > return 1; > } > > -/* XXX: handle errors */ > static void ide_read_dma_cb(void *opaque, int ret) > { > BMDMAState *bm = opaque; > @@ -899,6 +898,14 @@ > int n; > int64_t sector_num; > > + if (ret) { > + s->status = READY_STAT | ERR_STAT; > + s->error = ABRT_ERR | ECC_ERR; > + s->nsector = 0; > + ide_set_irq(s); > + goto eot; > + } > + > n = s->io_buffer_size >> 9; > sector_num = ide_get_sector(s); > if (n > 0) { > @@ -992,7 +999,6 @@ > } > } > > -/* XXX: handle errors */ > static void ide_write_dma_cb(void *opaque, int ret) > { > BMDMAState *bm = opaque; > @@ -1000,6 +1006,14 @@ > int n; > int64_t sector_num; > > + if (ret) { > + s->status = READY_STAT | ERR_STAT; > + s->error = ABRT_ERR | ECC_ERR; > + s->nsector = 0; > + ide_set_irq(s); > + goto eot; > + } > + > n = s->io_buffer_size >> 9; > sector_num = ide_get_sector(s); > if (n > 0) { > > >