[PATCH 0/2] remove limit on devargs parameters length

[PATCH 0/2] remove limit on devargs parameters length

[David Marchand]

Here is a little patchset that removes the limit on the devargs parameters length.
Previously, arguments specified by user would be stored in a static buffer,
while there is no particular reason why we should have such a constraint, afaik.


-- 
David Marchand

David Marchand (2):
  devargs: indent and cleanup
  devargs: remove limit on parameters length

 lib/librte_eal/common/eal_common_devargs.c  |   51 ++++++++++++++++-----------
 lib/librte_eal/common/include/rte_devargs.h |    4 +--
 2 files changed, 32 insertions(+), 23 deletions(-)

-- 
1.7.10.4

[PATCH 1/2] devargs: indent and cleanup

[David Marchand]

Prepare for next commit.
Fix some indent issues, refactor error code.

Signed-off-by: David Marchand <david.marchand-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
---
 lib/librte_eal/common/eal_common_devargs.c |   27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 4c7d11a..8c9b31a 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -48,7 +48,7 @@ struct rte_devargs_list devargs_list =
 int
 rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 {
-	struct rte_devargs *devargs;
+	struct rte_devargs *devargs = NULL;
 	char buf[RTE_DEVARGS_LEN];
 	char *sep;
 	int ret;
@@ -57,14 +57,14 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 	if (ret < 0 || ret >= (int)sizeof(buf)) {
 		RTE_LOG(ERR, EAL, "user device args too large: <%s>\n",
 			devargs_str);
-		return -1;
+		goto fail;
 	}
 
 	/* use malloc instead of rte_malloc as it's called early at init */
 	devargs = malloc(sizeof(*devargs));
 	if (devargs == NULL) {
 		RTE_LOG(ERR, EAL, "cannot allocate devargs\n");
-		return -1;
+		goto fail;
 	}
 	memset(devargs, 0, sizeof(*devargs));
 	devargs->type = devtype;
@@ -81,28 +81,29 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 	case RTE_DEVTYPE_BLACKLISTED_PCI:
 		/* try to parse pci identifier */
 		if (eal_parse_pci_BDF(buf, &devargs->pci.addr) != 0 &&
-			eal_parse_pci_DomBDF(buf, &devargs->pci.addr) != 0) {
-			RTE_LOG(ERR, EAL,
-				"invalid PCI identifier <%s>\n", buf);
-			free(devargs);
-			return -1;
+		    eal_parse_pci_DomBDF(buf, &devargs->pci.addr) != 0) {
+			RTE_LOG(ERR, EAL, "invalid PCI identifier <%s>\n", buf);
+			goto fail;
 		}
 		break;
 	case RTE_DEVTYPE_VIRTUAL:
 		/* save driver name */
 		ret = snprintf(devargs->virtual.drv_name,
-			sizeof(devargs->virtual.drv_name), "%s", buf);
+			       sizeof(devargs->virtual.drv_name), "%s", buf);
 		if (ret < 0 || ret >= (int)sizeof(devargs->virtual.drv_name)) {
-			RTE_LOG(ERR, EAL,
-				"driver name too large: <%s>\n", buf);
-			free(devargs);
-			return -1;
+			RTE_LOG(ERR, EAL, "driver name too large: <%s>\n", buf);
+			goto fail;
 		}
 		break;
 	}
 
 	TAILQ_INSERT_TAIL(&devargs_list, devargs, next);
 	return 0;
+
+fail:
+	if (devargs)
+		free(devargs);
+	return -1;
 }
 
 /* count the number of devices of a specified type */
-- 
1.7.10.4

[PATCH 2/2] devargs: remove limit on parameters length

[David Marchand]

As far as I know, there is no reason why we should have a limit on the length of
parameters that can be given for a device.
Remove this limit by using dynamic allocations.

