[dpdk-dev] [PATCH] eal: don't reset getopt lib

[dpdk-dev] [PATCH] eal: don't reset getopt lib

[Tiwei Bie]

Someone may need to call rte_eal_init() with a fake argc/argv array
in the middle of using getopt() to parse its own unrelated argc/argv
parameters. So getopt lib shouldn't be reset by rte_eal_init().

Now eal will always save optind, optarg and optopt (and optreset on
FreeBSD) at the beginning, initialize optind (and optreset on FreeBSD)
to 1 before calling getopt_long(), then restore all values after.

Suggested-by: Don Provan <dprovan@bivio.net>
Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
Signed-off-by: Tiwei Bie <btw@mail.ustc.edu.cn>
---
 lib/librte_eal/bsdapp/eal/eal.c   | 59 +++++++++++++++++++++++++++------
 lib/librte_eal/linuxapp/eal/eal.c | 69 ++++++++++++++++++++++++++++++---------
 2 files changed, 102 insertions(+), 26 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index 1b6f705..bd09377 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -312,8 +312,20 @@ eal_log_level_parse(int argc, char **argv)
 	int opt;
 	char **argvopt;
 	int option_index;
+	int old_optind;
+	int old_optopt;
+	int old_optreset;
+	char *old_optarg;
+
+	/* save getopt lib */
+	old_optind = optind;
+	old_optopt = optopt;
+	old_optreset = optreset;
+	old_optarg = optarg;
 
 	argvopt = argv;
+	optind = 1;
+	optreset = 1;
 
 	eal_reset_internal_config(&internal_config);
 
@@ -334,7 +346,11 @@ eal_log_level_parse(int argc, char **argv)
 			break;
 	}
 
-	optind = 0; /* reset getopt lib */
+	/* restore getopt lib */
+	optind = old_optind;
+	optopt = old_optopt;
+	optreset = old_optreset;
+	optarg = old_optarg;
 }
 
 /* Parse the argument given in the command line of the application */
@@ -345,25 +361,37 @@ eal_parse_args(int argc, char **argv)
 	char **argvopt;
 	int option_index;
 	char *prgname = argv[0];
+	int old_optind;
+	int old_optopt;
+	int old_optreset;
+	char *old_optarg;
+
+	/* save getopt lib */
+	old_optind = optind;
+	old_optopt = optopt;
+	old_optreset = optreset;
+	old_optarg = optarg;
 
 	argvopt = argv;
+	optind = 1;
+	optreset = 1;
 
 	while ((opt = getopt_long(argc, argvopt, eal_short_options,
 				  eal_long_options, &option_index)) != EOF) {
 
-		int ret;
-
 		/* getopt is not happy, stop right now */
 		if (opt == '?') {
 			eal_usage(prgname);
-			return -1;
+			ret = -1;
+			goto out;
 		}
 
 		ret = eal_parse_common_option(opt, optarg, &internal_config);
 		/* common parser is not happy */
 		if (ret < 0) {
 			eal_usage(prgname);
-			return -1;
+			ret = -1;
+			goto out;
 		}
 		/* common parser handled this option */
 		if (ret == 0)
@@ -387,23 +415,34 @@ eal_parse_args(int argc, char **argv)
 					"on FreeBSD\n", opt);
 			}
 			eal_usage(prgname);
-			return -1;
+			ret = -1;
+			goto out;
 		}
 	}
 
-	if (eal_adjust_config(&internal_config) != 0)
-		return -1;
+	if (eal_adjust_config(&internal_config) != 0) {
+		ret = -1;
+		goto out;
+	}
 
 	/* sanity checks */
 	if (eal_check_common_options(&internal_config) != 0) {
 		eal_usage(prgname);
-		return -1;
+		ret = -1;
+		goto out;
 	}
 
 	if (optind >= 0)
 		argv[optind-1] = prgname;
 	ret = optind-1;
-	optind = 0; /* reset getopt lib */
+
+out:
+	/* restore getopt lib */
+	optind = old_optind;
+	optopt = old_optopt;
+	optreset = old_optreset;
+	optarg = old_optarg;
+
 	return ret;
 }
 
diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index 33e1067..4796030 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -505,8 +505,17 @@ eal_log_level_parse(int argc, char **argv)
 	int opt;
 	char **argvopt;
 	int option_index;
+	int old_optind;
+	int old_optopt;
+	char *old_optarg;
+
+	/* save getopt lib */
+	old_optind = optind;
+	old_optopt = optopt;
+	old_optarg = optarg;
 
 	argvopt = argv;
+	optind = 1;
 
 	eal_reset_internal_config(&internal_config);
 
@@ -527,7 +536,10 @@ eal_log_level_parse(int argc, char **argv)
 			break;
 	}
 
-	optind = 0; /* reset getopt lib */
+	/* restore getopt lib */
+	optind = old_optind;
+	optopt = old_optopt;
+	optarg = old_optarg;
 }
 
 /* Parse the argument given in the command line of the application */
@@ -539,25 +551,34 @@ eal_parse_args(int argc, char **argv)
 	int option_index;
 	char *prgname = argv[0];
 	struct shared_driver *solib;
+	int old_optind;
+	int old_optopt;
+	char *old_optarg;
+
+	/* save getopt lib */
+	old_optind = optind;
+	old_optopt = optopt;
+	old_optarg = optarg;
 
 	argvopt = argv;
