Re: [dpdk-dev] [PATCH v6 1/2] eal: add uevent monitor for hot plug

[Guo, Jia]

On 12/26/2017 2:06 AM, Stephen Hemminger wrote:
On Thu,  2 Nov 2017 04:16:44 +0800
Jeff Guo <jia....@intel.com> wrote:

+int
+rte_dev_bind_driver(const char *dev_name, const char *drv_type) {
Bracket left after declaration.
thanks.


+       snprintf(drv_override_path, sizeof(drv_override_path),
+               "/sys/bus/pci/devices/%s/driver_override", dev_name);
+
+       /* specify the driver for a device by writing to driver_override */
+       drv_override_fd = open(drv_override_path, O_WRONLY);
+       if (drv_override_fd < 0) {
+               RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
+                       drv_override_path, strerror(errno));
+               goto err;
+       }

You should not have dev functions that assume PCI. Please split into common
and bus specific code.


make sense, will modify it into bus specific code.
+static int
+dev_uev_parse(const char *buf, struct rte_eal_uevent *event)
+{
+       char action[RTE_EAL_UEVENT_MSG_LEN];
+       char subsystem[RTE_EAL_UEVENT_MSG_LEN];
+       char dev_path[RTE_EAL_UEVENT_MSG_LEN];
+       char pci_slot_name[RTE_EAL_UEVENT_MSG_LEN];
+       int i = 0;
+
+       memset(action, 0, RTE_EAL_UEVENT_MSG_LEN);
+       memset(subsystem, 0, RTE_EAL_UEVENT_MSG_LEN);
+       memset(dev_path, 0, RTE_EAL_UEVENT_MSG_LEN);
+       memset(pci_slot_name, 0, RTE_EAL_UEVENT_MSG_LEN);
+
+       while (i < RTE_EAL_UEVENT_MSG_LEN) {
+               for (; i < RTE_EAL_UEVENT_MSG_LEN; i++) {
+                       if (*buf)
+                               break;
+                       buf++;
+               }
+               if (!strncmp(buf, "libudev", 7)) {
+                       buf += 7;
+                       i += 7;
+                       event->group = UEV_MONITOR_UDEV;
+               }
+               if (!strncmp(buf, "ACTION=", 7)) {
+                       buf += 7;
+                       i += 7;
+                       snprintf(action, sizeof(action), "%s", buf);
Why snprintf rather than strncpy?

snprintf would no need manual write '\0' and the src length is not 
explicit, and if concern about the efficiency of the snprintf scan, i 
will constrain the value of dest buf length.
+               } else if (!strncmp(buf, "DEVPATH=", 8)) {
+                       buf += 8;
+                       i += 8;
+                       snprintf(dev_path, sizeof(dev_path), "%s", buf);
+               } else if (!strncmp(buf, "SUBSYSTEM=", 10)) {
+                       buf += 10;
+                       i += 10;
+                       snprintf(subsystem, sizeof(subsystem), "%s", buf);
+               } else if (!strncmp(buf, "PCI_SLOT_NAME=", 14)) {
+                       buf += 14;
+                       i += 14;
+                       snprintf(pci_slot_name, sizeof(subsystem), "%s", buf);
+               }
+               for (; i < RTE_EAL_UEVENT_MSG_LEN; i++) {
+                       if (*buf == '\0')
+                               break;
+                       buf++;
+               }
+       }
+
+       if (!strncmp(subsystem, "pci", 3))
+               event->subsystem = UEV_SUBSYSTEM_PCI;
+       if (!strncmp(action, "add", 3))
+               event->type = RTE_EAL_DEV_EVENT_ADD;
+       if (!strncmp(action, "remove", 6))
+               event->type = RTE_EAL_DEV_EVENT_REMOVE;
+       event->devname = pci_slot_name;
Why do you need to first capture the strings, then set state variables?
Instead why not update event->xxx directly?
i think that would be more benefit to read and manage out of the loop.