Re: [Qemu-devel] [PATCH] report read/write errors to IDE guest driver as ECC errors

[Anthony Liguori <anthony@codemonkey.ws>]

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 <samuel.thibault@eu.citrix.com>
>   

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