+	optind = 1;
 
 	while ((opt = getopt_long(argc, argvopt, eal_short_options,
 				  eal_long_options, &option_index)) != EOF) {
 
-		int ret;
-
 		/* getopt is not happy, stop right now */
 		if (opt == '?') {
 			eal_usage(prgname);
-			return -1;
+			ret = -1;
+			goto out;
 		}
 
 		ret = eal_parse_common_option(opt, optarg, &internal_config);
 		/* common parser is not happy */
 		if (ret < 0) {
 			eal_usage(prgname);
-			return -1;
+			ret = -1;
+			goto out;
 		}
 		/* common parser handled this option */
 		if (ret == 0)
@@ -573,7 +594,8 @@ eal_parse_args(int argc, char **argv)
 			solib = malloc(sizeof(*solib));
 			if (solib == NULL) {
 				RTE_LOG(ERR, EAL, "malloc(solib) failed\n");
-				return -1;
+				ret = -1;
+				goto out;
 			}
 			memset(solib, 0, sizeof(*solib));
 			strncpy(solib->name, optarg, PATH_MAX-1);
@@ -589,7 +611,8 @@ eal_parse_args(int argc, char **argv)
 			RTE_LOG(ERR, EAL, "Can't support DPDK app "
 				"running on Dom0, please configure"
 				" RTE_LIBRTE_XEN_DOM0=y\n");
-			return -1;
+			ret = -1;
+			goto out;
 #endif
 			break;
 
@@ -606,7 +629,8 @@ eal_parse_args(int argc, char **argv)
 				RTE_LOG(ERR, EAL, "invalid parameters for --"
 						OPT_SOCKET_MEM "\n");
 				eal_usage(prgname);
-				return -1;
+				ret = -1;
+				goto out;
 			}
 			break;
 
@@ -615,7 +639,8 @@ eal_parse_args(int argc, char **argv)
 				RTE_LOG(ERR, EAL, "invalid parameter for --"
 						OPT_BASE_VIRTADDR "\n");
 				eal_usage(prgname);
-				return -1;
+				ret = -1;
+				goto out;
 			}
 			break;
 
@@ -624,7 +649,8 @@ eal_parse_args(int argc, char **argv)
 				RTE_LOG(ERR, EAL, "invalid parameters for --"
 						OPT_VFIO_INTR "\n");
 				eal_usage(prgname);
-				return -1;
+				ret = -1;
+				goto out;
 			}
 			break;
 
@@ -646,17 +672,21 @@ eal_parse_args(int argc, char **argv)
 					"on Linux\n", opt);
 			}
 			eal_usage(prgname);
-			return -1;
+			ret = -1;
+			goto out;
 		}
 	}
 
-	if (eal_adjust_config(&internal_config) != 0)
-		return -1;
+	if (eal_adjust_config(&internal_config) != 0) {
+		ret = -1;
+		goto out;
+	}
 
 	/* sanity checks */
 	if (eal_check_common_options(&internal_config) != 0) {
 		eal_usage(prgname);
-		return -1;
+		ret = -1;
+		goto out;
 	}
 
 	/* --xen-dom0 doesn't make sense with --socket-mem */
@@ -664,13 +694,20 @@ eal_parse_args(int argc, char **argv)
 		RTE_LOG(ERR, EAL, "Options --"OPT_SOCKET_MEM" cannot be specified "
 			"together with --"OPT_XEN_DOM0"\n");
 		eal_usage(prgname);
-		return -1;
+		ret = -1;
+		goto out;
 	}
 
 	if (optind >= 0)
 		argv[optind-1] = prgname;
 	ret = optind-1;
-	optind = 0; /* reset getopt lib */
+
+out:
+	/* restore getopt lib */
+	optind = old_optind;
+	optopt = old_optopt;
+	optarg = old_optarg;
+
 	return ret;
 }
 
-- 
2.1.2

Re: [dpdk-dev] [PATCH] eal: don't reset getopt lib

[Don Provan]

Looks perfect. Thanks!
-don

-----Original Message-----
From: Tiwei Bie [mailto:btw@mail.ustc.edu.cn] 
Sent: Thursday, October 15, 2015 4:46 AM
To: Don Provan <dprovan@bivio.net>; bruce.richardson@intel.com; dev@dpdk.org
Subject: [PATCH] eal: don't reset getopt lib

Someone may need to call rte_eal_init() with a fake argc/argv array in the middle of using getopt() to parse its own unrelated argc/argv parameters. So getopt lib shouldn't be reset by rte_eal_init().

Now eal will always save optind, optarg and optopt (and optreset on
FreeBSD) at the beginning, initialize optind (and optreset on FreeBSD) to 1 before calling getopt_long(), then restore all values after.

Suggested-by: Don Provan <dprovan@bivio.net>
Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
Signed-off-by: Tiwei Bie <btw@mail.ustc.edu.cn>
---
 lib/librte_eal/bsdapp/eal/eal.c   | 59 +++++++++++++++++++++++++++------
 lib/librte_eal/linuxapp/eal/eal.c | 69 ++++++++++++++++++++++++++++++---------
 2 files changed, 102 insertions(+), 26 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c index 1b6f705..bd09377 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -312,8 +312,20 @@ eal_log_level_parse(int argc, char **argv)
 	int opt;
 	char **argvopt;
 	int option_index;
+	int old_optind;
+	int old_optopt;
+	int old_optreset;
+	char *old_optarg;
+
+	/* save getopt lib */
+	old_optind = optind;
+	old_optopt = optopt;
+	old_optreset = optreset;
+	old_optarg = optarg;
 
 	argvopt = argv;
+	optind = 1;
+	optreset = 1;
 
 	eal_reset_internal_config(&internal_config);
 
@@ -334,7 +346,11 @@ eal_log_level_parse(int argc, char **argv)
 			break;
 	}
 
