Patchwork [1/3] virtio-blk: report non-zero status when failing SG_IO requests

login
register
mail settings
Submitter Paolo Bonzini
Date March 7, 2012, 6:01 p.m.
Message ID <1331143301-28408-2-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/145323/
State New
Headers show

Comments

Paolo Bonzini - March 7, 2012, 6:01 p.m.
Linux really looks only at scsi->errors.  Arguably it is their bug,
but we can make it safe for older guests now.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio-blk.c |   48 +++++++++++++++++++++++-------------------------
 1 files changed, 23 insertions(+), 25 deletions(-)
Orit Wasserman - March 13, 2012, 1:36 p.m.
On 03/07/2012 08:01 PM, Paolo Bonzini wrote:
> Linux really looks only at scsi->errors.  Arguably it is their bug,
> but we can make it safe for older guests now.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/virtio-blk.c |   48 +++++++++++++++++++++++-------------------------
>  1 files changed, 23 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index 49990f8..b7e510d 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -145,20 +145,12 @@ static VirtIOBlockReq *virtio_blk_get_request(VirtIOBlock *s)
>      return req;
>  }
>  
> -#ifdef __linux__
>  static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
>  {
> -    struct sg_io_hdr hdr;
> -    int ret;
> +    int ret = -1;
>      int status;
>      int i;
>  
> -    if ((req->dev->vdev.guest_features & (1 << VIRTIO_BLK_F_SCSI)) == 0) {
> -        virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
> -        g_free(req);
> -        return;
> -    }
> -
>      /*
>       * We require at least one output segment each for the virtio_blk_outhdr
>       * and the SCSI command block.
> @@ -173,20 +165,26 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
>      }
>  
>      /*
> -     * No support for bidirection commands yet.
> +     * The scsi inhdr is placed in the second-to-last input segment, just
> +     * before the regular inhdr.
>       */
> -    if (req->elem.out_num > 2 && req->elem.in_num > 3) {
> -        virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
> -        g_free(req);
> -        return;
> +    req->scsi = (void *)req->elem.in_sg[req->elem.in_num - 2].iov_base;
> +
> +    if ((req->dev->vdev.guest_features & (1 << VIRTIO_BLK_F_SCSI)) == 0) {
> +        status = VIRTIO_BLK_S_UNSUPP;
> +        goto fail;
>      }
>  
>      /*
> -     * The scsi inhdr is placed in the second-to-last input segment, just
> -     * before the regular inhdr.
> +     * No support for bidirection commands yet.
>       */
> -    req->scsi = (void *)req->elem.in_sg[req->elem.in_num - 2].iov_base;
> +    if (req->elem.out_num > 2 && req->elem.in_num > 3) {
> +        status = VIRTIO_BLK_S_UNSUPP;
> +        goto fail;
> +    }
>  
> +#ifdef __linux__
> +    struct sg_io_hdr hdr;
>      memset(&hdr, 0, sizeof(struct sg_io_hdr));
>      hdr.interface_id = 'S';
>      hdr.cmd_len = req->elem.out_sg[1].iov_len;
> @@ -229,9 +227,7 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
>  
>      ret = bdrv_ioctl(req->dev->bs, SG_IO, &hdr);
>      if (ret) {
> -        status = VIRTIO_BLK_S_UNSUPP;
> -        hdr.status = ret;
> -        hdr.resid = hdr.dxfer_len;
> +        goto fail;
>      } else if (hdr.status) {
>          status = VIRTIO_BLK_S_IOERR;
>      } else {
> @@ -258,14 +254,16 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
>  
>      virtio_blk_req_complete(req, status);
>      g_free(req);
> -}
>  #else
> -static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
> -{
> -    virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
> +    abort();
> +#endif
> +
> +fail:
> +    /* Just put anything nonzero so that the ioctl fails in the guest.  */
> +    stl_p(&req->scsi->errors, 255);
> +    virtio_blk_req_complete(req, status);

I get to following compile error:
In function ‘virtio_blk_handle_request’:
virtio-blk.c:264:28: error: ‘status’ may be used uninitialized in this function [-Werror=uninitialized]
virtio-blk.c:151:9: note: ‘status’ was declared here
cc1: all warnings being treated as errors

Are you using  -disable-werror ?

Orit
>      g_free(req);
>  }
> -#endif /* __linux__ */
>  
>  typedef struct MultiReqBuffer {
>      BlockRequest        blkreq[32];
Paolo Bonzini - March 13, 2012, 1:39 p.m.
Il 13/03/2012 14:36, Orit Wasserman ha scritto:
> I get to following compile error:
> In function ‘virtio_blk_handle_request’:
> virtio-blk.c:264:28: error: ‘status’ may be used uninitialized in this function [-Werror=uninitialized]
> virtio-blk.c:151:9: note: ‘status’ was declared here
> cc1: all warnings being treated as errors
> 
> Are you using  -disable-werror ?

No, perhaps a different compiler though.

Paolo
Orit Wasserman - March 13, 2012, 1:44 p.m.
On 03/13/2012 03:39 PM, Paolo Bonzini wrote:
> Il 13/03/2012 14:36, Orit Wasserman ha scritto:
>> I get to following compile error:
>> In function ‘virtio_blk_handle_request’:
>> virtio-blk.c:264:28: error: ‘status’ may be used uninitialized in this function [-Werror=uninitialized]
>> virtio-blk.c:151:9: note: ‘status’ was declared here
>> cc1: all warnings being treated as errors
>>
>> Are you using  -disable-werror ?
> 
> No, perhaps a different compiler though.

could be I'm using  (GCC) 4.6.1 20110908 (Red Hat 4.6.1-9)

> 
> Paolo
Eric Blake - March 13, 2012, 3:17 p.m.
On 03/13/2012 07:39 AM, Paolo Bonzini wrote:
> Il 13/03/2012 14:36, Orit Wasserman ha scritto:
>> I get to following compile error:
>> In function ‘virtio_blk_handle_request’:
>> virtio-blk.c:264:28: error: ‘status’ may be used uninitialized in this function [-Werror=uninitialized]
>> virtio-blk.c:151:9: note: ‘status’ was declared here
>> cc1: all warnings being treated as errors
>>
>> Are you using  -disable-werror ?
> 
> No, perhaps a different compiler though.

Or a difference in -O level.  gcc is notoriously bad at missing
-Wuninitialized at -O0, and not warning until -O1 or -O2.  [I'm still
impressed at how the Java language was able to mandate uninitialized
detection into the compiler as a required part of the language.]

Patch

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 49990f8..b7e510d 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -145,20 +145,12 @@  static VirtIOBlockReq *virtio_blk_get_request(VirtIOBlock *s)
     return req;
 }
 
-#ifdef __linux__
 static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
 {
-    struct sg_io_hdr hdr;
-    int ret;
+    int ret = -1;
     int status;
     int i;
 
-    if ((req->dev->vdev.guest_features & (1 << VIRTIO_BLK_F_SCSI)) == 0) {
-        virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
-        g_free(req);
-        return;
-    }
-
     /*
      * We require at least one output segment each for the virtio_blk_outhdr
      * and the SCSI command block.
@@ -173,20 +165,26 @@  static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
     }
 
     /*
-     * No support for bidirection commands yet.
+     * The scsi inhdr is placed in the second-to-last input segment, just
+     * before the regular inhdr.
      */
-    if (req->elem.out_num > 2 && req->elem.in_num > 3) {
-        virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
-        g_free(req);
-        return;
+    req->scsi = (void *)req->elem.in_sg[req->elem.in_num - 2].iov_base;
+
+    if ((req->dev->vdev.guest_features & (1 << VIRTIO_BLK_F_SCSI)) == 0) {
+        status = VIRTIO_BLK_S_UNSUPP;
+        goto fail;
     }
 
     /*
-     * The scsi inhdr is placed in the second-to-last input segment, just
-     * before the regular inhdr.
+     * No support for bidirection commands yet.
      */
-    req->scsi = (void *)req->elem.in_sg[req->elem.in_num - 2].iov_base;
+    if (req->elem.out_num > 2 && req->elem.in_num > 3) {
+        status = VIRTIO_BLK_S_UNSUPP;
+        goto fail;
+    }
 
+#ifdef __linux__
+    struct sg_io_hdr hdr;
     memset(&hdr, 0, sizeof(struct sg_io_hdr));
     hdr.interface_id = 'S';
     hdr.cmd_len = req->elem.out_sg[1].iov_len;
@@ -229,9 +227,7 @@  static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
 
     ret = bdrv_ioctl(req->dev->bs, SG_IO, &hdr);
     if (ret) {
-        status = VIRTIO_BLK_S_UNSUPP;
-        hdr.status = ret;
-        hdr.resid = hdr.dxfer_len;
+        goto fail;
     } else if (hdr.status) {
         status = VIRTIO_BLK_S_IOERR;
     } else {
@@ -258,14 +254,16 @@  static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
 
     virtio_blk_req_complete(req, status);
     g_free(req);
-}
 #else
-static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
-{
-    virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
+    abort();
+#endif
+
+fail:
+    /* Just put anything nonzero so that the ioctl fails in the guest.  */
+    stl_p(&req->scsi->errors, 255);
+    virtio_blk_req_complete(req, status);
     g_free(req);
 }
-#endif /* __linux__ */
 
 typedef struct MultiReqBuffer {
     BlockRequest        blkreq[32];