Messages in this thread | | | From | Linus Torvalds <> | Date | Mon, 9 Apr 2018 09:52:13 -0700 | Subject | Re: [PATCH] vhost: fix vhost_vq_access_ok() log check |
| |
On Mon, Apr 9, 2018 at 6:10 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote: > @@ -1246,7 +1246,7 @@ int vhost_vq_access_ok(struct vhost_virtqueue *vq) > { > int ret = vq_log_access_ok(vq, vq->log_base); > > - if (ret || vq->iotlb) > + if (!ret || vq->iotlb) > return ret;
That logic is still very non-obvious.
This code already had one bug because of an odd illegible test sequence. Let's not keep the crazy code.
Why not just do the *obvious* thing, and get rid of "ret" entirely, and make the damn thing return a boolean, and then just write it all as
/* Caller should have vq mutex and device mutex */ bool vhost_vq_access_ok(struct vhost_virtqueue *vq) { if (!vq_log_access_ok(vq, vq->log_base)) return false;
if (vq->iotlb || vq_access_ok(vq, vq->num, vq->desc, vq->avail, vq->used); }
which makes the logic obvious: if vq_log_access_ok() fails, then then vhost_vq_access_ok() fails unconditionally.
Otherwise, we need to have an iotlb, or a successful vq_access_ok() check.
Doesn't that all make more sense, and avoid the insane "ret" value use that is really quite subtle?
Linus
| |