diff mbox series

[v3,1/7] eal: add wrappers for POSIX string functions

Message ID 20210221142819.6769-2-dmitry.kozliuk@gmail.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series eal/windows: do not expose POSIX symbols | expand

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Dmitry Kozlyuk Feb. 21, 2021, 2:28 p.m. UTC
POSIX strncasecmp(), strdup(), and strtok_r() have different names
on Windows, respectively, strnicmp(), _strdup(), and strtok_s().

Add wrappers as inline functions, because they're used from librte_kvargs,
and thus cannot be in librte_eal; besides, implementation is trivial.

Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
 lib/librte_eal/common/eal_common_dev.c        |  6 +--
 lib/librte_eal/common/eal_common_devargs.c    |  7 ++--
 lib/librte_eal/common/eal_common_log.c        |  5 ++-
 lib/librte_eal/common/eal_common_options.c    | 12 +++---
 lib/librte_eal/common/eal_common_trace_ctf.c  |  2 +-
 .../common/eal_common_trace_utils.c           |  2 +-
 lib/librte_eal/include/rte_string_fns.h       | 42 +++++++++++++++++++
 7 files changed, 60 insertions(+), 16 deletions(-)

Comments

Andrew Rybchenko Feb. 23, 2021, 7:11 a.m. UTC | #1
On 2/21/21 5:28 PM, Dmitry Kozlyuk wrote:
> POSIX strncasecmp(), strdup(), and strtok_r() have different names
> on Windows, respectively, strnicmp(), _strdup(), and strtok_s().
> 
> Add wrappers as inline functions, because they're used from librte_kvargs,
> and thus cannot be in librte_eal; besides, implementation is trivial.
> 
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>

[snip]

> +/**
> + * @internal
> + * strdup(3) replacement for systems that don't have it.
> + */
> +static inline char *
> +rte_strdup(const char *str)
> +{
> +#ifdef RTE_EXEC_ENV_WINDOWS
> +	return _strdup(str);
> +#else
> +	return strdup(str);
> +#endif
> +}

Allocating memory using rte_strdup() I'd use rte_free()
to release it. I guess it will fail badly.
So, I think that a different, more specific prefix is
required for POSIX wrappers.
Nick Connolly Feb. 23, 2021, 9:53 p.m. UTC | #2
> Allocating memory using rte_strdup() I'd use rte_free()
> to release it. I guess it will fail badly.
> So, I think that a different, more specific prefix is
> required for POSIX wrappers.

Andrew: my understanding of Bruce's proposal is that the strdup() name 
will now be kept (in this case through an inline definition), so I think 
this will be ok.  However, your comment reminded me of something else 
that it's probably worth mentioning as an aside:

As a general guideline on Windows, memory allocated within a shared 
library is best freed within the same DLL to ensure it goes back to the 
correct heap.  So we'd want to avoid calling strdup and then returning 
the value to the application for it to free (hopefully this doesn't 
happen). With an inline definition there's no change in behaviour, but 
adding rte_strdup (or anything else that calls malloc) into librte_eal 
might be an issue.

Regards,
Nick
Andrew Rybchenko Feb. 24, 2021, 7:21 a.m. UTC | #3
On 2/24/21 12:53 AM, Nick Connolly wrote:
> 
>> Allocating memory using rte_strdup() I'd use rte_free()
>> to release it. I guess it will fail badly.
>> So, I think that a different, more specific prefix is
>> required for POSIX wrappers.
> 
> Andrew: my understanding of Bruce's proposal is that the strdup() name
> will now be kept (in this case through an inline definition), so I think
> this will be ok. 

Very good, glad to hear it. Thanks.

> However, your comment reminded me of something else
> that it's probably worth mentioning as an aside:
> 
> As a general guideline on Windows, memory allocated within a shared
> library is best freed within the same DLL to ensure it goes back to the
> correct heap.  So we'd want to avoid calling strdup and then returning
> the value to the application for it to free (hopefully this doesn't
> happen). With an inline definition there's no change in behaviour, but
> adding rte_strdup (or anything else that calls malloc) into librte_eal
> might be an issue.
> 
> Regards,
> Nick
Khoa To March 4, 2021, 6:47 a.m. UTC | #4
> POSIX strncasecmp(), strdup(), and strtok_r() have different names
> on Windows, respectively, strnicmp(), _strdup(), and strtok_s().
> 
> Add wrappers as inline functions, because they're used from librte_kvargs,
> and thus cannot be in librte_eal; besides, implementation is trivial.
> 
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---

Acked-by: Khoa To <khot@microsoft.com>
diff mbox series

Patch

diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
index 8a3bd3100..0a15fdcf7 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -462,7 +462,7 @@  rte_dev_event_callback_register(const char *device_name,
 			if (!device_name) {
 				event_cb->dev_name = NULL;
 			} else {
-				event_cb->dev_name = strdup(device_name);
+				event_cb->dev_name = rte_strdup(device_name);
 				if (event_cb->dev_name == NULL) {
 					ret = -ENOMEM;
 					goto error;
@@ -630,10 +630,10 @@  dev_str_sane_copy(const char *str)
 
 	end = strcspn(str, ",/");
 	if (str[end] == ',') {
-		copy = strdup(&str[end + 1]);
+		copy = rte_strdup(&str[end + 1]);
 	} else {
 		/* '/' or '\0' */
-		copy = strdup("");
+		copy = rte_strdup("");
 	}
 	if (copy == NULL) {
 		rte_errno = ENOMEM;
diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
index fcf3d9a3c..14e082a27 100644
--- a/lib/librte_eal/common/eal_common_devargs.c
+++ b/lib/librte_eal/common/eal_common_devargs.c
@@ -18,6 +18,7 @@ 
 #include <rte_errno.h>
 #include <rte_kvargs.h>
 #include <rte_log.h>
+#include <rte_string_fns.h>
 #include <rte_tailq.h>
 #include "eal_private.h"
 
@@ -75,7 +76,7 @@  rte_devargs_layers_parse(struct rte_devargs *devargs,
 	 * anything and keep referring only to it.
 	 */
 	if (devargs->data != devstr) {
-		devargs->data = strdup(devstr);
+		devargs->data = rte_strdup(devstr);
 		if (devargs->data == NULL) {
 			RTE_LOG(ERR, EAL, "OOM\n");
 			ret = -ENOMEM;
@@ -219,9 +220,9 @@  rte_devargs_parse(struct rte_devargs *da, const char *dev)
 	da->bus = bus;
 	/* Parse eventual device arguments */
 	if (devname[i] == ',')
-		da->args = strdup(&devname[i + 1]);
+		da->args = rte_strdup(&devname[i + 1]);
 	else
-		da->args = strdup("");
+		da->args = rte_strdup("");
 	if (da->args == NULL) {
 		RTE_LOG(ERR, EAL, "not enough memory to parse arguments\n");
 		return -ENOMEM;
diff --git a/lib/librte_eal/common/eal_common_log.c b/lib/librte_eal/common/eal_common_log.c
index c5554badb..557a8243f 100644
--- a/lib/librte_eal/common/eal_common_log.c
+++ b/lib/librte_eal/common/eal_common_log.c
@@ -14,6 +14,7 @@ 
 #include <rte_eal.h>
 #include <rte_log.h>
 #include <rte_per_lcore.h>
+#include <rte_string_fns.h>
 
 #include "eal_private.h"
 
@@ -194,7 +195,7 @@  static int rte_log_save_level(int priority,
 		if (regcomp(&opt_ll->re_match, regex, 0) != 0)
 			goto fail;
 	} else if (pattern) {
-		opt_ll->pattern = strdup(pattern);
+		opt_ll->pattern = rte_strdup(pattern);
 		if (opt_ll->pattern == NULL)
 			goto fail;
 	} else
@@ -270,7 +271,7 @@  rte_log_lookup(const char *name)
 static int
 __rte_log_register(const char *name, int id)
 {
-	char *dup_name = strdup(name);
+	char *dup_name = rte_strdup(name);
 
 	if (dup_name == NULL)
 		return -ENOMEM;
diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 622c7bc42..3612ad441 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -228,7 +228,7 @@  eal_save_args(int argc, char **argv)
 		return -1;
 
 	for (i = 0; i < argc; i++) {
-		eal_args[i] = strdup(argv[i]);
+		eal_args[i] = rte_strdup(argv[i]);
 		if (strcmp(argv[i], "--") == 0)
 			break;
 	}
@@ -243,7 +243,7 @@  eal_save_args(int argc, char **argv)
 		return -1;
 
 	for (j = 0; i < argc; j++, i++)
-		eal_app_args[j] = strdup(argv[i]);
+		eal_app_args[j] = rte_strdup(argv[i]);
 	eal_app_args[j] = NULL;
 
 	return 0;
@@ -1273,7 +1273,7 @@  eal_parse_log_level(const char *arg)
 	char *str, *level;
 	int priority;
 
-	str = strdup(arg);
+	str = rte_strdup(arg);
 	if (str == NULL)
 		return -1;
 
@@ -1324,11 +1324,11 @@  eal_parse_log_level(const char *arg)
 static enum rte_proc_type_t
 eal_parse_proc_type(const char *arg)
 {
-	if (strncasecmp(arg, "primary", sizeof("primary")) == 0)
+	if (rte_strncasecmp(arg, "primary", sizeof("primary")) == 0)
 		return RTE_PROC_PRIMARY;
-	if (strncasecmp(arg, "secondary", sizeof("secondary")) == 0)
+	if (rte_strncasecmp(arg, "secondary", sizeof("secondary")) == 0)
 		return RTE_PROC_SECONDARY;
-	if (strncasecmp(arg, "auto", sizeof("auto")) == 0)
+	if (rte_strncasecmp(arg, "auto", sizeof("auto")) == 0)
 		return RTE_PROC_AUTO;
 
 	return RTE_PROC_INVALID;
diff --git a/lib/librte_eal/common/eal_common_trace_ctf.c b/lib/librte_eal/common/eal_common_trace_ctf.c
index 33e419aac..4041d9af6 100644
--- a/lib/librte_eal/common/eal_common_trace_ctf.c
+++ b/lib/librte_eal/common/eal_common_trace_ctf.c
@@ -398,7 +398,7 @@  char *trace_metadata_fixup_field(const char *field)
 	if (strstr(field, ".") == NULL && strstr(field, "->") == NULL)
 		return NULL;
 
-	out = strdup(field);
+	out = rte_strdup(field);
 	if (out == NULL)
 		return NULL;
 	p = out;
diff --git a/lib/librte_eal/common/eal_common_trace_utils.c b/lib/librte_eal/common/eal_common_trace_utils.c
index 64f58fb66..d541a5ea9 100644
--- a/lib/librte_eal/common/eal_common_trace_utils.c
+++ b/lib/librte_eal/common/eal_common_trace_utils.c
@@ -145,7 +145,7 @@  eal_trace_args_save(const char *val)
 		return -ENOMEM;
 	}
 
-	arg->val = strdup(val);
+	arg->val = rte_strdup(val);
 	if (arg->val == NULL) {
 		trace_err("failed to allocate memory for %s", val);
 		free(arg);
diff --git a/lib/librte_eal/include/rte_string_fns.h b/lib/librte_eal/include/rte_string_fns.h
index 8bac8243c..2d9d5afc8 100644
--- a/lib/librte_eal/include/rte_string_fns.h
+++ b/lib/librte_eal/include/rte_string_fns.h
@@ -116,6 +116,48 @@  rte_strlcat(char *dst, const char *src, size_t size)
 ssize_t
 rte_strscpy(char *dst, const char *src, size_t dsize);
 
+/**
+ * @internal
+ * strncasecmp(3) replacement for systems that don't have it.
+ */
+static inline int
+rte_strncasecmp(const char *s1, const char *s2, size_t size)
+{
+#ifdef RTE_EXEC_ENV_WINDOWS
+	return _strnicmp(s1, s2, size);
+#else
+	return strncasecmp(s1, s2, size);
+#endif
+}
+
+/**
+ * @internal
+ * strtor_r(3) replacement for systems that don't have it.
+ */
+static inline char *
+rte_strtok(char *str, const char *delim, char **saveptr)
+{
+#ifdef RTE_EXEC_ENV_WINDOWS
+	return strtok_s(str, delim, saveptr);
+#else
+	return strtok_r(str, delim, saveptr);
+#endif
+}
+
+/**
+ * @internal
+ * strdup(3) replacement for systems that don't have it.
+ */
+static inline char *
+rte_strdup(const char *str)
+{
+#ifdef RTE_EXEC_ENV_WINDOWS
+	return _strdup(str);
+#else
+	return strdup(str);
+#endif
+}
+
 #ifdef __cplusplus
 }
 #endif