On Tue, Nov 28, 2017 at 10:11:54AM +0100, Cornelia Huck wrote: > On Mon, 27 Nov 2017 23:25:28 +0530 (IST) > P J P <ppan...@redhat.com> wrote: > > +-- On Mon, 27 Nov 2017, Stefan Hajnoczi wrote --+ > > | > + if (!vdev->vq[n].vring.num || !desc || !vdev->vq[n].vring.align) { > > | ... > > | vdev->vq[n].vring.desc = desc; > > | > > | Why !desc? > > > > virtio_queue_set_rings > > virtio_init_region_cache > > VirtQueue *vq = &vdev->vq[n]; > > ... > > addr = vq->vring.desc; > > if (!addr) { > > return; > > } > > > > These checks seem to be repeating all over. As mentioned earlier, could > > these > > be collated in one place, maybe virtio_queue_get_num()? > > > > int virtio_queue_get_num(VirtIODevice *vdev, int n) > > { > > VirtQueue *vq = &vdev->vq[n]; > > > > if (!vq->.vring.num > > || !vq->vring.desc > > || !vq->vring.align) { > > return 0; /* vq not set */ > > } > > > > return vdev->vq[n].vring.num; > > } > > This is conflating different things: > - vq does not exist (num == 0) > - vq is not setup by the guest (desc == 0) > - vq has no valid alignment (which is only relevant for legacy)
I agree. Stefan
signature.asc
Description: PGP signature