[dpdk-dev] [PATCH v2 1/2] eal: add uevent api for hot plug

[Guo, Jia]

The buf have contain lot of consistent '/0', so it is why I need to check that. anyway I will refine that and other return issue in v3.

Thanks jingjing .

Best regards,
Jeff Guo


-----Original Message-----
From: Wu, Jingjing 
Sent: Thursday, June 29, 2017 10:25 AM
To: Guo, Jia <jia.guo at intel.com>; Zhang, Helin <helin.zhang at intel.com>
Cc: dev at dpdk.org
Subject: RE: [PATCH v2 1/2] eal: add uevent api for hot plug

> +
> +int
> +rte_uevent_connect(void)
> +{
> +	struct sockaddr_nl addr;
> +	int ret;
> +	int netlink_fd = -1;
> +	int size = 64 * 1024;
> +	int nonblock = 1;
> +	memset(&addr, 0, sizeof(addr));
> +	addr.nl_family = AF_NETLINK;
> +	addr.nl_pid = 0;
> +	addr.nl_groups = 0xffffffff;
> +
> +	netlink_fd = socket(PF_NETLINK, SOCK_DGRAM,
> NETLINK_KOBJECT_UEVENT);
> +	if (netlink_fd < 0)
> +		return -1;
> +
> +	RTE_LOG(ERR, EAL,
> +	"netlink_fd is %d\n", netlink_fd);
Is this a ERR log?

> +
> +	setsockopt(netlink_fd, SOL_SOCKET, SO_RCVBUFFORCE, &size, 
> +sizeof(size));
> +
> +	ret = ioctl(netlink_fd, FIONBIO, &nonblock);
> +	if (ret != 0) {
> +		RTE_LOG(ERR, EAL,
> +		"ioctl(FIONBIO) failed\n");
> +		close(netlink_fd);
> +		return -1;
> +	}
> +
> +	if (bind(netlink_fd, (struct sockaddr *) &addr, sizeof(addr)) < 0) {
> +		close(netlink_fd);
> +		return -1;
> +	}
> +
> +	return netlink_fd;
> +}
> +
> +static int
> +parse_event(const char *buf, struct rte_uevent *event) {
> +	char action[RTE_UEVENT_MSG_LEN];
> +	char subsystem[RTE_UEVENT_MSG_LEN];
> +	char dev_path[RTE_UEVENT_MSG_LEN];
> +	int i = 0;
> +
> +	memset(action, 0, RTE_UEVENT_MSG_LEN);
> +	memset(subsystem, 0, RTE_UEVENT_MSG_LEN);
> +	memset(dev_path, 0, RTE_UEVENT_MSG_LEN);
> +
> +	while (*buf && i < RTE_UEVENT_MSG_LEN) {
> +		if (!strncmp(buf, "ACTION=", 7)) {
> +			buf += 7;
> +			snprintf(action, sizeof(action), "%s", buf);
> +		} else if (!strncmp(buf, "DEVPATH=", 8)) {
> +			buf += 8;
> +			snprintf(dev_path, sizeof(dev_path), "%s", buf);
> +		} else if (!strncmp(buf, "SUBSYSTEM=", 10)) {
> +			buf += 10;
> +			snprintf(subsystem, sizeof(subsystem), "%s", buf);
> +		}
> +		while (*buf++)
> +			i++;
> +		while (*buf == '\0') {
> +			buf++;
> +			i++;
> +		}
++ until to the end? The logic looks wrong. Please check carefully.

> +	}
> +
> +	if (!strncmp(subsystem, "uio", 3)) {
> +
> +		event->subsystem = RTE_UEVENT_SUBSYSTEM_UIO;
> +		if (!strncmp(action, "add", 3))
> +			event->action = RTE_UEVENT_ADD;
> +		if (!strncmp(action, "remove", 6))
> +			event->action = RTE_UEVENT_REMOVE;
> +		return 0;
> +	}
> +
> +	return -1;
> +}
> +
> +int
> +rte_uevent_get(int fd, struct rte_uevent *uevent) {
> +	int ret;
> +	char buf[RTE_UEVENT_MSG_LEN];
> +
> +	memset(uevent, 0, sizeof(struct rte_uevent));
> +	memset(buf, 0, RTE_UEVENT_MSG_LEN);
> +
> +	ret = recv(fd, buf, RTE_UEVENT_MSG_LEN - 1, MSG_DONTWAIT);
> +	if (ret > 0)
> +		return parse_event(buf, uevent);
> +
> +	if (ret < 0) {
Meaningless check.
> +		if (errno == EAGAIN || errno == EWOULDBLOCK) {
> +			return 0;
Return -1? The function is declared as 0 means success.

> +		} else {
> +			RTE_LOG(ERR, EAL,
> +			"Socket read error(%d): %s\n",
> +			errno, strerror(errno));
Why not return?

> +		}
> +	}
> +
> +	/* connection closed */
> +	if (ret == 0)
> +		return -1;
> +
> +	return 0;
> +}
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> index fa10329..2fead82 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> @@ -231,6 +231,10 @@
>  		close(dev->intr_handle.uio_cfg_fd);
>  		dev->intr_handle.uio_cfg_fd = -1;
>  	}
> +	if (dev->intr_handle.uevent_fd >= 0) {
> +		close(dev->intr_handle.uevent_fd);
> +		dev->intr_handle.uevent_fd = -1;
> +	}
>  	if (dev->intr_handle.fd >= 0) {
>  		close(dev->intr_handle.fd);
>  		dev->intr_handle.fd = -1;
> @@ -245,6 +249,7 @@
>  	char dirname[PATH_MAX];
>  	char cfgname[PATH_MAX];
>  	char devname[PATH_MAX]; /* contains the /dev/uioX */
> +	char uevtname[PATH_MAX];
>  	int uio_num;
>  	struct rte_pci_addr *loc;
> 
> @@ -276,6 +281,13 @@
>  		goto error;
>  	}
> 
> +	dev->intr_handle.uevent_fd = rte_uevent_connect();
> +	if (dev->intr_handle.uevent_fd < 0) {
> +		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
> +			uevtname, strerror(errno));
> +		goto error;
It seems uevtname is not assigned at all. Do we need it? And the log may means "cannot connect the event fd", right?. And even the event fd is failed to create, should it block the process? 


Thanks
Jingjing