Re: [Qemu-devel] [PATCH v3 8/9] virtio-scsi: convert virtio_scsi_bad_req() to use virtio_error()

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Mon, Sep 26, 2016 at 10:34:56AM +0200, Greg Kurz wrote:
> The virtio_scsi_bad_req() function is called when a guest sends a
> request with missing or ill-sized headers. This generally happens
> when the virtio_scsi_parse_req() function returns an error.
> 
> With this patch, virtio_scsi_bad_req() will mark the device as broken,
> detach the request from the virtqueue and free it, instead of forcing
> QEMU to exit.
> 
> In nearly all locations where virtio_scsi_bad_req() is called, the only
> thing to do next is to return to the caller.
> 
> The virtio_scsi_handle_cmd_req_prepare() function is an exception though.
> 
> It is called in a loop by virtio_scsi_handle_cmd_vq() and passed requests
> freshly popped from a cmd virtqueue; virtio_scsi_handle_cmd_req_prepare()
> does some sanity checks on the request and returns a boolean flag to
> indicate whether the request should be queued or not. In the latter case,
> virtio_scsi_handle_cmd_req_prepare() has detected a non-fatal error and
> sent a response back to the guest.
> 
> We have now a new condition to take into account: the device is broken
> and should stop all processing.
> 
> The return value of virtio_scsi_handle_cmd_req_prepare() is hence changed
> to an int. A return value of zero means that the request should be queued.
> Other non-fatal error cases where the reqyest shoudn't be queued  return

s/reqyest/request/

> @@ -574,11 +578,24 @@ static void virtio_scsi_handle_cmd_req_submit(VirtIOSCSI *s, VirtIOSCSIReq *req)
>  void virtio_scsi_handle_cmd_vq(VirtIOSCSI *s, VirtQueue *vq)
>  {
>      VirtIOSCSIReq *req, *next;
> +    int ret;
> +
>      QTAILQ_HEAD(, VirtIOSCSIReq) reqs = QTAILQ_HEAD_INITIALIZER(reqs);
>  
>      while ((req = virtio_scsi_pop_req(s, vq))) {
> -        if (virtio_scsi_handle_cmd_req_prepare(s, req)) {
> +        ret = virtio_scsi_handle_cmd_req_prepare(s, req);
> +        if (!ret) {
>              QTAILQ_INSERT_TAIL(&reqs, req, next);
> +        } else if (ret == -EINVAL) {
> +            /* The device is broken and shouldn't process any request */
> +            while (!QTAILQ_EMPTY(&reqs)) {
> +                req = QTAILQ_FIRST(&reqs);
> +                QTAILQ_REMOVE(&reqs, req, next);
> +                blk_io_unplug(req->sreq->dev->conf.blk);

Are you sure blk_io_plug() was called for this request?  If we returned
early in  virtio_scsi_handle_cmd_req_prepare() then it wasn't called.

> +                scsi_req_unref(req->sreq);

Which scsi_req_ref() is this paired with?  If it's the call in
scsi_req_enqueue() then that function was never called and we shouldn't
unref.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]