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

[Greg Kurz]

On Mon, 26 Sep 2016 17:35:38 +0100
Stefan Hajnoczi <address@hidden> wrote:

> 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/
> 

oops...

> > @@ -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.
> 

Early return in virtio_scsi_handle_cmd_req_prepare() means an error was
detected, in which case the request didn't get queued; we are sure that
blk_io_plug() was called for all requests in this queue.

> > +                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.

It is paired with the one in virtio_scsi_handle_cmd_req_prepare(), which
is called just before blk_io_plug().

But looking at the patch again, I realize I missed this:

@@ -562,7 +562,7 @@ static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s,
     }
     scsi_req_ref(req->sreq);
     blk_io_plug(d->conf.blk);
-    return true;
+    return 0;
 }
 
 static void virtio_scsi_handle_cmd_req_submit(VirtIOSCSI *s, VirtIOSCSIReq *req

I'll send a v4.

Cheers.

--
Greg

pgpA8bPCwkFM5.pgp
Description: OpenPGP digital signature