-	optind = 0; /* reset getopt lib */
+	/* restore getopt lib */
+	optind = old_optind;
+	optopt = old_optopt;
+	optreset = old_optreset;
+	optarg = old_optarg;
 }
 
 /* Parse the argument given in the command line of the application */ @@ -345,25 +361,37 @@ eal_parse_args(int argc, char **argv)
 	char **argvopt;
 	int option_index;
 	char *prgname = argv[0];
+	int old_optind;
+	int old_optopt;
+	int old_optreset;
+	char *old_optarg;
+
+	/* save getopt lib */
+	old_optind = optind;
+	old_optopt = optopt;
+	old_optreset = optreset;
+	old_optarg = optarg;
 
 	argvopt = argv;
+	optind = 1;
+	optreset = 1;
 
 	while ((opt = getopt_long(argc, argvopt, eal_short_options,
 				  eal_long_options, &option_index)) != EOF) {
 
-		int ret;
-
 		/* getopt is not happy, stop right now */
 		if (opt == '?') {
 			eal_usage(prgname);
-			return -1;
+			ret = -1;
+			goto out;
 		}
 
 		ret = eal_parse_common_option(opt, optarg, &internal_config);
 		/* common parser is not happy */
 		if (ret < 0) {
 			eal_usage(prgname);
-			return -1;
+			ret = -1;
+			goto out;
 		}
 		/* common parser handled this option */
 		if (ret == 0)
@@ -387,23 +415,34 @@ eal_parse_args(int argc, char **argv)
 					"on FreeBSD\n", opt);
 			}
 			eal_usage(prgname);
-			return -1;
+			ret = -1;
+			goto out;
 		}
 	}
 
-	if (eal_adjust_config(&internal_config) != 0)
-		return -1;
+	if (eal_adjust_config(&internal_config) != 0) {
+		ret = -1;
+		goto out;
+	}
 
 	/* sanity checks */
 	if (eal_check_common_options(&internal_config) != 0) {
 		eal_usage(prgname);
-		return -1;
+		ret = -1;
+		goto out;
 	}
 
 	if (optind >= 0)
 		argv[optind-1] = prgname;
 	ret = optind-1;
-	optind = 0; /* reset getopt lib */
+
+out:
+	/* restore getopt lib */
+	optind = old_optind;
+	optopt = old_optopt;
+	optreset = old_optreset;
+	optarg = old_optarg;
+
 	return ret;
 }
 
diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index 33e1067..4796030 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -505,8 +505,17 @@ eal_log_level_parse(int argc, char **argv)
 	int opt;
 	char **argvopt;
 	int option_index;
+	int old_optind;
+	int old_optopt;
+	char *old_optarg;
+
+	/* save getopt lib */
+	old_optind = optind;
+	old_optopt = optopt;
+	old_optarg = optarg;
 
 	argvopt = argv;
+	optind = 1;
 
 	eal_reset_internal_config(&internal_config);
 
@@ -527,7 +536,10 @@ eal_log_level_parse(int argc, char **argv)
 			break;
 	}
 
-	optind = 0; /* reset getopt lib */
+	/* restore getopt lib */
+	optind = old_optind;
+	optopt = old_optopt;
+	optarg = old_optarg;
 }
 
 /* Parse the argument given in the command line of the application */ @@ -539,25 +551,34 @@ eal_parse_args(int argc, char **argv)
 	int option_index;
 	char *prgname = argv[0];
 	struct shared_driver *solib;
+	int old_optind;
+	int old_optopt;
+	char *old_optarg;
+
+	/* save getopt lib */
+	old_optind = optind;
+	old_optopt = optopt;
+	old_optarg = optarg;
 
 	argvopt = argv;
+	optind = 1;
 
 	while ((opt = getopt_long(argc, argvopt, eal_short_options,
 				  eal_long_options, &option_index)) != EOF) {
 
-		int ret;
-
 		/* getopt is not happy, stop right now */
 		if (opt == '?') {
 			eal_usage(prgname);
-			return -1;
+			ret = -1;
+			goto out;
 		}
 
 		ret = eal_parse_common_option(opt, optarg, &internal_config);
 		/* common parser is not happy */
 		if (ret < 0) {
 			eal_usage(prgname);
-			return -1;
+			ret = -1;
+			goto out;
 		}
 		/* common parser handled this option */
 		if (ret == 0)
@@ -573,7 +594,8 @@ eal_parse_args(int argc, char **argv)
 			solib = malloc(sizeof(*solib));
 			if (solib == NULL) {
 				RTE_LOG(ERR, EAL, "malloc(solib) failed\n");
-				return -1;
+				ret = -1;
+				goto out;
 			}
 			memset(solib, 0, sizeof(*solib));
 			strncpy(solib->name, optarg, PATH_MAX-1); @@ -589,7 +611,8 @@ eal_parse_args(int argc, char **argv)
 			RTE_LOG(ERR, EAL, "Can't support DPDK app "
 				"running on Dom0, please configure"
 				" RTE_LIBRTE_XEN_DOM0=y\n");
-			return -1;
+			ret = -1;
+			goto out;
 #endif
 			break;
 
@@ -606,7 +629,8 @@ eal_parse_args(int argc, char **argv)
 				RTE_LOG(ERR, EAL, "invalid parameters for --"
 						OPT_SOCKET_MEM "\n");
 				eal_usage(prgname);
-				return -1;
+				ret = -1;
+				goto out;
 			}
 			break;
 
@@ -615,7 +639,8 @@ eal_parse_args(int argc, char **argv)
 				RTE_LOG(ERR, EAL, "invalid parameter for --"
 						OPT_BASE_VIRTADDR "\n");
 				eal_usage(prgname);
-				return -1;
+				ret = -1;
+				goto out;
 			}
 			break;
 
@@ -624,7 +649,8 @@ eal_parse_args(int argc, char **argv)
 				RTE_LOG(ERR, EAL, "invalid parameters for --"
 						OPT_VFIO_INTR "\n");
 				eal_usage(prgname);