Signed-off-by: David Marchand <david.marchand-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
---
 lib/librte_eal/common/eal_common_devargs.c  |   26 +++++++++++++++++---------
 lib/librte_eal/common/include/rte_devargs.h |    4 ++--
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 8c9b31a..3aace08 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -49,17 +49,10 @@ int
 rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 {
 	struct rte_devargs *devargs = NULL;
-	char buf[RTE_DEVARGS_LEN];
+	char *buf = NULL;
 	char *sep;
 	int ret;
 
-	ret = snprintf(buf, sizeof(buf), "%s", devargs_str);
-	if (ret < 0 || ret >= (int)sizeof(buf)) {
-		RTE_LOG(ERR, EAL, "user device args too large: <%s>\n",
-			devargs_str);
-		goto fail;
-	}
-
 	/* use malloc instead of rte_malloc as it's called early at init */
 	devargs = malloc(sizeof(*devargs));
 	if (devargs == NULL) {
@@ -69,11 +62,21 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 	memset(devargs, 0, sizeof(*devargs));
 	devargs->type = devtype;
 
+	buf = strdup(devargs_str);
+	if (buf == NULL) {
+		RTE_LOG(ERR, EAL, "cannot allocate temp memory for devargs\n");
+		goto fail;
+	}
+
 	/* set the first ',' to '\0' to split name and arguments */
 	sep = strchr(buf, ',');
 	if (sep != NULL) {
 		sep[0] = '\0';
-		snprintf(devargs->args, sizeof(devargs->args), "%s", sep + 1);
+		devargs->args = strdup(sep + 1);
+		if (devargs->args == NULL) {
+			RTE_LOG(ERR, EAL, "cannot allocate for devargs args\n");
+			goto fail;
+		}
 	}
 
 	switch (devargs->type) {
@@ -97,10 +100,15 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 		break;
 	}
 
+	free(buf);
 	TAILQ_INSERT_TAIL(&devargs_list, devargs, next);
 	return 0;
 
 fail:
+	if (devargs->args)
+		free(devargs->args);
+	if (buf)
+		free(buf);
 	if (devargs)
 		free(devargs);
 	return -1;
diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
index 9f9c98f..996e180 100644
--- a/lib/librte_eal/common/include/rte_devargs.h
+++ b/lib/librte_eal/common/include/rte_devargs.h
@@ -88,8 +88,8 @@ struct rte_devargs {
 			char drv_name[32];
 		} virtual;
 	};
-#define RTE_DEVARGS_LEN 256
-	char args[RTE_DEVARGS_LEN]; /**< Arguments string as given by user. */
+	/** Arguments string as given by user. */
+	char *args;
 };
 
 /** user device double-linked queue type definition */
-- 
1.7.10.4

Re: [PATCH 2/2] devargs: remove limit on parameters length

[Stephen Hemminger]

On Wed,  7 Jan 2015 14:03:29 +0100
David Marchand <david.marchand-pdR9zngts4EAvxtiuMwx3w@public.gmane.org> wrote:

> +	buf = strdup(devargs_str);
> +	if (buf == NULL) {
> +		RTE_LOG(ERR, EAL, "cannot allocate temp memory for devargs\n");
> +		goto fail;
> +	}
> +

If string is only used in same function you might consider using strdupa() which avoids
worrying about freeing in error paths.

Re: [PATCH 2/2] devargs: remove limit on parameters length

[David Marchand]

On Wed, Jan 7, 2015 at 11:59 PM, Stephen Hemminger <
stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org> wrote:

