On Tue, Apr 21, 2015 at 03:24:25PM +0800, Fam Zheng wrote:
> On Tue, 04/21 08:49, Michael S. Tsirkin wrote:
> > On Fri, Apr 17, 2015 at 03:59:21PM +0800, Fam Zheng wrote:
> > > When getting invalid data from vring, virtqueue_pop used to print an
> > > error and exit.
> > >
> > > Add an errp parameter so it can return the error to callers.
> > >
> > > Signed-off-by: Fam Zheng <f...@redhat.com>
> > > ---
> > > hw/9pfs/virtio-9p.c | 2 +-
> > > hw/block/virtio-blk.c | 2 +-
> > > hw/char/virtio-serial-bus.c | 10 +++----
> > > hw/net/virtio-net.c | 6 ++--
> > > hw/scsi/virtio-scsi.c | 2 +-
> > > hw/virtio/virtio-balloon.c | 4 +--
> > > hw/virtio/virtio-rng.c | 2 +-
> > > hw/virtio/virtio.c | 70
> > > ++++++++++++++++++++++++++++++++++-----------
> > > include/hw/virtio/virtio.h | 2 +-
> > > 9 files changed, 69 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> > > index 4964da0..17d0c4a 100644
> > > --- a/hw/9pfs/virtio-9p.c
> > > +++ b/hw/9pfs/virtio-9p.c
> > > @@ -3259,7 +3259,7 @@ void handle_9p_output(VirtIODevice *vdev, VirtQueue
> > > *vq)
> > > ssize_t len;
> > >
> > > while ((pdu = alloc_pdu(s)) &&
> > > - (len = virtqueue_pop(vq, &pdu->elem)) != 0) {
> > > + (len = virtqueue_pop(vq, &pdu->elem, &error_abort)) != 0) {
> > > uint8_t *ptr;
> > > pdu->s = s;
> > > BUG_ON(pdu->elem.out_num == 0 || pdu->elem.in_num == 0);
> > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > index f7d8528..0b66ee1 100644
> > > --- a/hw/block/virtio-blk.c
> > > +++ b/hw/block/virtio-blk.c
> > > @@ -191,7 +191,7 @@ static VirtIOBlockReq
> > > *virtio_blk_get_request(VirtIOBlock *s)
> > > {
> > > VirtIOBlockReq *req = virtio_blk_alloc_request(s);
> > >
> > > - if (!virtqueue_pop(s->vq, &req->elem)) {
> > > + if (!virtqueue_pop(s->vq, &req->elem, &error_abort)) {
> > > virtio_blk_free_request(req);
> > > return NULL;
> > > }
> > > diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> > > index a56dafc..76a934b 100644
> > > --- a/hw/char/virtio-serial-bus.c
> > > +++ b/hw/char/virtio-serial-bus.c
> > > @@ -94,7 +94,7 @@ static size_t write_to_port(VirtIOSerialPort *port,
> > > while (offset < size) {
> > > size_t len;
> > >
> > > - if (!virtqueue_pop(vq, &elem)) {
> > > + if (!virtqueue_pop(vq, &elem, &error_abort)) {
> > > break;
> > > }
> > >
> > > @@ -116,7 +116,7 @@ static void discard_vq_data(VirtQueue *vq,
> > > VirtIODevice *vdev)
> > > if (!virtio_queue_ready(vq)) {
> > > return;
> > > }
> > > - while (virtqueue_pop(vq, &elem)) {
> > > + while (virtqueue_pop(vq, &elem, &error_abort)) {
> > > virtqueue_push(vq, &elem, 0);
> > > }
> > > virtio_notify(vdev, vq);
> > > @@ -137,7 +137,7 @@ static void do_flush_queued_data(VirtIOSerialPort
> > > *port, VirtQueue *vq,
> > >
> > > /* Pop an elem only if we haven't left off a previous one
> > > mid-way */
> > > if (!port->elem.out_num) {
> > > - if (!virtqueue_pop(vq, &port->elem)) {
> > > + if (!virtqueue_pop(vq, &port->elem, &error_abort)) {
> > > break;
> > > }
> > > port->iov_idx = 0;
> > > @@ -190,7 +190,7 @@ static size_t send_control_msg(VirtIOSerial *vser,
> > > void *buf, size_t len)
> > > if (!virtio_queue_ready(vq)) {
> > > return 0;
> > > }
> > > - if (!virtqueue_pop(vq, &elem)) {
> > > + if (!virtqueue_pop(vq, &elem, &error_abort)) {
> > > return 0;
> > > }
> > >
> > > @@ -420,7 +420,7 @@ static void control_out(VirtIODevice *vdev, VirtQueue
> > > *vq)
> > >
> > > len = 0;
> > > buf = NULL;
> > > - while (virtqueue_pop(vq, &elem)) {
> > > + while (virtqueue_pop(vq, &elem, &error_abort)) {
> > > size_t cur_len;
> > >
> > > cur_len = iov_size(elem.out_sg, elem.out_num);
> > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > index 59f76bc..bbcb51f 100644
> > > --- a/hw/net/virtio-net.c
> > > +++ b/hw/net/virtio-net.c
> > > @@ -804,7 +804,7 @@ static void virtio_net_handle_ctrl(VirtIODevice
> > > *vdev, VirtQueue *vq)
> > > struct iovec *iov, *iov2;
> > > unsigned int iov_cnt;
> > >
> > > - while (virtqueue_pop(vq, &elem)) {
> > > + while (virtqueue_pop(vq, &elem, &error_abort)) {
> > > if (iov_size(elem.in_sg, elem.in_num) < sizeof(status) ||
> > > iov_size(elem.out_sg, elem.out_num) < sizeof(ctrl)) {
> > > error_report("virtio-net ctrl missing headers");
> > > @@ -1031,7 +1031,7 @@ static ssize_t virtio_net_receive(NetClientState
> > > *nc, const uint8_t *buf, size_t
> > >
> > > total = 0;
> > >
> > > - if (virtqueue_pop(q->rx_vq, &elem) == 0) {
> > > + if (virtqueue_pop(q->rx_vq, &elem, &error_abort) == 0) {
> > > if (i == 0)
> > > return -1;
> > > error_report("virtio-net unexpected empty queue: "
> > > @@ -1134,7 +1134,7 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue
> > > *q)
> > > return num_packets;
> > > }
> > >
> > > - while (virtqueue_pop(q->tx_vq, &elem)) {
> > > + while (virtqueue_pop(q->tx_vq, &elem, &error_abort)) {
> > > ssize_t ret, len;
> > > unsigned int out_num = elem.out_num;
> > > struct iovec *out_sg = &elem.out_sg[0];
> > > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> > > index c9bea06..40ba03d 100644
> > > --- a/hw/scsi/virtio-scsi.c
> > > +++ b/hw/scsi/virtio-scsi.c
> > > @@ -177,7 +177,7 @@ static int virtio_scsi_parse_req(VirtIOSCSIReq *req,
> > > static VirtIOSCSIReq *virtio_scsi_pop_req(VirtIOSCSI *s, VirtQueue *vq)
> > > {
> > > VirtIOSCSIReq *req = virtio_scsi_init_req(s, vq);
> > > - if (!virtqueue_pop(vq, &req->elem)) {
> > > + if (!virtqueue_pop(vq, &req->elem, &error_abort)) {
> > > virtio_scsi_free_req(req);
> > > return NULL;
> > > }
> > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > > index 95b0643..e26c0a7 100644
> > > --- a/hw/virtio/virtio-balloon.c
> > > +++ b/hw/virtio/virtio-balloon.c
> > > @@ -206,7 +206,7 @@ static void virtio_balloon_handle_output(VirtIODevice
> > > *vdev, VirtQueue *vq)
> > > VirtQueueElement elem;
> > > MemoryRegionSection section;
> > >
> > > - while (virtqueue_pop(vq, &elem)) {
> > > + while (virtqueue_pop(vq, &elem, &error_abort)) {
> > > size_t offset = 0;
> > > uint32_t pfn;
> > >
> > > @@ -246,7 +246,7 @@ static void virtio_balloon_receive_stats(VirtIODevice
> > > *vdev, VirtQueue *vq)
> > > size_t offset = 0;
> > > qemu_timeval tv;
> > >
> > > - if (!virtqueue_pop(vq, elem)) {
> > > + if (!virtqueue_pop(vq, elem, &error_abort)) {
> > > goto out;
> > > }
> > >
> > > diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
> > > index 9e1bd75..c3cbdc3 100644
> > > --- a/hw/virtio/virtio-rng.c
> > > +++ b/hw/virtio/virtio-rng.c
> > > @@ -56,7 +56,7 @@ static void chr_read(void *opaque, const void *buf,
> > > size_t size)
> > >
> > > offset = 0;
> > > while (offset < size) {
> > > - if (!virtqueue_pop(vrng->vq, &elem)) {
> > > + if (!virtqueue_pop(vrng->vq, &elem, &error_abort)) {
> > > break;
> > > }
> > > len = iov_from_buf(elem.in_sg, elem.in_num,
> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > index 1cd454b..e6f9f6b 100644
> > > --- a/hw/virtio/virtio.c
> > > +++ b/hw/virtio/virtio.c
> > > @@ -481,29 +481,49 @@ fail:
> > > error_setg(errp, "virtio: error trying to map MMIO memory");
> > > }
> > >
> > > -int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
> > > +static void virtqueue_undo_map_sg(struct iovec *sg, hwaddr *addr,
> > > + size_t num_sg, int is_write)
> > > +{
> > > + int i;
> > > +
> > > + for (i = 0; i < num_sg; i++) {
> > > + cpu_physical_memory_unmap(sg[i].iov_base, sg[i].iov_len,
> > > + is_write, 0);
> > > + }
> > > +}
> > > +
> > > +int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem, Error **errp)
> > > {
> > > unsigned int i, head, max;
> > > + int ret;
> > > hwaddr desc_pa = vq->vring.desc;
> > > VirtIODevice *vdev = vq->vdev;
> > > + Error *local_err = NULL;
> > >
> > > - if (!virtqueue_num_heads(vq, vq->last_avail_idx, &error_abort))
> > > - return 0;
> > > + ret = virtqueue_num_heads(vq, vq->last_avail_idx, &local_err);
> > > + if (ret <= 0) {
> > > + goto err;
> > > + }
> > >
> >
> > Strange.
> > Doesn't this make pop print an error if ring is empty?
>
> Code at err label propagates local_err (NULL if empty, hence nop) and returns
> ret (0 if empty). Maybe rename it to "out".
But there's no need to print two errors. virtqueue_num_heads already
prints an error, so just exit here.
> >
> >
> >
> > > /* When we start there are none of either input nor output. */
> > > elem->out_num = elem->in_num = 0;
> > >
> > > max = vq->vring.num;
> > >
> > > - i = head = virtqueue_get_head(vq, vq->last_avail_idx++,
> > > &error_abort);
> > > + ret = virtqueue_get_head(vq, vq->last_avail_idx++, &local_err);
> > > + if (ret < 0) {
> > > + goto err;
> > > + }
> > > + head = i = ret;
> > > +
> > > if (virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
> > > vring_set_avail_event(vq, vq->last_avail_idx);
> > > }
> > >
> > > if (vring_desc_flags(vdev, desc_pa, i) & VRING_DESC_F_INDIRECT) {
> > > if (vring_desc_len(vdev, desc_pa, i) % sizeof(VRingDesc)) {
> > > - error_report("Invalid size for indirect buffer table");
> > > - exit(1);
> > > + error_setg(errp, "Invalid size for indirect buffer table");
> > > + return -EINVAL;
> >
> > Again, you have both error_setg and a return code.
> > There's no need for this.
> > Just return ring empty on error.
>
> How would caller distinguish real ring empty from error then?
>
> Fam
There's no need for it to do this. When some function detects an error,
it should set the NEEDS_RESET status, and be done with it. No need to
propagate the error condition back and forth, it would be a bunch of
poorly-tested code.
--
MST