-				return -1;
+				ret = -1;
+				goto out;
 			}
 			break;
 
@@ -646,17 +672,21 @@ eal_parse_args(int argc, char **argv)
 					"on Linux\n", opt);
 			}
 			eal_usage(prgname);
-			return -1;
+			ret = -1;
+			goto out;
 		}
 	}
 
-	if (eal_adjust_config(&internal_config) != 0)
-		return -1;
+	if (eal_adjust_config(&internal_config) != 0) {
+		ret = -1;
+		goto out;
+	}
 
 	/* sanity checks */
 	if (eal_check_common_options(&internal_config) != 0) {
 		eal_usage(prgname);
-		return -1;
+		ret = -1;
+		goto out;
 	}
 
 	/* --xen-dom0 doesn't make sense with --socket-mem */ @@ -664,13 +694,20 @@ eal_parse_args(int argc, char **argv)
 		RTE_LOG(ERR, EAL, "Options --"OPT_SOCKET_MEM" cannot be specified "
 			"together with --"OPT_XEN_DOM0"\n");
 		eal_usage(prgname);
-		return -1;
+		ret = -1;
+		goto out;
 	}
 
 	if (optind >= 0)
 		argv[optind-1] = prgname;
 	ret = optind-1;
-	optind = 0; /* reset getopt lib */
+
+out:
+	/* restore getopt lib */
+	optind = old_optind;
+	optopt = old_optopt;
+	optarg = old_optarg;
+
 	return ret;
 }
 
--
2.1.2

Re: [dpdk-dev] [PATCH] eal: don't reset getopt lib

[Tiwei Bie]

On Thu, Oct 15, 2015 at 04:22:53PM +0000, Don Provan wrote:
> Looks perfect. Thanks!

Thanks! It's my pleasure. :-)

Best wishes,
Tiwei Bie