> On Wed,  7 Jan 2015 14:03:29 +0100
> David Marchand <david.marchand-pdR9zngts4EAvxtiuMwx3w@public.gmane.org> wrote:
>
> > +     buf = strdup(devargs_str);
> > +     if (buf == NULL) {
> > +             RTE_LOG(ERR, EAL, "cannot allocate temp memory for
> devargs\n");
> > +             goto fail;
> > +     }
> > +
>
> If string is only used in same function you might consider using strdupa()
> which avoids
> worrying about freeing in error paths.
>

Hum, why not.
My only concern is strdupa() availability on BSD.

-- 
David Marchand

Re: [PATCH 2/2] devargs: remove limit on parameters length

[Panu Matilainen]

On 01/08/2015 12:59 AM, Stephen Hemminger wrote:
> On Wed,  7 Jan 2015 14:03:29 +0100
> David Marchand <david.marchand-pdR9zngts4EAvxtiuMwx3w@public.gmane.org> wrote:
>
>> +	buf = strdup(devargs_str);
>> +	if (buf == NULL) {
>> +		RTE_LOG(ERR, EAL, "cannot allocate temp memory for devargs\n");
>> +		goto fail;
>> +	}
>> +
>
> If string is only used in same function you might consider using strdupa() which avoids
> worrying about freeing in error paths.
>

It also "frees" you from having to worry about failures in the first 
place because alloca() failures are not nice and catchable, its 
undefined behavior.

If arbitrary length parameters is the goal then using the alloca() 
family of functions is replacing a controlled failure with an 
unpredictable crash.

	- Panu -

[PATCH v2 0/3] remove limit on devargs parameters length

[David Marchand]

Here is a little patchset that removes the limit on the devargs parameters
length. Previously, arguments specified by user would be stored in a static
buffer, while there is no particular reason why we should have such a
constraint, afaik.

Changes since v1:
- fix devargs tests (problem reported by Thomas)

-- 
David Marchand

David Marchand (3):
  devargs: indent and cleanup
  devargs: remove limit on parameters length
  app/test: fix devargs tests

 app/test/test_devargs.c                     |    6 ++--
 app/test/test_pci.c                         |    2 ++
 lib/librte_eal/common/eal_common_devargs.c  |   51 ++++++++++++++++-----------
 lib/librte_eal/common/include/rte_devargs.h |    4 +--
 4 files changed, 38 insertions(+), 25 deletions(-)

-- 
1.7.10.4

[PATCH v2 1/3] devargs: indent and cleanup

[David Marchand]

Prepare for next commit.
Fix some indent issues, refactor error code.

Signed-off-by: David Marchand <david.marchand-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
---
 lib/librte_eal/common/eal_common_devargs.c |   27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 4c7d11a..8c9b31a 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -48,7 +48,7 @@ struct rte_devargs_list devargs_list =
 int
 rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 {
-	struct rte_devargs *devargs;
+	struct rte_devargs *devargs = NULL;
 	char buf[RTE_DEVARGS_LEN];
 	char *sep;
 	int ret;
@@ -57,14 +57,14 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 	if (ret < 0 || ret >= (int)sizeof(buf)) {
 		RTE_LOG(ERR, EAL, "user device args too large: <%s>\n",
 			devargs_str);
-		return -1;
+		goto fail;
 	}
 
 	/* use malloc instead of rte_malloc as it's called early at init */
 	devargs = malloc(sizeof(*devargs));
 	if (devargs == NULL) {
 		RTE_LOG(ERR, EAL, "cannot allocate devargs\n");
-		return -1;
+		goto fail;
 	}
 	memset(devargs, 0, sizeof(*devargs));
 	devargs->type = devtype;
@@ -81,28 +81,29 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 	case RTE_DEVTYPE_BLACKLISTED_PCI:
 		/* try to parse pci identifier */
 		if (eal_parse_pci_BDF(buf, &devargs->pci.addr) != 0 &&
-			eal_parse_pci_DomBDF(buf, &devargs->pci.addr) != 0) {
-			RTE_LOG(ERR, EAL,
-				"invalid PCI identifier <%s>\n", buf);
-			free(devargs);
-			return -1;
+		    eal_parse_pci_DomBDF(buf, &devargs->pci.addr) != 0) {
+			RTE_LOG(ERR, EAL, "invalid PCI identifier <%s>\n", buf);
+			goto fail;
 		}
 		break;
 	case RTE_DEVTYPE_VIRTUAL:
 		/* save driver name */
 		ret = snprintf(devargs->virtual.drv_name,
-			sizeof(devargs->virtual.drv_name), "%s", buf);
+			       sizeof(devargs->virtual.drv_name), "%s", buf);
 		if (ret < 0 || ret >= (int)sizeof(devargs->virtual.drv_name)) {
-			RTE_LOG(ERR, EAL,
-				"driver name too large: <%s>\n", buf);
-			free(devargs);
-			return -1;
+			RTE_LOG(ERR, EAL, "driver name too large: <%s>\n", buf);
+			goto fail;
 		}
 		break;
 	}
 
 	TAILQ_INSERT_TAIL(&devargs_list, devargs, next);
 	return 0;
+
+fail:
+	if (devargs)
+		free(devargs);
+	return -1;
 }
 
 /* count the number of devices of a specified type */
-- 
1.7.10.4

[PATCH v2 2/3] devargs: remove limit on parameters length

[David Marchand]

As far as I know, there is no reason why we should have a limit on the length of
parameters that can be given for a device.
Remove this limit by using dynamic allocations.

