[PATCH rdma-core 0/5] Small fixes for verbs

[PATCH rdma-core 0/5] Small fixes for verbs

[Jason Gunthorpe]

This is a precursor to a larger series reworking how the providers
are loaded.

The big item is switching verbs fully to use ccan lists and using the new
ccan functions to rework how the sysfs list is merged into the device list.

Jason Gunthorpe (5):
  verbs: Remove diagnostic ignored "-Wmissing-prototypes"
  util: Fix check_snprintf to use __rc__ for local
  verbs: Use ccan list.h instead of open coding lists
  verbs: Tidy up ibverbs_get_device_list
  ocrdma: Remove ocrdma_dev_list

 libibverbs/init.c              | 145 ++++++++++++++++++++---------------------
 providers/ocrdma/ocrdma_main.c |   7 --
 providers/ocrdma/ocrdma_main.h |   1 -
 util/util.h                    |   4 +-
 4 files changed, 73 insertions(+), 84 deletions(-)

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-core 1/5] verbs: Remove diagnostic ignored "-Wmissing-prototypes"

[Jason Gunthorpe]

When the symbol version function __ibv_register_driver
was deleted the pragma was accidently left behind.

Fixes: 6843def955d0 ("verbs: Remove ibv_register_driver")
Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
---
 libibverbs/init.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/libibverbs/init.c b/libibverbs/init.c
index 4d064bc6cdf213..e0fa4caae6f125 100644
--- a/libibverbs/init.c
+++ b/libibverbs/init.c
@@ -50,8 +50,6 @@
 #include <util/util.h>
 #include "ibverbs.h"
 
-#pragma GCC diagnostic ignored "-Wmissing-prototypes"
-
 int abi_ver;
 
 struct ibv_sysfs_dev {
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-core 2/5] util: Fix check_snprintf to use __rc__ for local

[Jason Gunthorpe]

To avoid clashing with user variables.

Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
---
 util/util.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/util/util.h b/util/util.h
