From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [PATCH v2] vhost: add interface name to virtio-net struct Date: Fri, 19 Dec 2014 17:02:19 +0100 Message-ID: <2876256.xg5KmeNcVB@xps13> References: <1418926027-19794-1-git-send-email-ciara.loftus@intel.com> <2601191342CEEE43887BDE71AB977258213C2436@IRSMSX105.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Cc: dev-VfR2kkLFssw@public.gmane.org To: "Ananyev, Konstantin" Return-path: In-Reply-To: <2601191342CEEE43887BDE71AB977258213C2436-kPTMFJFq+rEu0RiL9chJVbfspsVTdybXVpNB7YpNyf8@public.gmane.org> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" 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 > > > > 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 > > Signed-off-by: Anthony Fee > > Acked-by: Huawei Xie [...] > > /* > > + * 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