Message ID | 1331143301-28408-2-git-send-email-pbonzini@redhat.com |
---|---|
State | New |
Headers | show |
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];
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
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
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.]
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];
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(-)