On Wed, Aug 31, 2016 at 06:04:22AM -0600, nayden wrote:
> Addressing "this function needs a cleanup block, lots of free(blah); return
> (0)"
> comment for vioblk_notifyq() and doing the same for vionet_enq_rx().
>
go for it, thanks.
> Index: virtio.c
> ===================================================================
> RCS file: /home/nayden/cvsync/src/usr.sbin/vmd/virtio.c,v
> retrieving revision 1.17
> diff -u -p -r1.17 virtio.c
> --- virtio.c 17 Aug 2016 05:07:13 -0000 1.17
> +++ virtio.c 31 Aug 2016 10:35:55 -0000
> @@ -353,8 +353,7 @@ vioblk_do_write(struct vioblk_dev *dev,
> }
>
> /*
> - * XXX this function needs a cleanup block, lots of free(blah); return (0)
> - * in various cases, ds should be set to VIRTIO_BLK_S_IOERR, if we can
> + * XXX in various cases, ds should be set to VIRTIO_BLK_S_IOERR, if we can
> * XXX cant trust ring data from VM, be extra cautious.
> */
> int
> @@ -390,8 +389,7 @@ vioblk_notifyq(struct vioblk_dev *dev)
>
> if (read_mem(q_gpa, vr, vr_sz)) {
> log_warnx("error reading gpa 0x%llx", q_gpa);
> - free(vr);
> - return (0);
> + goto out;
> }
>
> /* Compute offsets in ring of descriptors, avail ring, and used ring */
> @@ -406,8 +404,7 @@ vioblk_notifyq(struct vioblk_dev *dev)
>
> if ((avail->idx & VIOBLK_QUEUE_MASK) == idx) {
> log_warnx("vioblk queue notify - nothing to do?");
> - free(vr);
> - return (0);
> + goto out;
> }
>
> cmd_desc_idx = avail->ring[idx] & VIOBLK_QUEUE_MASK;
> @@ -416,16 +413,14 @@ vioblk_notifyq(struct vioblk_dev *dev)
> if ((cmd_desc->flags & VRING_DESC_F_NEXT) == 0) {
> log_warnx("unchained vioblk cmd descriptor received "
> "(idx %d)", cmd_desc_idx);
> - free(vr);
> - return (0);
> + goto out;
> }
>
> /* Read command from descriptor ring */
> if (read_mem(cmd_desc->addr, &cmd, cmd_desc->len)) {
> log_warnx("vioblk: command read_mem error @ 0x%llx",
> cmd_desc->addr);
> - free(vr);
> - return (0);
> + goto out;
> }
>
> switch (cmd.type) {
> @@ -437,8 +432,7 @@ vioblk_notifyq(struct vioblk_dev *dev)
> if ((secdata_desc->flags & VRING_DESC_F_NEXT) == 0) {
> log_warnx("unchained vioblk data descriptor "
> "received (idx %d)", cmd_desc_idx);
> - free(vr);
> - return (0);
> + goto out;
> }
>
> secbias = 0;
> @@ -453,8 +447,7 @@ vioblk_notifyq(struct vioblk_dev *dev)
> if (secdata == NULL) {
> log_warnx("vioblk: block read error, "
> "sector %lld", cmd.sector);
> - free(vr);
> - return (0);
> + goto out;
> }
>
> if (write_mem(secdata_desc->addr, secdata,
> @@ -463,9 +456,8 @@ vioblk_notifyq(struct vioblk_dev *dev)
> "data to gpa @ 0x%llx",
> secdata_desc->addr);
> dump_descriptor_chain(desc, cmd_desc_idx);
> - free(vr);
> free(secdata);
> - return (0);
> + goto out;
> }
>
> free(secdata);
> @@ -484,8 +476,7 @@ vioblk_notifyq(struct vioblk_dev *dev)
> log_warnx("can't write device status data @ "
> "0x%llx", ds_desc->addr);
> dump_descriptor_chain(desc, cmd_desc_idx);
> - free(vr);
> - return (0);
> + goto out;
> }
>
>
> @@ -509,16 +500,14 @@ vioblk_notifyq(struct vioblk_dev *dev)
> if ((secdata_desc->flags & VRING_DESC_F_NEXT) == 0) {
> log_warnx("wr vioblk: unchained vioblk data "
> "descriptor received (idx %d)", cmd_desc_idx);
> - free(vr);
> - return (0);
> + goto out;
> }
>
> secdata = malloc(MAXPHYS);
> if (secdata == NULL) {
> log_warn("wr vioblk: malloc error, len %d",
> secdata_desc->len);
> - free(vr);
> - return (0);
> + goto out;
> }
>
> secbias = 0;
> @@ -529,17 +518,15 @@ vioblk_notifyq(struct vioblk_dev *dev)
> "sector data @ 0x%llx",
> secdata_desc->addr);
> dump_descriptor_chain(desc, cmd_desc_idx);
> - free(vr);
> free(secdata);
> - return (0);
> + goto out;
> }
>
> if (vioblk_do_write(dev, cmd.sector + secbias,
> secdata, (ssize_t)secdata_desc->len)) {
> log_warnx("wr vioblk: disk write error");
> - free(vr);
> free(secdata);
> - return (0);
> + goto out;
> }
>
> secbias += secdata_desc->len / VIRTIO_BLK_SECTOR_SIZE;
> @@ -559,8 +546,7 @@ vioblk_notifyq(struct vioblk_dev *dev)
> log_warnx("wr vioblk: can't write device status "
> "data @ 0x%llx", ds_desc->addr);
> dump_descriptor_chain(desc, cmd_desc_idx);
> - free(vr);
> - return (0);
> + goto out;
> }
>
> ret = 1;
> @@ -584,8 +570,7 @@ vioblk_notifyq(struct vioblk_dev *dev)
> log_warnx("fl vioblk: can't write device status "
> "data @ 0x%llx", ds_desc->addr);
> dump_descriptor_chain(desc, cmd_desc_idx);
> - free(vr);
> - return (0);
> + goto out;
> }
>
> ret = 1;
> @@ -601,9 +586,8 @@ vioblk_notifyq(struct vioblk_dev *dev)
> }
> break;
> }
> -
> +out:
> free(vr);
> -
> return (ret);
> }
>
> @@ -810,8 +794,7 @@ vionet_enq_rx(struct vionet_dev *dev, ch
>
> if (read_mem(q_gpa, vr, vr_sz)) {
> log_warnx("rx enq: error reading gpa 0x%llx", q_gpa);
> - free(vr);
> - return (0);
> + goto out;
> }
>
> /* Compute offsets in ring of descriptors, avail ring, and used ring */
> @@ -825,8 +808,7 @@ vionet_enq_rx(struct vionet_dev *dev, ch
>
> if ((dev->vq[0].notified_avail & VIONET_QUEUE_MASK) == idx) {
> log_warnx("vionet queue notify - no space, dropping packet");
> - free(vr);
> - return (0);
> + goto out;
> }
>
> hdr_desc_idx = avail->ring[idx] & VIONET_QUEUE_MASK;
> @@ -839,16 +821,14 @@ vionet_enq_rx(struct vionet_dev *dev, ch
> if ((pkt_desc->flags & VRING_DESC_F_WRITE) == 0) {
> log_warnx("unexpected readable rx descriptor %d",
> pkt_desc_idx);
> - free(vr);
> - return (0);
> + goto out;
> }
>
> /* Write packet to descriptor ring */
> if (write_mem(pkt_desc->addr, pkt, sz)) {
> log_warnx("vionet: rx enq packet write_mem error @ "
> "0x%llx", pkt_desc->addr);
> - free(vr);
> - return (0);
> + goto out;
> }
>
> ret = 1;
> @@ -868,9 +848,8 @@ vionet_enq_rx(struct vionet_dev *dev, ch
> if (write_mem(q_gpa + off, &used->idx, sizeof used->idx))
> log_warnx("vionet: error writing vio ring");
> }
> -
> +out:
> free(vr);
> -
> return (ret);
> }
>
>
