Re: [PATCH v8 7/8] vhost-user: add vhost-user

On Thu, 14 Nov 2024 16:16:48 +0100
Laurent Vivier <lvivier(a)redhat.com> wrote:

...
  On 14/11/2024 15:23, Stefano Brivio wrote:
  On Thu, 14 Nov 2024 11:20:09 +0100
 Laurent Vivier <lvivier(a)redhat.com> wrote:
   
  On 17/10/2024 02:10, Stefano Brivio wrote:  
 > +		if (frame_size == 0)
> +			first = &iov_vu[i + 1];
> +
> +		if (iov_vu[i + 1].iov_len > (size_t)len)
> +			iov_vu[i + 1].iov_len = len;
> +
> +		len -= iov_vu[i + 1].iov_len;
> +		iov_used++;
> +
> +		frame_size += iov_vu[i + 1].iov_len;
> +		num_buffers++;
> +
> +		if (frame_size >= mss || len == 0 ||
> +		    i + 1 == iov_cnt || !vu_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF)) {
> +			if (i + 1 == iov_cnt)
> +				check = NULL;
> +
> +			/* restore first iovec base: point to vnet header */
> +			vu_set_vnethdr(vdev, first, num_buffers, l2_hdrlen);
> +
> +			tcp_vu_prepare(c, conn, first, frame_size, &check);
> +			if (*c->pcap)  {
> +				tcp_vu_update_check(tapside, first, num_buffers);
> +				pcap_iov(first, num_buffers,
> +					 sizeof(struct virtio_net_hdr_mrg_rxbuf));
> +			}
> +
> +			conn->seq_to_tap += frame_size;  
 We always increase this, even if, later...
      
> +
> +			frame_size = 0;
> +			num_buffers = 0;
> +		}
> +	}
> +
> +	/* release unused buffers */
> +	vu_queue_rewind(vq, iov_cnt - iov_used);
> +
> +	/* send packets */
> +	vu_flush(vdev, vq, elem, iov_used);  
 we fail to send packets, that is, even if vu_queue_fill_by_index()
 returns early because (!vq->vring.avail).   
 vring.avail is a pointer to a structure. vring.avail is NULL if there is something wrong
 during the initialization. It's imported code, I think it's only a sanity
check.
 So in theory vu_flush() cannot fail.   
 
 Oh, I see now. I actually think it's preferable to crash in that
 (theoretically impossible) case, without even an ASSERT() (we would
 dereference a NULL pointer, eventually, even if not here). 
 
 So what you propose is to remove the "if (!vq->vring.avail) return;"?

Yes, all of them, actually. It also avoids confusion I think.

-- 
Stefano