index cbf0deca2dde7b..30d32ee6b51e24 100644
--- a/util/util.h
+++ b/util/util.h
@@ -8,8 +8,8 @@
  * error */
 #define check_snprintf(buf, len, fmt, ...)                                     \
 	({                                                                     \
-		int rc = snprintf(buf, len, fmt, ##__VA_ARGS__);               \
-		(rc < len && rc >= 0);                                         \
+		int __rc__ = snprintf(buf, len, fmt, ##__VA_ARGS__);           \
+		(__rc__ < len && __rc__ >= 0);                                 \
 	})
 
 /* a CMP b. See also the BSD macro timercmp(). */
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-core 3/5] verbs: Use ccan list.h instead of open coding lists

[Jason Gunthorpe]

For ibv_sysfs_dev, ibv_driver_name and ibv_driver internal structs. ccan
lists are easier to understand and use than the open coded versions.

Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
---
 libibverbs/init.c | 62 +++++++++++++++++++++++++------------------------------
 1 file changed, 28 insertions(+), 34 deletions(-)

diff --git a/libibverbs/init.c b/libibverbs/init.c
index e0fa4caae6f125..91ce8b4e378458 100644
--- a/libibverbs/init.c
+++ b/libibverbs/init.c
@@ -47,17 +47,18 @@
 #include <errno.h>
 #include <assert.h>
 
+#include <ccan/list.h>
 #include <util/util.h>
 #include "ibverbs.h"
 
 int abi_ver;
 
 struct ibv_sysfs_dev {
+	struct list_node	entry;
 	char		        sysfs_name[IBV_SYSFS_NAME_MAX];
 	char		        ibdev_name[IBV_SYSFS_NAME_MAX];
 	char		        sysfs_path[IBV_SYSFS_PATH_MAX];
 	char		        ibdev_path[IBV_SYSFS_PATH_MAX];
-	struct ibv_sysfs_dev   *next;
 	int			abi_ver;
 	int			have_driver;
 	struct timespec		time_created;
@@ -65,20 +66,20 @@ struct ibv_sysfs_dev {
 };
 
 struct ibv_driver_name {
+	struct list_node	entry;
 	char		       *name;
-	struct ibv_driver_name *next;
 };
 
 struct ibv_driver {
+	struct list_node	entry;
 	const char	       *name;
 	verbs_driver_init_func	verbs_init_func;
-	struct ibv_driver      *next;
 };
 
-static struct ibv_driver_name *driver_name_list;
-static struct ibv_driver *head_driver, *tail_driver;
+static LIST_HEAD(driver_name_list);
+static LIST_HEAD(driver_list);
 
-static int find_sysfs_devs(struct ibv_sysfs_dev **tmp_sysfs_dev_list)
+static int find_sysfs_devs(struct list_head *tmp_sysfs_dev_list)
 {
 	char class_path[IBV_SYSFS_PATH_MAX];
 	DIR *class_dir;
@@ -147,7 +148,6 @@ static int find_sysfs_devs(struct ibv_sysfs_dev **tmp_sysfs_dev_list)
 
 		sysfs_dev->time_created = buf.st_mtim;
 
-		sysfs_dev->next        = *tmp_sysfs_dev_list;
 		sysfs_dev->have_driver = 0;
 		sysfs_dev->used = 0;
 		if (ibv_read_sysfs_file(sysfs_dev->sysfs_path, "abi_version",
@@ -156,7 +156,8 @@ static int find_sysfs_devs(struct ibv_sysfs_dev **tmp_sysfs_dev_list)
 		else
 			sysfs_dev->abi_ver = 0;
 
-		*tmp_sysfs_dev_list = sysfs_dev;
+		list_node_init(&sysfs_dev->entry);
+		list_add(tmp_sysfs_dev_list, &sysfs_dev->entry);
 		sysfs_dev      = NULL;
 	}
 
@@ -181,13 +182,9 @@ void verbs_register_driver(const char *name,
 
 	driver->name            = name;
 	driver->verbs_init_func = verbs_init_func;
-	driver->next            = NULL;
 
-	if (tail_driver)
-		tail_driver->next = driver;
-	else
-		head_driver = driver;
-	tail_driver = driver;
+	list_node_init(&driver->entry);
+	list_add_tail(&driver_list, &driver->entry);
 }
 
 static void load_driver(const char *name)
@@ -261,9 +258,7 @@ static void load_drivers(void)
 		}
 	}
 
-	for (name = driver_name_list, next_name = name ? name->next : NULL;
-	     name;
-	     name = next_name, next_name = name ? name->next : NULL) {
+	list_for_each_safe(&driver_name_list, name, next_name, entry) {
 		load_driver(name->name);
 		free(name->name);
 		free(name);
@@ -314,8 +309,8 @@ static void read_config_file(const char *path)
 				continue;
 			}
 
-			driver_name->next = driver_name_list;
-			driver_name_list  = driver_name;
+			list_node_init(&driver_name->entry);
+			list_add(&driver_name_list, &driver_name->entry);
 		} else
 			fprintf(stderr, PFX "Warning: ignoring bad config directive "
 				"'%s' in file '%s'.\n", field, path);
@@ -427,7 +422,7 @@ static struct verbs_device *try_drivers(struct ibv_sysfs_dev *sysfs_dev)
 	struct ibv_driver *driver;
 	struct verbs_device *dev;
 
-	for (driver = head_driver; driver; driver = driver->next) {
+	list_for_each(&driver_list, driver, entry) {
 		dev = try_driver(driver, sysfs_dev);
 		if (dev)
 			return dev;
@@ -488,7 +483,8 @@ static int same_sysfs_dev(struct ibv_sysfs_dev *sysfs1,
 
 int ibverbs_get_device_list(struct list_head *list)
 {
-	struct ibv_sysfs_dev *tmp_sysfs_dev_list = NULL, *sysfs_dev, *next_dev;
+	LIST_HEAD(tmp_sysfs_dev_list);
+	struct ibv_sysfs_dev *sysfs_dev, *next_dev;
 	struct verbs_device *vdev, *tmp;
 	static int drivers_loaded;
 	int num_devices = 0;
@@ -501,23 +497,25 @@ int ibverbs_get_device_list(struct list_head *list)
 		return -ret;
 
 	list_for_each_safe(list, vdev, tmp, entry) {
-		for (sysfs_dev = tmp_sysfs_dev_list; sysfs_dev; sysfs_dev =
-		     sysfs_dev->next) {
+		struct ibv_sysfs_dev *old_sysfs = NULL;
+
+		list_for_each(&tmp_sysfs_dev_list, sysfs_dev, entry) {
 			if (same_sysfs_dev(vdev->sysfs, sysfs_dev)) {
-				sysfs_dev->have_driver = 1;
-				num_devices++;
+				old_sysfs = sysfs_dev;
 				break;
 			}
 		}
 
-		if (!sysfs_dev) {
+		if (old_sysfs) {
+			sysfs_dev->have_driver = 1;
+			num_devices++;
+		} else {
 			list_del(&vdev->entry);
 			ibverbs_device_put(&vdev->device);
 		}
 	}
 
-	for (sysfs_dev = tmp_sysfs_dev_list; sysfs_dev; sysfs_dev =
-	     sysfs_dev->next) {
+	list_for_each(&tmp_sysfs_dev_list, sysfs_dev, entry) {
 		if (sysfs_dev->have_driver)
 			continue;
 		vdev = try_drivers(sysfs_dev);
@@ -555,8 +553,7 @@ int ibverbs_get_device_list(struct list_head *list)
 	load_drivers();
 	drivers_loaded = 1;
 
-	for (sysfs_dev = tmp_sysfs_dev_list; sysfs_dev; sysfs_dev =
-	     sysfs_dev->next) {
+	list_for_each(&tmp_sysfs_dev_list, sysfs_dev, entry) {
 		if (sysfs_dev->have_driver)
 			continue;
 
@@ -569,10 +566,7 @@ int ibverbs_get_device_list(struct list_head *list)
 	}
 
 out:
-	for (sysfs_dev = tmp_sysfs_dev_list,
-	     next_dev = sysfs_dev ? sysfs_dev->next : NULL;
-	     sysfs_dev;
-	     sysfs_dev = next_dev, next_dev = sysfs_dev ? sysfs_dev->next : NULL) {
+	list_for_each_safe(&tmp_sysfs_dev_list, sysfs_dev, next_dev, entry) {
 		if (!sysfs_dev->have_driver && getenv("IBV_SHOW_WARNINGS")) {
 			fprintf(stderr, PFX "Warning: no userspace device-specific "
 				"driver found for %s\n", sysfs_dev->sysfs_path);
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-core 4/5] verbs: Tidy up ibverbs_get_device_list

[Jason Gunthorpe]

Now that we have ccan lists the logic here can be simplified greatly.
Eliminate the confusing used and have_driver values, instead just
delete items from the sysfs list as we run down the process. This
directly guarantees discovered sysfs items are handled only once,
and makes the lifetime of the sysfs pointers much clearer.

Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
---
 libibverbs/init.c | 93 +++++++++++++++++++++++++++++--------------------------
 1 file changed, 49 insertions(+), 44 deletions(-)

diff --git a/libibverbs/init.c b/libibverbs/init.c
index 91ce8b4e378458..04583bb5137c96 100644
--- a/libibverbs/init.c
+++ b/libibverbs/init.c
@@ -60,9 +60,7 @@ struct ibv_sysfs_dev {
 	char		        sysfs_path[IBV_SYSFS_PATH_MAX];
 	char		        ibdev_path[IBV_SYSFS_PATH_MAX];
 	int			abi_ver;
-	int			have_driver;
 	struct timespec		time_created;
-	int			used;
 };
 
 struct ibv_driver_name {
@@ -148,8 +146,6 @@ static int find_sysfs_devs(struct list_head *tmp_sysfs_dev_list)
 
 		sysfs_dev->time_created = buf.st_mtim;
 
-		sysfs_dev->have_driver = 0;
-		sysfs_dev->used = 0;
 		if (ibv_read_sysfs_file(sysfs_dev->sysfs_path, "abi_version",
 					value, sizeof value) > 0)
 			sysfs_dev->abi_ver = strtol(value, NULL, 10);
@@ -412,7 +408,6 @@ static struct verbs_device *try_driver(struct ibv_driver *driver,
 	strcpy(dev->name,       sysfs_dev->ibdev_name);
 	strcpy(dev->ibdev_path, sysfs_dev->ibdev_path);
 	vdev->sysfs = sysfs_dev;
-	sysfs_dev->used = 1;
 
 	return vdev;
 }
@@ -481,25 +476,51 @@ static int same_sysfs_dev(struct ibv_sysfs_dev *sysfs1,
 	return 0;
 }
 
-int ibverbs_get_device_list(struct list_head *list)
+/* Match every ibv_sysfs_dev in the sysfs_list to a driver and add a new entry
+ * to device_list. Once matched to a driver the entry in sysfs_list is
+ * removed.
+ */
+static void try_all_drivers(struct list_head *sysfs_list,
+			    struct list_head *device_list,
+			    unsigned int *num_devices)
+{
+	struct ibv_sysfs_dev *sysfs_dev;
+	struct ibv_sysfs_dev *tmp;
+	struct verbs_device *vdev;
+
+	list_for_each_safe(sysfs_list, sysfs_dev, tmp, entry) {
+		vdev = try_drivers(sysfs_dev);
+		if (vdev) {
+			list_del(&sysfs_dev->entry);
+			// Ownership of sysfs_dev moves into vdev->sysfs
+			list_add(device_list, &vdev->entry);
+			(*num_devices)++;
+		}
+	}
+}
+
+int ibverbs_get_device_list(struct list_head *device_list)
 {
-	LIST_HEAD(tmp_sysfs_dev_list);
+	LIST_HEAD(sysfs_list);
 	struct ibv_sysfs_dev *sysfs_dev, *next_dev;
 	struct verbs_device *vdev, *tmp;
 	static int drivers_loaded;
-	int num_devices = 0;
+	unsigned int num_devices = 0;
 	int statically_linked = 0;
-	int no_driver = 0;
 	int ret;
 
-	ret = find_sysfs_devs(&tmp_sysfs_dev_list);
+	ret = find_sysfs_devs(&sysfs_list);
 	if (ret)
 		return -ret;
 
-	list_for_each_safe(list, vdev, tmp, entry) {
+	/* Remove entries from the sysfs_list that are already preset in the
+	 * device_list, and remove entries from the device_list that are not
+	 * present in the sysfs_list.
+	 */
+	list_for_each_safe(device_list, vdev, tmp, entry) {
 		struct ibv_sysfs_dev *old_sysfs = NULL;
 
-		list_for_each(&tmp_sysfs_dev_list, sysfs_dev, entry) {
+		list_for_each(&sysfs_list, sysfs_dev, entry) {
 			if (same_sysfs_dev(vdev->sysfs, sysfs_dev)) {
 				old_sysfs = sysfs_dev;
 				break;
@@ -507,7 +528,8 @@ int ibverbs_get_device_list(struct list_head *list)
 		}
 
 		if (old_sysfs) {
-			sysfs_dev->have_driver = 1;
+			list_del(&old_sysfs->entry);
+			free(old_sysfs);
 			num_devices++;
 		} else {
 			list_del(&vdev->entry);
@@ -515,19 +537,9 @@ int ibverbs_get_device_list(struct list_head *list)
 		}
 	}
 
-	list_for_each(&tmp_sysfs_dev_list, sysfs_dev, entry) {
-		if (sysfs_dev->have_driver)
-			continue;
-		vdev = try_drivers(sysfs_dev);
-		if (vdev) {
-			sysfs_dev->have_driver = 1;
-			list_add(list, &vdev->entry);
-			num_devices++;
-		} else
-			no_driver = 1;
-	}
+	try_all_drivers(&sysfs_list, device_list, &num_devices);
 
-	if (!no_driver || drivers_loaded)
+	if (list_empty(&sysfs_list) || drivers_loaded)
 		goto out;
 
 	/*
@@ -553,29 +565,22 @@ int ibverbs_get_device_list(struct list_head *list)
 	load_drivers();
 	drivers_loaded = 1;
 
-	list_for_each(&tmp_sysfs_dev_list, sysfs_dev, entry) {
-		if (sysfs_dev->have_driver)
-			continue;
-
-		vdev = try_drivers(sysfs_dev);
-		if (vdev) {
-			sysfs_dev->have_driver = 1;
-			list_add(list, &vdev->entry);
-			num_devices++;
-		}
-	}
+	try_all_drivers(&sysfs_list, device_list, &num_devices);
 
 out:
-	list_for_each_safe(&tmp_sysfs_dev_list, sysfs_dev, next_dev, entry) {
-		if (!sysfs_dev->have_driver && getenv("IBV_SHOW_WARNINGS")) {
-			fprintf(stderr, PFX "Warning: no userspace device-specific "
-				"driver found for %s\n", sysfs_dev->sysfs_path);
+	/* Anything left in sysfs_list was not assoicated with a
+	 * driver.
+	 */
+	list_for_each_safe(&sysfs_list, sysfs_dev, next_dev, entry) {
+		if (getenv("IBV_SHOW_WARNINGS")) {
+			fprintf(stderr, PFX
+				"Warning: no userspace device-specific driver found for %s\n",
+				sysfs_dev->sysfs_path);
 			if (statically_linked)
-				fprintf(stderr, "	When linking libibverbs statically, "
-					"driver must be statically linked too.\n");
+				fprintf(stderr,
+					"	When linking libibverbs statically, driver must be statically linked too.\n");
 		}
-		if (!sysfs_dev->used)
-			free(sysfs_dev);
+		free(sysfs_dev);
 	}
 
 	return num_devices;
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-core 5/5] ocrdma: Remove ocrdma_dev_list

[Jason Gunthorpe]

Nothing ever reads this.

Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
---
 providers/ocrdma/ocrdma_main.c | 7 -------
 providers/ocrdma/ocrdma_main.h | 1 -
 2 files changed, 8 deletions(-)

diff --git a/providers/ocrdma/ocrdma_main.c b/providers/ocrdma/ocrdma_main.c
index 591630677461cf..8888067761ab37 100644
--- a/providers/ocrdma/ocrdma_main.c
+++ b/providers/ocrdma/ocrdma_main.c
@@ -66,9 +66,6 @@ static struct {
 	UCNA(EMULEX, GEN1), UCNA(EMULEX, GEN2), UCNA(EMULEX, GEN2_VF)
 };
 
-static LIST_HEAD(ocrdma_dev_list);
-static pthread_mutex_t ocrdma_dev_list_lock = PTHREAD_MUTEX_INITIALIZER;
-
 static struct ibv_context *ocrdma_alloc_context(struct ibv_device *, int);
 static void ocrdma_free_context(struct ibv_context *);
 
@@ -229,10 +226,6 @@ found:
 	pthread_mutex_init(&dev->dev_lock, NULL);
 	pthread_spin_init(&dev->flush_q_lock, PTHREAD_PROCESS_PRIVATE);
 	dev->ibv_dev.ops = &ocrdma_dev_ops;
-	list_node_init(&dev->entry);
-	pthread_mutex_lock(&ocrdma_dev_list_lock);
-	list_add_tail(&ocrdma_dev_list, &dev->entry);
-	pthread_mutex_unlock(&ocrdma_dev_list_lock);
 	return &dev->ibv_dev;
 qp_err:
 	free(dev);
diff --git a/providers/ocrdma/ocrdma_main.h b/providers/ocrdma/ocrdma_main.h
index b2b27abdcabfd1..05bb12435fb1cc 100644
--- a/providers/ocrdma/ocrdma_main.h
+++ b/providers/ocrdma/ocrdma_main.h
@@ -58,7 +58,6 @@ struct ocrdma_device {
 	struct ocrdma_qp **qp_tbl;
 	pthread_mutex_t dev_lock;
 	pthread_spinlock_t flush_q_lock;
-	struct list_node entry;
 	int id;
 	int gen;
 	uint32_t wqe_size;
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core 2/5] util: Fix check_snprintf to use __rc__ for local

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1306 bytes --]

On Thu, Aug 31, 2017 at 02:50:56PM -0600, Jason Gunthorpe wrote:
> To avoid clashing with user variables.
>
> Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> ---
>  util/util.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/util/util.h b/util/util.h
> index cbf0deca2dde7b..30d32ee6b51e24 100644
> --- a/util/util.h
> +++ b/util/util.h
> @@ -8,8 +8,8 @@
>   * error */
>  #define check_snprintf(buf, len, fmt, ...)                                     \
>  	({                                                                     \
> -		int rc = snprintf(buf, len, fmt, ##__VA_ARGS__);               \
> -		(rc < len && rc >= 0);                                         \
> +		int __rc__ = snprintf(buf, len, fmt, ##__VA_ARGS__);           \

It can't clash, because it is declared in the scope {..} of that define and
it is local to check_snprintf macro.

> +		(__rc__ < len && __rc__ >= 0);                                 \
>  	})
>
>  /* a CMP b. See also the BSD macro timercmp(). */
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-core 2/5] util: Fix check_snprintf to use __rc__ for local

[Nicolas Morey-Chaisemartin]

Le 01/09/2017 à 06:56, Leon Romanovsky a écrit :
> On Thu, Aug 31, 2017 at 02:50:56PM -0600, Jason Gunthorpe wrote:
>> To avoid clashing with user variables.
>>
>> Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
>> ---
>>  util/util.h | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/util/util.h b/util/util.h
>> index cbf0deca2dde7b..30d32ee6b51e24 100644
>> --- a/util/util.h
>> +++ b/util/util.h
>> @@ -8,8 +8,8 @@
>>   * error */
>>  #define check_snprintf(buf, len, fmt, ...)                                     \
>>  	({                                                                     \
>> -		int rc = snprintf(buf, len, fmt, ##__VA_ARGS__);               \
>> -		(rc < len && rc >= 0);                                         \
>> +		int __rc__ = snprintf(buf, len, fmt, ##__VA_ARGS__);           \
> It can't clash, because it is declared in the scope {..} of that define and
> it is local to check_snprintf macro.

It does if any of the va-args is called rc.
This is "valid" C although not sure what printfs does print here (probably random stack value)
{
 int i = printf("i = %d\n", i);
}

Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core 3/5] verbs: Use ccan list.h instead of open coding lists

[Nicolas Morey-Chaisemartin]

Le 31/08/2017 à 22:50, Jason Gunthorpe a écrit :
> For ibv_sysfs_dev, ibv_driver_name and ibv_driver internal structs. ccan
> lists are easier to understand and use than the open coded versions.
>
> Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> ---
>  libibverbs/init.c | 62 +++++++++++++++++++++++++------------------------------
>  1 file changed, 28 insertions(+), 34 deletions(-)
>
> diff --git a/libibverbs/init.c b/libibverbs/init.c
[...]
> @@ -156,7 +156,8 @@ static int find_sysfs_devs(struct ibv_sysfs_dev **tmp_sysfs_dev_list)
>  		else
>  			sysfs_dev->abi_ver = 0;
>  
> -		*tmp_sysfs_dev_list = sysfs_dev;
> +		list_node_init(&sysfs_dev->entry);
> +		list_add(tmp_sysfs_dev_list, &sysfs_dev->entry);

I don't think you need to init the node.
According to the header file: " * The new list_node does not need to be initialized; it will be overwritten."

>  		sysfs_dev      = NULL;
>  	}
>  
> @@ -181,13 +182,9 @@ void verbs_register_driver(const char *name,
>  
>  	driver->name            = name;
>  	driver->verbs_init_func = verbs_init_func;
> -	driver->next            = NULL;
>  
> -	if (tail_driver)
> -		tail_driver->next = driver;
> -	else
> -		head_driver = driver;
> -	tail_driver = driver;
> +	list_node_init(&driver->entry);
> +	list_add_tail(&driver_list, &driver->entry);

Same here

>  }
>  
>  static void load_driver(const char *name)
> @@ -261,9 +258,7 @@ static void load_drivers(void)
>  		}
>  	}
>  
> -	for (name = driver_name_list, next_name = name ? name->next : NULL;
> -	     name;
> -	     name = next_name, next_name = name ? name->next : NULL) {
> +	list_for_each_safe(&driver_name_list, name, next_name, entry) {
>  		load_driver(name->name);
>  		free(name->name);
>  		free(name);
> @@ -314,8 +309,8 @@ static void read_config_file(const char *path)
>  				continue;
>  			}
>  
> -			driver_name->next = driver_name_list;
> -			driver_name_list  = driver_name;
> +			list_node_init(&driver_name->entry);
> +			list_add(&driver_name_list, &driver_name->entry);

And here


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core 2/5] util: Fix check_snprintf to use __rc__ for local

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1457 bytes --]

On Fri, Sep 01, 2017 at 09:00:01AM +0200, Nicolas Morey-Chaisemartin wrote:
>
>
> Le 01/09/2017 à 06:56, Leon Romanovsky a écrit :
> > On Thu, Aug 31, 2017 at 02:50:56PM -0600, Jason Gunthorpe wrote:
> >> To avoid clashing with user variables.
> >>
> >> Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> >> ---
> >>  util/util.h | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/util/util.h b/util/util.h
> >> index cbf0deca2dde7b..30d32ee6b51e24 100644
> >> --- a/util/util.h
> >> +++ b/util/util.h
> >> @@ -8,8 +8,8 @@
> >>   * error */
> >>  #define check_snprintf(buf, len, fmt, ...)                                     \
> >>  	({                                                                     \
> >> -		int rc = snprintf(buf, len, fmt, ##__VA_ARGS__);               \
> >> -		(rc < len && rc >= 0);                                         \
> >> +		int __rc__ = snprintf(buf, len, fmt, ##__VA_ARGS__);           \
> > It can't clash, because it is declared in the scope {..} of that define and
> > it is local to check_snprintf macro.
>
> It does if any of the va-args is called rc.
> This is "valid" C although not sure what printfs does print here (probably random stack value)
> {
>  int i = printf("i = %d\n", i);
> }

So how does this rename solve the issue?
Now, we can clash with __rc__ variable.

Thanks

>
> Nicolas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-core 2/5] util: Fix check_snprintf to use __rc__ for local

[Nicolas Morey-Chaisemartin]

Le 01/09/2017 à 11:35, Leon Romanovsky a écrit :
> On Fri, Sep 01, 2017 at 09:00:01AM +0200, Nicolas Morey-Chaisemartin wrote:
>>
>> Le 01/09/2017 à 06:56, Leon Romanovsky a écrit :
>>> On Thu, Aug 31, 2017 at 02:50:56PM -0600, Jason Gunthorpe wrote:
>>>> To avoid clashing with user variables.
>>>>
>>>> Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
>>>> ---
>>>>  util/util.h | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/util/util.h b/util/util.h
>>>> index cbf0deca2dde7b..30d32ee6b51e24 100644
>>>> --- a/util/util.h
>>>> +++ b/util/util.h
>>>> @@ -8,8 +8,8 @@
>>>>   * error */
>>>>  #define check_snprintf(buf, len, fmt, ...)                                     \
>>>>  	({                                                                     \
>>>> -		int rc = snprintf(buf, len, fmt, ##__VA_ARGS__);               \
>>>> -		(rc < len && rc >= 0);                                         \
>>>> +		int __rc__ = snprintf(buf, len, fmt, ##__VA_ARGS__);           \
>>> It can't clash, because it is declared in the scope {..} of that define and
>>> it is local to check_snprintf macro.
>> It does if any of the va-args is called rc.
>> This is "valid" C although not sure what printfs does print here (probably random stack value)
>> {
>>  int i = printf("i = %d\n", i);
>> }
> So how does this rename solve the issue?
> Now, we can clash with __rc__ variable.
>
> Thanks

There is no safe way to handle that. It can be detected with -Wshadow though.
But rc is quite a common name (39 definitions in the code base), __rc__ much less (unused... yet)


If you want some real fun, try writing macros with local variables that can be called recursively ;)

Nicolas

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core 2/5] util: Fix check_snprintf to use __rc__ for local

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2424 bytes --]

On Fri, Sep 01, 2017 at 11:46:02AM +0200, Nicolas Morey-Chaisemartin wrote:
>
>
> Le 01/09/2017 à 11:35, Leon Romanovsky a écrit :
> > On Fri, Sep 01, 2017 at 09:00:01AM +0200, Nicolas Morey-Chaisemartin wrote:
> >>
> >> Le 01/09/2017 à 06:56, Leon Romanovsky a écrit :
> >>> On Thu, Aug 31, 2017 at 02:50:56PM -0600, Jason Gunthorpe wrote:
> >>>> To avoid clashing with user variables.
> >>>>
> >>>> Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> >>>> ---
> >>>>  util/util.h | 4 ++--
> >>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/util/util.h b/util/util.h
> >>>> index cbf0deca2dde7b..30d32ee6b51e24 100644
> >>>> --- a/util/util.h
> >>>> +++ b/util/util.h
> >>>> @@ -8,8 +8,8 @@
> >>>>   * error */
> >>>>  #define check_snprintf(buf, len, fmt, ...)                                     \
> >>>>  	({                                                                     \
> >>>> -		int rc = snprintf(buf, len, fmt, ##__VA_ARGS__);               \
> >>>> -		(rc < len && rc >= 0);                                         \
> >>>> +		int __rc__ = snprintf(buf, len, fmt, ##__VA_ARGS__);           \
> >>> It can't clash, because it is declared in the scope {..} of that define and
> >>> it is local to check_snprintf macro.
> >> It does if any of the va-args is called rc.
> >> This is "valid" C although not sure what printfs does print here (probably random stack value)
> >> {
> >>  int i = printf("i = %d\n", i);
> >> }
> > So how does this rename solve the issue?
> > Now, we can clash with __rc__ variable.
> >
> > Thanks
>
> There is no safe way to handle that. It can be detected with -Wshadow though.
> But rc is quite a common name (39 definitions in the code base), __rc__ much less (unused... yet)

Or commit message should reflect that it is not fix, but attempt to
reduce name collision, or the patch should provide real fix. For
example, create inline function and convert all 5 callers to use it
without ##__VA_ARGS__.

>
>
> If you want some real fun, try writing macros with local variables that can be called recursively ;)
>
> Nicolas
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-core 2/5] util: Fix check_snprintf to use __rc__ for local

[Jason Gunthorpe]

On Fri, Sep 01, 2017 at 07:56:32AM +0300, Leon Romanovsky wrote:
> On Thu, Aug 31, 2017 at 02:50:56PM -0600, Jason Gunthorpe wrote:
> > To avoid clashing with user variables.
> >
> > Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> >  util/util.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/util/util.h b/util/util.h
> > index cbf0deca2dde7b..30d32ee6b51e24 100644
> > +++ b/util/util.h
> > @@ -8,8 +8,8 @@
> >   * error */
> >  #define check_snprintf(buf, len, fmt, ...)                                     \
> >  	({                                                                     \
> > -		int rc = snprintf(buf, len, fmt, ##__VA_ARGS__);               \
> > -		(rc < len && rc >= 0);                                         \
> > +		int __rc__ = snprintf(buf, len, fmt, ##__VA_ARGS__);           \
> 
> It can't clash, because it is declared in the scope {..} of that define and
> it is local to check_snprintf macro.

This triggers a -Wshadow warning:

 int rc;
 check_snprinf(str, sizeof(str), ..)

And rc is commonly used in callers, so it shouldn't.

Perhaps this addresses all the concerns:

/* Return true if the snprintf succeeded, false if there was truncation or
 * error */
static inline bool __good_snprintf(size_t len, int rc)
{
	return (rc < len && rc >= 0);
}

#define check_snprintf(buf, len, fmt, ...)    \
	__good_snprintf(len, snprintf(buf, len, fmt, ##__VA_ARGS__))

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core 2/5] util: Fix check_snprintf to use __rc__ for local

[Bart Van Assche]

On Fri, 2017-09-01 at 09:00 -0600, Jason Gunthorpe wrote:
> On Fri, Sep 01, 2017 at 07:56:32AM +0300, Leon Romanovsky wrote:
> > On Thu, Aug 31, 2017 at 02:50:56PM -0600, Jason Gunthorpe wrote:
> > > To avoid clashing with user variables.
> > > 
> > > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > >  util/util.h | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/util/util.h b/util/util.h
> > > index cbf0deca2dde7b..30d32ee6b51e24 100644
> > > +++ b/util/util.h
> > > @@ -8,8 +8,8 @@
> > >   * error */
> > >  #define check_snprintf(buf, len, fmt, ...)                                     \
> > >  	({                                                                     \
> > > -		int rc = snprintf(buf, len, fmt, ##__VA_ARGS__);               \
> > > -		(rc < len && rc >= 0);                                         \
> > > +		int __rc__ = snprintf(buf, len, fmt, ##__VA_ARGS__);           \
> > 
> > It can't clash, because it is declared in the scope {..} of that define and
> > it is local to check_snprintf macro.
> 
> This triggers a -Wshadow warning:
> 
>  int rc;
>  check_snprinf(str, sizeof(str), ..)
> 
> And rc is commonly used in callers, so it shouldn't.
> 
> Perhaps this addresses all the concerns:
> 
> /* Return true if the snprintf succeeded, false if there was truncation or
>  * error */
> static inline bool __good_snprintf(size_t len, int rc)
> {
> 	return (rc < len && rc >= 0);
> }
> 
> #define check_snprintf(buf, len, fmt, ...)    \
> 	__good_snprintf(len, snprintf(buf, len, fmt, ##__VA_ARGS__))

If an inline function will be introduced anyway, why not to make check_snprintf()
itself an inline function and use vsnprintf() instead of snprintf()?

Bart.

Re: [PATCH rdma-core 2/5] util: Fix check_snprintf to use __rc__ for local

[Jason Gunthorpe]

On Fri, Sep 01, 2017 at 03:13:20PM +0000, Bart Van Assche wrote:

> > #define check_snprintf(buf, len, fmt, ...)    \
> > 	__good_snprintf(len, snprintf(buf, len, fmt, ##__VA_ARGS__))
> 
> If an inline function will be introduced anyway, why not to make check_snprintf()
> itself an inline function and use vsnprintf() instead of snprintf()?

Well.. Generally speaking I prefer to avoid wrappering 'magic'
functions like snprintf with inlines because you loose the 'magic'.

eg, however it is done, modern gcc triggers -Wformat-length analysis
for snprintf, and if you wrapper it with an inline it appears that
analysis is lost. This is separate from the well known
attribute((printf)).

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core 3/5] verbs: Use ccan list.h instead of open coding lists

[Jason Gunthorpe]

On Fri, Sep 01, 2017 at 09:05:33AM +0200, Nicolas Morey-Chaisemartin wrote:
> > For ibv_sysfs_dev, ibv_driver_name and ibv_driver internal structs. ccan
> > lists are easier to understand and use than the open coded versions.
> >
> > Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> >  libibverbs/init.c | 62 +++++++++++++++++++++++++------------------------------
> >  1 file changed, 28 insertions(+), 34 deletions(-)
> >
> > diff --git a/libibverbs/init.c b/libibverbs/init.c
> [...]
> > @@ -156,7 +156,8 @@ static int find_sysfs_devs(struct ibv_sysfs_dev **tmp_sysfs_dev_list)
> >  		else
> >  			sysfs_dev->abi_ver = 0;
> >  
> > -		*tmp_sysfs_dev_list = sysfs_dev;
> > +		list_node_init(&sysfs_dev->entry);
> > +		list_add(tmp_sysfs_dev_list, &sysfs_dev->entry);
> 
> I don't think you need to init the node.
> According to the header file: " * The new list_node does not need to be initialized; it will be overwritten."

Right you are, all fixed.

Thanks,
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core 4/5] verbs: Tidy up ibverbs_get_device_list

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 6121 bytes --]

On Thu, Aug 31, 2017 at 02:50:58PM -0600, Jason Gunthorpe wrote:
> Now that we have ccan lists the logic here can be simplified greatly.
> Eliminate the confusing used and have_driver values, instead just
> delete items from the sysfs list as we run down the process. This
> directly guarantees discovered sysfs items are handled only once,
> and makes the lifetime of the sysfs pointers much clearer.
>
> Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> ---
>  libibverbs/init.c | 93 +++++++++++++++++++++++++++++--------------------------
>  1 file changed, 49 insertions(+), 44 deletions(-)
>
> diff --git a/libibverbs/init.c b/libibverbs/init.c
> index 91ce8b4e378458..04583bb5137c96 100644
> --- a/libibverbs/init.c
> +++ b/libibverbs/init.c
> @@ -60,9 +60,7 @@ struct ibv_sysfs_dev {
>  	char		        sysfs_path[IBV_SYSFS_PATH_MAX];
>  	char		        ibdev_path[IBV_SYSFS_PATH_MAX];
>  	int			abi_ver;
> -	int			have_driver;
>  	struct timespec		time_created;
> -	int			used;
>  };
>
>  struct ibv_driver_name {
> @@ -148,8 +146,6 @@ static int find_sysfs_devs(struct list_head *tmp_sysfs_dev_list)
>
>  		sysfs_dev->time_created = buf.st_mtim;
>
> -		sysfs_dev->have_driver = 0;
> -		sysfs_dev->used = 0;
>  		if (ibv_read_sysfs_file(sysfs_dev->sysfs_path, "abi_version",
>  					value, sizeof value) > 0)
>  			sysfs_dev->abi_ver = strtol(value, NULL, 10);
> @@ -412,7 +408,6 @@ static struct verbs_device *try_driver(struct ibv_driver *driver,
>  	strcpy(dev->name,       sysfs_dev->ibdev_name);
>  	strcpy(dev->ibdev_path, sysfs_dev->ibdev_path);
>  	vdev->sysfs = sysfs_dev;
> -	sysfs_dev->used = 1;
>
>  	return vdev;
>  }
> @@ -481,25 +476,51 @@ static int same_sysfs_dev(struct ibv_sysfs_dev *sysfs1,
>  	return 0;
>  }
>
> -int ibverbs_get_device_list(struct list_head *list)
> +/* Match every ibv_sysfs_dev in the sysfs_list to a driver and add a new entry
> + * to device_list. Once matched to a driver the entry in sysfs_list is
> + * removed.
> + */
> +static void try_all_drivers(struct list_head *sysfs_list,
> +			    struct list_head *device_list,
> +			    unsigned int *num_devices)
> +{
> +	struct ibv_sysfs_dev *sysfs_dev;
> +	struct ibv_sysfs_dev *tmp;
> +	struct verbs_device *vdev;
> +
> +	list_for_each_safe(sysfs_list, sysfs_dev, tmp, entry) {
> +		vdev = try_drivers(sysfs_dev);
> +		if (vdev) {
> +			list_del(&sysfs_dev->entry);
> +			// Ownership of sysfs_dev moves into vdev->sysfs

Please don't use "//" C++ style in C files.
Thanks

> +			list_add(device_list, &vdev->entry);
> +			(*num_devices)++;
> +		}
> +	}
> +}
> +
> +int ibverbs_get_device_list(struct list_head *device_list)
>  {
> -	LIST_HEAD(tmp_sysfs_dev_list);
> +	LIST_HEAD(sysfs_list);
>  	struct ibv_sysfs_dev *sysfs_dev, *next_dev;
>  	struct verbs_device *vdev, *tmp;
>  	static int drivers_loaded;
> -	int num_devices = 0;
> +	unsigned int num_devices = 0;
>  	int statically_linked = 0;
> -	int no_driver = 0;
>  	int ret;
>
> -	ret = find_sysfs_devs(&tmp_sysfs_dev_list);
> +	ret = find_sysfs_devs(&sysfs_list);
>  	if (ret)
>  		return -ret;
>
> -	list_for_each_safe(list, vdev, tmp, entry) {
> +	/* Remove entries from the sysfs_list that are already preset in the
> +	 * device_list, and remove entries from the device_list that are not
> +	 * present in the sysfs_list.
> +	 */
> +	list_for_each_safe(device_list, vdev, tmp, entry) {
>  		struct ibv_sysfs_dev *old_sysfs = NULL;
>
> -		list_for_each(&tmp_sysfs_dev_list, sysfs_dev, entry) {
> +		list_for_each(&sysfs_list, sysfs_dev, entry) {
>  			if (same_sysfs_dev(vdev->sysfs, sysfs_dev)) {
>  				old_sysfs = sysfs_dev;
>  				break;
> @@ -507,7 +528,8 @@ int ibverbs_get_device_list(struct list_head *list)
>  		}
>
>  		if (old_sysfs) {
> -			sysfs_dev->have_driver = 1;
> +			list_del(&old_sysfs->entry);
> +			free(old_sysfs);
>  			num_devices++;
>  		} else {
>  			list_del(&vdev->entry);
> @@ -515,19 +537,9 @@ int ibverbs_get_device_list(struct list_head *list)
>  		}
>  	}
>
> -	list_for_each(&tmp_sysfs_dev_list, sysfs_dev, entry) {
> -		if (sysfs_dev->have_driver)
> -			continue;
> -		vdev = try_drivers(sysfs_dev);
> -		if (vdev) {
> -			sysfs_dev->have_driver = 1;
> -			list_add(list, &vdev->entry);
> -			num_devices++;
> -		} else
> -			no_driver = 1;
> -	}
> +	try_all_drivers(&sysfs_list, device_list, &num_devices);
>
> -	if (!no_driver || drivers_loaded)
> +	if (list_empty(&sysfs_list) || drivers_loaded)
>  		goto out;
>
>  	/*
> @@ -553,29 +565,22 @@ int ibverbs_get_device_list(struct list_head *list)
>  	load_drivers();
>  	drivers_loaded = 1;
>
> -	list_for_each(&tmp_sysfs_dev_list, sysfs_dev, entry) {
> -		if (sysfs_dev->have_driver)
> -			continue;
> -
> -		vdev = try_drivers(sysfs_dev);
> -		if (vdev) {
> -			sysfs_dev->have_driver = 1;
> -			list_add(list, &vdev->entry);
> -			num_devices++;
> -		}
> -	}
> +	try_all_drivers(&sysfs_list, device_list, &num_devices);
>
>  out:
> -	list_for_each_safe(&tmp_sysfs_dev_list, sysfs_dev, next_dev, entry) {
> -		if (!sysfs_dev->have_driver && getenv("IBV_SHOW_WARNINGS")) {
> -			fprintf(stderr, PFX "Warning: no userspace device-specific "
> -				"driver found for %s\n", sysfs_dev->sysfs_path);
> +	/* Anything left in sysfs_list was not assoicated with a
> +	 * driver.
> +	 */
> +	list_for_each_safe(&sysfs_list, sysfs_dev, next_dev, entry) {
> +		if (getenv("IBV_SHOW_WARNINGS")) {
> +			fprintf(stderr, PFX
> +				"Warning: no userspace device-specific driver found for %s\n",
> +				sysfs_dev->sysfs_path);
>  			if (statically_linked)
> -				fprintf(stderr, "	When linking libibverbs statically, "
> -					"driver must be statically linked too.\n");
> +				fprintf(stderr,
> +					"	When linking libibverbs statically, driver must be statically linked too.\n");
>  		}
> -		if (!sysfs_dev->used)
> -			free(sysfs_dev);
> +		free(sysfs_dev);
>  	}
>
>  	return num_devices;
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-core 4/5] verbs: Tidy up ibverbs_get_device_list

[Jason Gunthorpe]

On Sun, Sep 03, 2017 at 05:03:51PM +0300, Leon Romanovsky wrote:
> > +			list_del(&sysfs_dev->entry);
> > +			// Ownership of sysfs_dev moves into vdev->sysfs
> 
> Please don't use "//" C++ style in C files.

I thought we were following the C11 rules? We use all the other
features now and don't support pre C11 compilers. (unlike the kernel)

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core 4/5] verbs: Tidy up ibverbs_get_device_list

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 567 bytes --]

On Sun, Sep 03, 2017 at 08:48:39AM -0600, Jason Gunthorpe wrote:
> On Sun, Sep 03, 2017 at 05:03:51PM +0300, Leon Romanovsky wrote:
> > > +			list_del(&sysfs_dev->entry);
> > > +			// Ownership of sysfs_dev moves into vdev->sysfs
> >
> > Please don't use "//" C++ style in C files.
>
> I thought we were following the C11 rules?

Not in the coding style, to simplify development, it is better to stick
with one known to everyone here style - kernel coding style.

> We use all the other features now and don't support pre C11 compilers. (unlike the kernel)
>
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-core 0/5] Small fixes for verbs

[Yishai Hadas]

On 8/31/2017 11:50 PM, Jason Gunthorpe wrote:
> This is a precursor to a larger series reworking how the providers
> are loaded.
>
> The big item is switching verbs fully to use ccan lists and using the new
> ccan functions to rework how the sysfs list is merged into the device list.
>
> Jason Gunthorpe (5):
>   verbs: Remove diagnostic ignored "-Wmissing-prototypes"
>   util: Fix check_snprintf to use __rc__ for local
>   verbs: Use ccan list.h instead of open coding lists
>   verbs: Tidy up ibverbs_get_device_list
>   ocrdma: Remove ocrdma_dev_list
>
>  libibverbs/init.c              | 145 ++++++++++++++++++++---------------------
>  providers/ocrdma/ocrdma_main.c |   7 --
>  providers/ocrdma/ocrdma_main.h |   1 -
>  util/util.h                    |   4 +-
>  4 files changed, 73 insertions(+), 84 deletions(-)
>

The series in the ML is not up-to-date with the latest pull request from 
GITHUB, let's please have it in sync for next time.

Passed over the pull request, it looks fine to me.
Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html