> -don
> 
> -----Original Message-----
> From: Tiwei Bie [mailto:btw@mail.ustc.edu.cn] 
> Sent: Thursday, October 15, 2015 4:46 AM
> To: Don Provan <dprovan@bivio.net>; bruce.richardson@intel.com; dev@dpdk.org
> Subject: [PATCH] eal: don't reset getopt lib
> 
> Someone may need to call rte_eal_init() with a fake argc/argv array in the middle of using getopt() to parse its own unrelated argc/argv parameters. So getopt lib shouldn't be reset by rte_eal_init().
> 
> Now eal will always save optind, optarg and optopt (and optreset on
> FreeBSD) at the beginning, initialize optind (and optreset on FreeBSD) to 1 before calling getopt_long(), then restore all values after.
> 
> Suggested-by: Don Provan <dprovan@bivio.net>
> Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
> Signed-off-by: Tiwei Bie <btw@mail.ustc.edu.cn>
> ---
>  lib/librte_eal/bsdapp/eal/eal.c   | 59 +++++++++++++++++++++++++++------
>  lib/librte_eal/linuxapp/eal/eal.c | 69 ++++++++++++++++++++++++++++++---------
>  2 files changed, 102 insertions(+), 26 deletions(-)
> 
> diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c index 1b6f705..bd09377 100644
> --- a/lib/librte_eal/bsdapp/eal/eal.c
> +++ b/lib/librte_eal/bsdapp/eal/eal.c
> @@ -312,8 +312,20 @@ eal_log_level_parse(int argc, char **argv)
>  	int opt;
>  	char **argvopt;
>  	int option_index;
> +	int old_optind;
> +	int old_optopt;
> +	int old_optreset;
> +	char *old_optarg;
> +
> +	/* save getopt lib */
> +	old_optind = optind;
> +	old_optopt = optopt;
> +	old_optreset = optreset;
> +	old_optarg = optarg;
>  
>  	argvopt = argv;
> +	optind = 1;
> +	optreset = 1;
>  
>  	eal_reset_internal_config(&internal_config);
>  
> @@ -334,7 +346,11 @@ eal_log_level_parse(int argc, char **argv)
>  			break;
>  	}
>  
> -	optind = 0; /* reset getopt lib */
> +	/* restore getopt lib */
> +	optind = old_optind;
> +	optopt = old_optopt;
> +	optreset = old_optreset;
> +	optarg = old_optarg;
>  }
>  
>  /* Parse the argument given in the command line of the application */ @@ -345,25 +361,37 @@ eal_parse_args(int argc, char **argv)
>  	char **argvopt;
>  	int option_index;
>  	char *prgname = argv[0];
> +	int old_optind;
> +	int old_optopt;
> +	int old_optreset;
> +	char *old_optarg;
> +
> +	/* save getopt lib */
> +	old_optind = optind;
> +	old_optopt = optopt;
> +	old_optreset = optreset;
> +	old_optarg = optarg;
>  
>  	argvopt = argv;
> +	optind = 1;
> +	optreset = 1;
>  
>  	while ((opt = getopt_long(argc, argvopt, eal_short_options,
>  				  eal_long_options, &option_index)) != EOF) {
>  
> -		int ret;
> -
>  		/* getopt is not happy, stop right now */
>  		if (opt == '?') {
>  			eal_usage(prgname);
> -			return -1;
> +			ret = -1;
> +			goto out;
>  		}
>  
>  		ret = eal_parse_common_option(opt, optarg, &internal_config);
>  		/* common parser is not happy */
>  		if (ret < 0) {
>  			eal_usage(prgname);
> -			return -1;
> +			ret = -1;
> +			goto out;
>  		}
>  		/* common parser handled this option */
>  		if (ret == 0)
> @@ -387,23 +415,34 @@ eal_parse_args(int argc, char **argv)
>  					"on FreeBSD\n", opt);
>  			}
>  			eal_usage(prgname);
> -			return -1;
> +			ret = -1;
> +			goto out;
>  		}
>  	}
>  
> -	if (eal_adjust_config(&internal_config) != 0)
> -		return -1;
> +	if (eal_adjust_config(&internal_config) != 0) {
> +		ret = -1;
> +		goto out;
> +	}
>  
>  	/* sanity checks */
>  	if (eal_check_common_options(&internal_config) != 0) {
>  		eal_usage(prgname);
> -		return -1;
> +		ret = -1;
> +		goto out;
>  	}
>  
>  	if (optind >= 0)
>  		argv[optind-1] = prgname;
>  	ret = optind-1;
> -	optind = 0; /* reset getopt lib */
> +
> +out:
> +	/* restore getopt lib */
> +	optind = old_optind;
> +	optopt = old_optopt;
> +	optreset = old_optreset;
> +	optarg = old_optarg;
> +
>  	return ret;
>  }
>  
> diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
> index 33e1067..4796030 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -505,8 +505,17 @@ eal_log_level_parse(int argc, char **argv)
>  	int opt;
>  	char **argvopt;
>  	int option_index;
> +	int old_optind;
> +	int old_optopt;
> +	char *old_optarg;
> +
> +	/* save getopt lib */
> +	old_optind = optind;
> +	old_optopt = optopt;
> +	old_optarg = optarg;
>  
>  	argvopt = argv;
> +	optind = 1;
>  
>  	eal_reset_internal_config(&internal_config);
>  
> @@ -527,7 +536,10 @@ eal_log_level_parse(int argc, char **argv)
>  			break;
>  	}
>  
> -	optind = 0; /* reset getopt lib */
> +	/* restore getopt lib */
> +	optind = old_optind;
> +	optopt = old_optopt;
> +	optarg = old_optarg;
>  }
>  
>  /* Parse the argument given in the command line of the application */ @@ -539,25 +551,34 @@ eal_parse_args(int argc, char **argv)
>  	int option_index;
>  	char *prgname = argv[0];
>  	struct shared_driver *solib;
> +	int old_optind;
> +	int old_optopt;
> +	char *old_optarg;
> +
> +	/* save getopt lib */
> +	old_optind = optind;
> +	old_optopt = optopt;
> +	old_optarg = optarg;
>  
>  	argvopt = argv;
> +	optind = 1;
>  
>  	while ((opt = getopt_long(argc, argvopt, eal_short_options,
>  				  eal_long_options, &option_index)) != EOF) {
>  
> -		int ret;
> -
>  		/* getopt is not happy, stop right now */
>  		if (opt == '?') {
>  			eal_usage(prgname);
> -			return -1;
> +			ret = -1;
> +			goto out;
>  		}
>  
>  		ret = eal_parse_common_option(opt, optarg, &internal_config);
>  		/* common parser is not happy */
>  		if (ret < 0) {
>  			eal_usage(prgname);
> -			return -1;
> +			ret = -1;
> +			goto out;
>  		}
>  		/* common parser handled this option */
>  		if (ret == 0)
> @@ -573,7 +594,8 @@ eal_parse_args(int argc, char **argv)
>  			solib = malloc(sizeof(*solib));
>  			if (solib == NULL) {
>  				RTE_LOG(ERR, EAL, "malloc(solib) failed\n");
> -				return -1;
> +				ret = -1;
> +				goto out;
>  			}
>  			memset(solib, 0, sizeof(*solib));
>  			strncpy(solib->name, optarg, PATH_MAX-1); @@ -589,7 +611,8 @@ eal_parse_args(int argc, char **argv)
>  			RTE_LOG(ERR, EAL, "Can't support DPDK app "
>  				"running on Dom0, please configure"
>  				" RTE_LIBRTE_XEN_DOM0=y\n");
> -			return -1;
> +			ret = -1;
> +			goto out;
>  #endif
>  			break;
>  
> @@ -606,7 +629,8 @@ eal_parse_args(int argc, char **argv)
>  				RTE_LOG(ERR, EAL, "invalid parameters for --"
>  						OPT_SOCKET_MEM "\n");
>  				eal_usage(prgname);
> -				return -1;
> +				ret = -1;
> +				goto out;
>  			}
>  			break;
>  
> @@ -615,7 +639,8 @@ eal_parse_args(int argc, char **argv)
>  				RTE_LOG(ERR, EAL, "invalid parameter for --"
>  						OPT_BASE_VIRTADDR "\n");
>  				eal_usage(prgname);
> -				return -1;
> +				ret = -1;
> +				goto out;
>  			}
>  			break;
>  
> @@ -624,7 +649,8 @@ eal_parse_args(int argc, char **argv)
>  				RTE_LOG(ERR, EAL, "invalid parameters for --"
>  						OPT_VFIO_INTR "\n");
>  				eal_usage(prgname);
> -				return -1;
> +				ret = -1;
> +				goto out;
>  			}
>  			break;
>  
> @@ -646,17 +672,21 @@ eal_parse_args(int argc, char **argv)
>  					"on Linux\n", opt);
>  			}
>  			eal_usage(prgname);
> -			return -1;
> +			ret = -1;
> +			goto out;
>  		}
>  	}
>  
> -	if (eal_adjust_config(&internal_config) != 0)
> -		return -1;
> +	if (eal_adjust_config(&internal_config) != 0) {
> +		ret = -1;
> +		goto out;
> +	}
>  
>  	/* sanity checks */
>  	if (eal_check_common_options(&internal_config) != 0) {
>  		eal_usage(prgname);
> -		return -1;
> +		ret = -1;
> +		goto out;
>  	}
>  
>  	/* --xen-dom0 doesn't make sense with --socket-mem */ @@ -664,13 +694,20 @@ eal_parse_args(int argc, char **argv)
>  		RTE_LOG(ERR, EAL, "Options --"OPT_SOCKET_MEM" cannot be specified "
>  			"together with --"OPT_XEN_DOM0"\n");
>  		eal_usage(prgname);
> -		return -1;
> +		ret = -1;
> +		goto out;
>  	}
>  
>  	if (optind >= 0)
>  		argv[optind-1] = prgname;
>  	ret = optind-1;
> -	optind = 0; /* reset getopt lib */
> +
> +out:
> +	/* restore getopt lib */
> +	optind = old_optind;
> +	optopt = old_optopt;
> +	optarg = old_optarg;
> +
>  	return ret;
>  }
>  
> --
> 2.1.2
> 
> 

Re: [dpdk-dev] [PATCH] eal: don't reset getopt lib

[Bruce Richardson]

On Thu, Oct 15, 2015 at 07:46:04PM +0800, Tiwei Bie wrote:
> Someone may need to call rte_eal_init() with a fake argc/argv array
> in the middle of using getopt() to parse its own unrelated argc/argv
> parameters. So getopt lib shouldn't be reset by rte_eal_init().
> 
> Now eal will always save optind, optarg and optopt (and optreset on
> FreeBSD) at the beginning, initialize optind (and optreset on FreeBSD)
> to 1 before calling getopt_long(), then restore all values after.

This patch looks good overall. Minor comment inline below.

/Bruce
> 
> Suggested-by: Don Provan <dprovan@bivio.net>
> Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
> Signed-off-by: Tiwei Bie <btw@mail.ustc.edu.cn>
> ---
>  lib/librte_eal/bsdapp/eal/eal.c   | 59 +++++++++++++++++++++++++++------
>  lib/librte_eal/linuxapp/eal/eal.c | 69 ++++++++++++++++++++++++++++++---------
>  2 files changed, 102 insertions(+), 26 deletions(-)
> 
> diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
> index 1b6f705..bd09377 100644
> --- a/lib/librte_eal/bsdapp/eal/eal.c
> +++ b/lib/librte_eal/bsdapp/eal/eal.c
> @@ -312,8 +312,20 @@ eal_log_level_parse(int argc, char **argv)
>  	int opt;
>  	char **argvopt;
>  	int option_index;
> +	int old_optind;
> +	int old_optopt;
> +	int old_optreset;
> +	char *old_optarg;
> +
> +	/* save getopt lib */
> +	old_optind = optind;
> +	old_optopt = optopt;
> +	old_optreset = optreset;
> +	old_optarg = optarg;

Rather than adding in 10 lines, you could shorten this, and provide additional
compiler hints by merging these assignments into the variable declarations, and
then making the vars const. e.g.

	const int old_optind = optind;

>  
>  	argvopt = argv;
> +	optind = 1;
> +	optreset = 1;
<snip>

Re: [dpdk-dev] [PATCH] eal: don't reset getopt lib

[Tiwei Bie]

On Mon, Oct 19, 2015 at 11:36:41AM +0100, Bruce Richardson wrote:
> On Thu, Oct 15, 2015 at 07:46:04PM +0800, Tiwei Bie wrote:
> > Someone may need to call rte_eal_init() with a fake argc/argv array
> > in the middle of using getopt() to parse its own unrelated argc/argv
> > parameters. So getopt lib shouldn't be reset by rte_eal_init().
> > 
> > Now eal will always save optind, optarg and optopt (and optreset on
> > FreeBSD) at the beginning, initialize optind (and optreset on FreeBSD)
> > to 1 before calling getopt_long(), then restore all values after.
> 
> This patch looks good overall. Minor comment inline below.
> 

Thanks for review! :-)

Best regards,
Tiwei Bie

[dpdk-dev] [PATCH v2] eal: don't reset getopt lib

[Tiwei Bie]

Someone may need to call rte_eal_init() with a fake argc/argv array
in the middle of using getopt() to parse its own unrelated argc/argv
parameters. So getopt lib shouldn't be reset by rte_eal_init().

Now eal will always save optind, optarg and optopt (and optreset on
FreeBSD) at the beginning, initialize optind (and optreset on FreeBSD)
to 1 before calling getopt_long(), then restore all values after.

Suggested-by: Don Provan <dprovan@bivio.net>
Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
Signed-off-by: Tiwei Bie <btw@mail.ustc.edu.cn>
Reviewed-by: Don Provan <dprovan@bivio.net>
Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>
---
v2:
 - constify some variables

 lib/librte_eal/bsdapp/eal/eal.c   | 47 ++++++++++++++++++++++++-------
 lib/librte_eal/linuxapp/eal/eal.c | 59 ++++++++++++++++++++++++++++-----------
 2 files changed, 80 insertions(+), 26 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index 1b6f705..b356517 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -312,8 +312,14 @@ eal_log_level_parse(int argc, char **argv)
 	int opt;
 	char **argvopt;
 	int option_index;
+	const int old_optind = optind;
+	const int old_optopt = optopt;
+	const int old_optreset = optreset;
+	char * const old_optarg = optarg;
 
 	argvopt = argv;
+	optind = 1;
+	optreset = 1;
 
 	eal_reset_internal_config(&internal_config);
 
@@ -334,7 +340,11 @@ eal_log_level_parse(int argc, char **argv)
 			break;
 	}
 
-	optind = 0; /* reset getopt lib */
+	/* restore getopt lib */
+	optind = old_optind;
+	optopt = old_optopt;
+	optreset = old_optreset;
+	optarg = old_optarg;
 }
 
 /* Parse the argument given in the command line of the application */
