diff mbox

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

Message ID 1444909564-53691-1-git-send-email-btw@mail.ustc.edu.cn (mailing list archive)
State Superseded, archived
Headers

Commit Message

Tiwei Bie Oct. 15, 2015, 11:46 a.m. UTC 
  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(-)

Comments

Bruce Richardson Oct. 19, 2015, 10:36 a.m. UTC | #1 
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>
Tiwei Bie Oct. 19, 2015, 1:15 p.m. UTC | #2 
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
diff mbox

Patch

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;
 }