Signed-off-by: David Marchand <david.marchand-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
---
 lib/librte_eal/common/eal_common_devargs.c  |   26 +++++++++++++++++---------
 lib/librte_eal/common/include/rte_devargs.h |    4 ++--
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index 8c9b31a..3aace08 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -49,17 +49,10 @@ int
 rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 {
 	struct rte_devargs *devargs = NULL;
-	char buf[RTE_DEVARGS_LEN];
+	char *buf = NULL;
 	char *sep;
 	int ret;
 
-	ret = snprintf(buf, sizeof(buf), "%s", devargs_str);
-	if (ret < 0 || ret >= (int)sizeof(buf)) {
-		RTE_LOG(ERR, EAL, "user device args too large: <%s>\n",
-			devargs_str);
-		goto fail;
-	}
-
 	/* use malloc instead of rte_malloc as it's called early at init */
 	devargs = malloc(sizeof(*devargs));
 	if (devargs == NULL) {
@@ -69,11 +62,21 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 	memset(devargs, 0, sizeof(*devargs));
 	devargs->type = devtype;
 
+	buf = strdup(devargs_str);
+	if (buf == NULL) {
+		RTE_LOG(ERR, EAL, "cannot allocate temp memory for devargs\n");
+		goto fail;
+	}
+
 	/* set the first ',' to '\0' to split name and arguments */
 	sep = strchr(buf, ',');
 	if (sep != NULL) {
 		sep[0] = '\0';
-		snprintf(devargs->args, sizeof(devargs->args), "%s", sep + 1);
+		devargs->args = strdup(sep + 1);
+		if (devargs->args == NULL) {
+			RTE_LOG(ERR, EAL, "cannot allocate for devargs args\n");
+			goto fail;
+		}
 	}
 
 	switch (devargs->type) {
@@ -97,10 +100,15 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
 		break;
 	}
 
+	free(buf);
 	TAILQ_INSERT_TAIL(&devargs_list, devargs, next);
 	return 0;
 
 fail:
+	if (devargs->args)
+		free(devargs->args);
+	if (buf)
+		free(buf);
 	if (devargs)
 		free(devargs);
 	return -1;
diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
index 9f9c98f..996e180 100644
--- a/lib/librte_eal/common/include/rte_devargs.h
+++ b/lib/librte_eal/common/include/rte_devargs.h
@@ -88,8 +88,8 @@ struct rte_devargs {
 			char drv_name[32];
 		} virtual;
 	};
-#define RTE_DEVARGS_LEN 256
-	char args[RTE_DEVARGS_LEN]; /**< Arguments string as given by user. */
+	/** Arguments string as given by user. */
+	char *args;
 };
 
 /** user device double-linked queue type definition */
-- 
1.7.10.4

[PATCH v2 3/3] app/test: fix devargs tests

[David Marchand]

Add missing free for devargs->args and fix tests.

Signed-off-by: David Marchand <david.marchand-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
---
 app/test/test_devargs.c |    6 ++++--
 app/test/test_pci.c     |    2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/app/test/test_devargs.c b/app/test/test_devargs.c
index 3d9f7bc..08fb781 100644
--- a/app/test/test_devargs.c
+++ b/app/test/test_devargs.c
@@ -48,6 +48,8 @@ static void free_devargs_list(void)
 	while (!TAILQ_EMPTY(&devargs_list)) {
 		devargs = TAILQ_FIRST(&devargs_list);
 		TAILQ_REMOVE(&devargs_list, devargs, next);
+		if (devargs->args)
+			free(devargs->args);
 		free(devargs);
 	}
 }
@@ -92,7 +94,7 @@ test_devargs(void)
 	if (strncmp(devargs->virtual.drv_name, "eth_ring1",
 			sizeof(devargs->virtual.drv_name)) != 0)
 		goto fail;
-	if (strncmp(devargs->args, "k1=val,k2=val2", sizeof(devargs->args)) != 0)
+	if (!devargs->args || strcmp(devargs->args, "k1=val,k2=val2") != 0)
 		goto fail;
 	free_devargs_list();
 
@@ -105,7 +107,7 @@ test_devargs(void)
 		devargs->pci.addr.devid != 0 ||
 		devargs->pci.addr.function != 1)
 		goto fail;
-	if (strncmp(devargs->args, "", sizeof(devargs->args)) != 0)
+	if (devargs->args)
 		goto fail;
 	free_devargs_list();
 
diff --git a/app/test/test_pci.c b/app/test/test_pci.c
index 747d8b7..5530d99 100644
--- a/app/test/test_pci.c
+++ b/app/test/test_pci.c
@@ -139,6 +139,8 @@ static void free_devargs_list(void)
 	while (!TAILQ_EMPTY(&devargs_list)) {
 		devargs = TAILQ_FIRST(&devargs_list);
 		TAILQ_REMOVE(&devargs_list, devargs, next);
+		if (devargs->args)
+			free(devargs->args);
 		free(devargs);
 	}
 }
-- 
1.7.10.4

Re: [PATCH v2 0/3] remove limit on devargs parameters length

[Thomas Monjalon]

2015-02-13 16:03, David Marchand:
> Here is a little patchset that removes the limit on the devargs parameters
> length. Previously, arguments specified by user would be stored in a static
> buffer, while there is no particular reason why we should have such a
> constraint, afaik.
> 
> Changes since v1:
> - fix devargs tests (problem reported by Thomas)
> 
> David Marchand (3):
>   devargs: indent and cleanup
>   devargs: remove limit on parameters length
>   app/test: fix devargs tests

Acked-by: Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>

Applied, thanks