@@ -345,25 +355,31 @@ eal_parse_args(int argc, char **argv)
 	char **argvopt;
 	int option_index;
 	char *prgname = argv[0];
+	const int old_optind = optind;
+	const int old_optopt = optopt;
+	const int old_optreset = optreset;
+	char * const old_optarg = optarg;
 
 	argvopt = argv;
+	optind = 1;
+	optreset = 1;
 
 	while ((opt = getopt_long(argc, argvopt, eal_short_options,
 				  eal_long_options, &option_index)) != EOF) {
 
-		int ret;
-
 		/* getopt is not happy, stop right now */
 		if (opt == '?') {
 			eal_usage(prgname);
-			return -1;
+			ret = -1;
+			goto out;
 		}
 
 		ret = eal_parse_common_option(opt, optarg, &internal_config);
 		/* common parser is not happy */
 		if (ret < 0) {
 			eal_usage(prgname);
-			return -1;
+			ret = -1;
+			goto out;
 		}
 		/* common parser handled this option */
 		if (ret == 0)
@@ -387,23 +403,34 @@ eal_parse_args(int argc, char **argv)
 					"on FreeBSD\n", opt);
 			}
 			eal_usage(prgname);
-			return -1;
+			ret = -1;
+			goto out;
 		}
 	}
 
-	if (eal_adjust_config(&internal_config) != 0)
-		return -1;
+	if (eal_adjust_config(&internal_config) != 0) {
+		ret = -1;
+		goto out;
+	}
 
 	/* sanity checks */
 	if (eal_check_common_options(&internal_config) != 0) {
 		eal_usage(prgname);
-		return -1;
+		ret = -1;
+		goto out;
 	}
 
 	if (optind >= 0)
 		argv[optind-1] = prgname;
 	ret = optind-1;
-	optind = 0; /* reset getopt lib */
+
+out:
+	/* restore getopt lib */
+	optind = old_optind;
+	optopt = old_optopt;
+	optreset = old_optreset;
+	optarg = old_optarg;
+
 	return ret;
 }
 
diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index 33e1067..89a81bd 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -505,8 +505,12 @@ eal_log_level_parse(int argc, char **argv)
 	int opt;
 	char **argvopt;
 	int option_index;
+	const int old_optind = optind;
+	const int old_optopt = optopt;
+	char * const old_optarg = optarg;
 
 	argvopt = argv;
+	optind = 1;
 
 	eal_reset_internal_config(&internal_config);
 
@@ -527,7 +531,10 @@ eal_log_level_parse(int argc, char **argv)
 			break;
 	}
 
-	optind = 0; /* reset getopt lib */
+	/* restore getopt lib */
+	optind = old_optind;
+	optopt = old_optopt;
+	optarg = old_optarg;
 }
 
 /* Parse the argument given in the command line of the application */
@@ -539,25 +546,29 @@ eal_parse_args(int argc, char **argv)
 	int option_index;
 	char *prgname = argv[0];
 	struct shared_driver *solib;
+	const int old_optind = optind;
+	const int old_optopt = optopt;
+	char * const old_optarg = optarg;
 
 	argvopt = argv;
+	optind = 1;
 
 	while ((opt = getopt_long(argc, argvopt, eal_short_options,
 				  eal_long_options, &option_index)) != EOF) {
 
-		int ret;
-
 		/* getopt is not happy, stop right now */
 		if (opt == '?') {
 			eal_usage(prgname);
-			return -1;
+			ret = -1;
+			goto out;
 		}
 
 		ret = eal_parse_common_option(opt, optarg, &internal_config);
 		/* common parser is not happy */
 		if (ret < 0) {
 			eal_usage(prgname);
-			return -1;
+			ret = -1;
+			goto out;
 		}
 		/* common parser handled this option */
 		if (ret == 0)
@@ -573,7 +584,8 @@ eal_parse_args(int argc, char **argv)
 			solib = malloc(sizeof(*solib));
 			if (solib == NULL) {
 				RTE_LOG(ERR, EAL, "malloc(solib) failed\n");
-				return -1;
+				ret = -1;
+				goto out;
 			}
 			memset(solib, 0, sizeof(*solib));
 			strncpy(solib->name, optarg, PATH_MAX-1);
@@ -589,7 +601,8 @@ eal_parse_args(int argc, char **argv)
 			RTE_LOG(ERR, EAL, "Can't support DPDK app "
 				"running on Dom0, please configure"
 				" RTE_LIBRTE_XEN_DOM0=y\n");
-			return -1;
+			ret = -1;
+			goto out;
 #endif
 			break;
 
@@ -606,7 +619,8 @@ eal_parse_args(int argc, char **argv)
 				RTE_LOG(ERR, EAL, "invalid parameters for --"
 						OPT_SOCKET_MEM "\n");
 				eal_usage(prgname);
-				return -1;
+				ret = -1;
+				goto out;
 			}
 			break;
 
@@ -615,7 +629,8 @@ eal_parse_args(int argc, char **argv)
 				RTE_LOG(ERR, EAL, "invalid parameter for --"
 						OPT_BASE_VIRTADDR "\n");
 				eal_usage(prgname);
-				return -1;
+				ret = -1;
+				goto out;
 			}
 			break;
 
@@ -624,7 +639,8 @@ eal_parse_args(int argc, char **argv)
 				RTE_LOG(ERR, EAL, "invalid parameters for --"
 						OPT_VFIO_INTR "\n");
 				eal_usage(prgname);
-				return -1;
+				ret = -1;
+				goto out;
 			}
 			break;
 
@@ -646,17 +662,21 @@ eal_parse_args(int argc, char **argv)
 					"on Linux\n", opt);
 			}
 			eal_usage(prgname);
