On Mon, Apr 9, 2018 at 6:10 AM, Stefan Hajnoczi <stefanha@xxxxxxxxxx> 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 _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization