Re: [PATCH v2] vhost: add interface name to virtio-net struct

[Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>]

Hi Konstantin,

2014-12-19 15:39, Ananyev, Konstantin:
> Hi Ciara,
> 
> > -----Original Message-----
> > From: Loftus, Ciara
> > Sent: Thursday, December 18, 2014 6:07 PM
> > To: dev-VfR2kkLFssw@public.gmane.org
> > Cc: thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org; stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org; vincent.jardin-pdR9zngts4EAvxtiuMwx3w@public.gmane.org; Ananyev, Konstantin; Czesnowicz,
> > Przemyslaw; Loftus, Ciara; Anthony Fee
> > Subject: [PATCH v2] vhost: add interface name to virtio-net struct
> > 
> > From: Ciara Loftus <ciara.loftus-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > 
> > This patch fixes the issue whereby when using userspace vhost ports
> > in the context of vSwitching, the name provided to the hypervisor/QEMU
> > of the vhost tap device needs to be exposed in the library, in order
> > for the vSwitch to be able to direct packets to the correct device.
> > This patch introduces an 'ifname' member to the virtio-net structure
> > which is populated with the tap device name when QEMU is brought up
> > with a vhost device.
> > 
> > Signed-off-by: Ciara Loftus <ciara.loftus-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Signed-off-by: Anthony Fee <anthonyx.fee-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Acked-by: Huawei Xie <huawei.xie-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
[...]
> >  /*
> > + * Function to get the tap device name from the provided file descriptor and
> > + * save it in the device structure.
> > + */
> > +static int
> > +get_ifname(struct virtio_net *dev, int tap_fd, int pid)
> > +{
> > +	struct eventfd_copy fd_tap;
> > +	struct ifreq ifr;
> > +	uint32_t size, ifr_size;
> > +	int ret;
> > +
> > +	fd_tap.source_fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
> > +	fd_tap.target_fd = tap_fd;
> > +	fd_tap.target_pid = pid;
> > +
> > +	if (eventfd_copy(dev, &fd_tap))
> > +		return -1;
> > +
> > +	ret = ioctl(fd_tap.source_fd, TUNGETIFF, &ifr);
> > +
> > +	if (close(fd_tap.source_fd) < 0)
> > +		RTE_LOG(ERR, VHOST_CONFIG,
> > +			"(%"PRIu64") fd close failed\n",
> > +			dev->device_fh);
> > +
> > +	if (ret >= 0) {
> > +		ifr_size = strnlen(ifr.ifr_name, sizeof(ifr.ifr_name));
> > +		size = ifr_size > sizeof(dev->ifname)?
> > +				sizeof(dev->ifname): ifr_size;
> 
> If you saying that  sizeof(ifr.ifr_name) would always be equal to sizeof(dev->ifname),
> then why  are you doing the comparison above?
> "ifr_size <= sizeof(dev->ifname)" would always be true, right?
> From other side, if  if strlen(ifr.ifr_name)  < sizeof(dev->ifname), you wouldn't copy over terminating 0.
> I think it needs to be something like:
> 
> size = strnlen(ifr.ifr_name, sizeof(ifr.ifr_name)) + 1;
> size = RTE_MIN(size, sizeof(dev->ifname));
> strncpy(dev->ifname, ifr.ifr_name, size);
> dev->ifname[sizeof(dev->ifname) - 1] = 0;

This patch has been applied as hot fix.
If you think you can improve it, please submit a patch.
We are releasing in few hours (probably tomorrow morning).

> > +
> > +		strncpy(dev->ifname, ifr.ifr_name, size);
> > +	} else
> > +		RTE_LOG(ERR, VHOST_CONFIG,
> > +			"(%"PRIu64") TUNGETIFF ioctl failed\n",
> > +			dev->device_fh);
> > +
> > +	return 0;
> 
> Shouldn't we return a failure if ret is negative (ioclt failed)? 
> Konstantin