-			return -1;
+			ret = -1;
+			goto out;
 		}
 	}
 
-	if (eal_adjust_config(&internal_config) != 0)
-		return -1;
+	if (eal_adjust_config(&internal_config) != 0) {
+		ret = -1;
+		goto out;
+	}
 
 	/* sanity checks */
 	if (eal_check_common_options(&internal_config) != 0) {
 		eal_usage(prgname);
-		return -1;
+		ret = -1;
+		goto out;
 	}
 
 	/* --xen-dom0 doesn't make sense with --socket-mem */
@@ -664,13 +684,20 @@ eal_parse_args(int argc, char **argv)
 		RTE_LOG(ERR, EAL, "Options --"OPT_SOCKET_MEM" cannot be specified "
 			"together with --"OPT_XEN_DOM0"\n");
 		eal_usage(prgname);
-		return -1;
+		ret = -1;
+		goto out;
 	}
 
 	if (optind >= 0)
 		argv[optind-1] = prgname;
 	ret = optind-1;
-	optind = 0; /* reset getopt lib */
+
+out:
+	/* restore getopt lib */
+	optind = old_optind;
+	optopt = old_optopt;
+	optarg = old_optarg;
+
 	return ret;
 }
 
-- 
2.1.2

Re: [dpdk-dev] [PATCH v2] eal: don't reset getopt lib

[Bruce Richardson]

On Mon, Oct 19, 2015 at 09:13:10PM +0800, Tiwei Bie wrote:
> Someone may need to call rte_eal_init() with a fake argc/argv array
> in the middle of using getopt() to parse its own unrelated argc/argv
> parameters. So getopt lib shouldn't be reset by rte_eal_init().
> 
> Now eal will always save optind, optarg and optopt (and optreset on
> FreeBSD) at the beginning, initialize optind (and optreset on FreeBSD)
> to 1 before calling getopt_long(), then restore all values after.
> 
> Suggested-by: Don Provan <dprovan@bivio.net>
> Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
> Signed-off-by: Tiwei Bie <btw@mail.ustc.edu.cn>
> Reviewed-by: Don Provan <dprovan@bivio.net>
> Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [dpdk-dev] [PATCH v2] eal: don't reset getopt lib

[David Marchand]

On Mon, Oct 19, 2015 at 3:16 PM, Bruce Richardson <
bruce.richardson@intel.com> wrote:

> On Mon, Oct 19, 2015 at 09:13:10PM +0800, Tiwei Bie wrote:
> > Someone may need to call rte_eal_init() with a fake argc/argv array
> > in the middle of using getopt() to parse its own unrelated argc/argv
> > parameters. So getopt lib shouldn't be reset by rte_eal_init().
> >
> > Now eal will always save optind, optarg and optopt (and optreset on
> > FreeBSD) at the beginning, initialize optind (and optreset on FreeBSD)
> > to 1 before calling getopt_long(), then restore all values after.
> >
> > Suggested-by: Don Provan <dprovan@bivio.net>
> > Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
> > Signed-off-by: Tiwei Bie <btw@mail.ustc.edu.cn>
> > Reviewed-by: Don Provan <dprovan@bivio.net>
> > Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>
>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>

Acked-by: David Marchand <david.marchand@6wind.com>

Thanks Tiwei.

-- 
David Marchand

Re: [dpdk-dev] [PATCH v2] eal: don't reset getopt lib

[Tiwei Bie]

On Wed, Oct 21, 2015 at 07:33:42AM +0200, David Marchand wrote:
> On Mon, Oct 19, 2015 at 3:16 PM, Bruce Richardson <bruce.richardson@intel.com>
> wrote:
> 
> > On Mon, Oct 19, 2015 at 09:13:10PM +0800, Tiwei Bie wrote:
> > > Someone may need to call rte_eal_init() with a fake argc/argv array
> > > in the middle of using getopt() to parse its own unrelated argc/argv
> > > parameters. So getopt lib shouldn't be reset by rte_eal_init().
> > >
> > > Now eal will always save optind, optarg and optopt (and optreset on
> > > FreeBSD) at the beginning, initialize optind (and optreset on FreeBSD)
> > > to 1 before calling getopt_long(), then restore all values after.
> > >
> > > Suggested-by: Don Provan <dprovan@bivio.net>
> > > Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
> > > Signed-off-by: Tiwei Bie <btw@mail.ustc.edu.cn>
> > > Reviewed-by: Don Provan <dprovan@bivio.net>
> > > Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>
> > 
> > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > 
> 
> Acked-by: David Marchand <david.marchand@6wind.com>
> 
> Thanks Tiwei.
> 

My pleasure. Thanks! :-)

Best regards,
Tiwei Bie

Re: [dpdk-dev] [PATCH v2] eal: don't reset getopt lib

[Thomas Monjalon]

> > > Someone may need to call rte_eal_init() with a fake argc/argv array
> > > in the middle of using getopt() to parse its own unrelated argc/argv
> > > parameters. So getopt lib shouldn't be reset by rte_eal_init().
> > >
> > > Now eal will always save optind, optarg and optopt (and optreset on
> > > FreeBSD) at the beginning, initialize optind (and optreset on FreeBSD)
> > > to 1 before calling getopt_long(), then restore all values after.
> > >
> > > Suggested-by: Don Provan <dprovan@bivio.net>
> > > Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
> > > Signed-off-by: Tiwei Bie <btw@mail.ustc.edu.cn>
> > > Reviewed-by: Don Provan <dprovan@bivio.net>
> > > Reviewed-by: Bruce Richardson <bruce.richardson@intel.com>
> >
> > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> Acked-by: David Marchand <david.marchand@6wind.com>

Applied, thanks