| Message ID | 1462984942-53326-13-git-send-email-christophe.milard@linaro.org |
|---|---|
| State | New |
| Headers | show |
Not sure which patches you want to squash here: "helper: adding a function to merge getopt parameters" and "parsing the complete set of options" ? Aren't these 2 different steps? They were at least tested separately... Or did I misunderstood? Christophe. On 12 May 2016 at 10:21, Savolainen, Petri (Nokia - FI/Espoo) < petri.savolainen@nokia.com> wrote: > It would be really good to squash these patches introducing (and then > modifying the previously introduced) new helper function prototypes into a > single patch. This way it's hard to see the complete picture what functions > were actually added into helper/linux.h > > > -Petri > > > > -----Original Message----- > > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of > > Christophe Milard > > Sent: Wednesday, May 11, 2016 7:42 PM > > To: brian.brooks@linaro.org; mike.holmes@linaro.org; lng- > > odp@lists.linaro.org > > Subject: [lng-odp] [PATCHv6 12/38] helper: parsing the complete set of > > options > > > > The odph_parse_options() function is given the ability to receive > > getopt command line description parameter from it caller, hence allowing > > the caller to have some command line parameter(s). > > The caller shall first call odph_parse_options() with its own parameter > > description as parameter. > > odph_parse_options() is then checking the complete set of options, > > issuing error message for unknown options (those being neither a caller's > > valid command line option or a helper valid command line option), > > and collecting the sementic of helper options. > > Then the caller shall parse the sementic of its own options, > > with the opterr variable set to zero (hence ignoring helper options). > > > > Signed-off-by: Christophe Milard <christophe.milard@linaro.org> > > --- > > helper/include/odp/helper/linux.h | 16 +++++++++++++++- > > helper/linux.c | 24 +++++++++++++++++++----- > > helper/test/odpthreads.c | 2 +- > > test/validation/common/odp_cunit_common.c | 2 +- > > 4 files changed, 36 insertions(+), 8 deletions(-) > > > > diff --git a/helper/include/odp/helper/linux.h > > b/helper/include/odp/helper/linux.h > > index 9767af4..71c8027 100644 > > --- a/helper/include/odp/helper/linux.h > > +++ b/helper/include/odp/helper/linux.h > > @@ -25,6 +25,7 @@ extern "C" { > > #include <odp_api.h> > > > > #include <pthread.h> > > +#include <getopt.h> > > #include <sys/types.h> > > > > /** Thread parameter for Linux pthreads and processes */ > > @@ -241,15 +242,28 @@ int odph_merge_getopt_options(const char > > *shortopts1, > > * Parse linux helper options > > * > > * Parse the command line options. Pick up options meant for the helper > > itself. > > + * If the caller is also having a set of option to parse, it should > > include > > + * their description here (shortopts desribes the short options and > > longopts > > + * describes the long options, as for getopt_long()). > > + * This function will issue errors on unknown arguments, so callers > > failing > > + * to pass their own command line options description here will see > their > > + * options rejected. > > + * (the caller wants to set opterr to zero when parsing its own stuff > > + * with getopts to avoid reacting on helper's options). > > * > > * @param argc argument count > > * @param argv argument values > > + * @param caller_shortopts caller's set of short options (string). or > > NULL. > > + * @param caller_longopts caller's set of long options (getopt option > > array). > > + * or NULL. > > * > > * @return On success: 0 > > * On failure: -1 (failure occurs only if a value passed for a > > helper > > * option is invalid. callers cannot have own > options) > > */ > > -int odph_parse_options(int argc, char *argv[]); > > +int odph_parse_options(int argc, char *argv[], > > + const char *caller_shortopts, > > + const struct option *caller_longopts); > > #ifdef __cplusplus > > } > > #endif > > diff --git a/helper/linux.c b/helper/linux.c > > index d1b7825..5fc09a1 100644 > > --- a/helper/linux.c > > +++ b/helper/linux.c > > @@ -16,7 +16,6 @@ > > #include <stdlib.h> > > #include <string.h> > > #include <stdio.h> > > -#include <getopt.h> > > > > #include <odp_api.h> > > #include <odp/helper/linux.h> > > @@ -576,27 +575,39 @@ int odph_merge_getopt_options(const char > > *shortopts1, > > /* > > * Parse command line options to extract options affecting helpers. > > */ > > -int odph_parse_options(int argc, char *argv[]) > > +int odph_parse_options(int argc, char *argv[], > > + const char *caller_shortopts, > > + const struct option *caller_longopts) > > { > > int c; > > + char *shortopts; > > + struct option *longopts; > > > > - static struct option long_options[] = { > > + static struct option helper_long_options[] = { > > /* These options set a flag. */ > > {"odph_proc", no_argument, &helper_options.proc, 1}, > > {"odph_thread", no_argument, &helper_options.thrd, 1}, > > {0, 0, 0, 0} > > }; > > > > + static char *helper_short_options = ""; > > + > > /* defaults: */ > > helper_options.proc = false; > > helper_options.thrd = false; > > > > + /* merge caller's command line options descriptions with helper's: > > */ > > + if (odph_merge_getopt_options(caller_shortopts, > > helper_short_options, > > + caller_longopts, helper_long_options, > > + &shortopts, &longopts) < 0) > > + return -1; > > + > > while (1) { > > /* getopt_long stores the option index here. */ > > int option_index = 0; > > > > - c = getopt_long (argc, argv, "", > > - long_options, &option_index); > > + c = getopt_long (argc, argv, > > + shortopts, longopts, &option_index); > > > > /* Detect the end of the options. */ > > if (c == -1) > > @@ -605,5 +616,8 @@ int odph_parse_options(int argc, char *argv[]) > > > > optind = 0; /* caller expects this to be zero if it parses too*/ > > > > + free(shortopts); > > + free(longopts); > > + > > return 0; > > } > > diff --git a/helper/test/odpthreads.c b/helper/test/odpthreads.c > > index 369da62..bba4fa5 100644 > > --- a/helper/test/odpthreads.c > > +++ b/helper/test/odpthreads.c > > @@ -39,7 +39,7 @@ int main(int argc, char *argv[]) > > char cpumaskstr[ODP_CPUMASK_STR_SIZE]; > > > > /* let helper collect its own arguments (e.g. --odph_proc) */ > > - odph_parse_options(argc, argv); > > + odph_parse_options(argc, argv, NULL, NULL); > > > > if (odp_init_global(&instance, NULL, NULL)) { > > LOG_ERR("Error: ODP global init failed.\n"); > > diff --git a/test/validation/common/odp_cunit_common.c > > b/test/validation/common/odp_cunit_common.c > > index be23c6b..7df9aa6 100644 > > --- a/test/validation/common/odp_cunit_common.c > > +++ b/test/validation/common/odp_cunit_common.c > > @@ -354,5 +354,5 @@ int odp_cunit_register(odp_suiteinfo_t testsuites[]) > > */ > > int odp_cunit_parse_options(int argc, char *argv[]) > > { > > - return odph_parse_options(argc, argv); > > + return odph_parse_options(argc, argv, NULL, NULL); > > } > > -- > > 2.5.0 > > > > _______________________________________________ > > lng-odp mailing list > > lng-odp@lists.linaro.org > > https://lists.linaro.org/mailman/listinfo/lng-odp >
On 12 May 2016 at 12:54, Savolainen, Petri (Nokia - FI/Espoo) < petri.savolainen@nokia.com> wrote: > Those that add new definitions into helper/linux.h and depend on each > other, so that the entire proposed spec is easy to see today and later on > from git history. Since those are new calls, it should be easy to introduce > those in their final form. Patches (e.g. 1 and 12) should not expose > intermediate, unnecessary steps done during development of a new feature, > but the new feature in its final form. > > > > At least 1 and 12, since those introduce and modify the same function. > Maybe 11 if odph_merge_getopt_options() is always needed with odph_parse_options(). > Is merge function defined for the user or is it internal to the helper? > Does not make sense to me: as explained in the cover letter, patch 1 to 10 is introducing the ability to parse options in helpers. This is applied to the tests (patch 6 to 10) as none of the tests have their own options (as of today) Patch 11... enables the caller helper to have its own options. This is then applied to exemple/perf... which already had their own option, and hence could not be handled as the tests were. > > > Maybe 4 and 5, because the first introduces types that the other depends > on. > That could be done if you wish > > > Maybe 5 and 12 (at least 12 should be before 5), if odph_parse_options() > must be always called before odph_odpthreads_create(). > same problem as with patch 1 and 12: Patch 1 to 10 is one group. 11 to 37 another. I hoped the cover-letter made that clear. Christophe. > > > > > > > -Petri > > > > > > *From:* lng-odp [mailto:lng-odp-bounces@lists.linaro.org] *On Behalf Of *Christophe > Milard > *Sent:* Thursday, May 12, 2016 12:05 PM > *To:* Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia.com> > *Cc:* lng-odp@lists.linaro.org > *Subject:* Re: [lng-odp] [PATCHv6 12/38] helper: parsing the complete set > of options > > > > Not sure which patches you want to squash here: "helper: adding a function > to merge getopt parameters" and "parsing the complete set of options" ? > > Aren't these 2 different steps? They were at least tested separately... > > Or did I misunderstood? > > > > Christophe. > > > > On 12 May 2016 at 10:21, Savolainen, Petri (Nokia - FI/Espoo) < > petri.savolainen@nokia.com> wrote: > > It would be really good to squash these patches introducing (and then > modifying the previously introduced) new helper function prototypes into a > single patch. This way it's hard to see the complete picture what functions > were actually added into helper/linux.h > > > -Petri > > > > > -----Original Message----- > > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of > > Christophe Milard > > Sent: Wednesday, May 11, 2016 7:42 PM > > To: brian.brooks@linaro.org; mike.holmes@linaro.org; lng- > > odp@lists.linaro.org > > Subject: [lng-odp] [PATCHv6 12/38] helper: parsing the complete set of > > options > > > > The odph_parse_options() function is given the ability to receive > > getopt command line description parameter from it caller, hence allowing > > the caller to have some command line parameter(s). > > The caller shall first call odph_parse_options() with its own parameter > > description as parameter. > > odph_parse_options() is then checking the complete set of options, > > issuing error message for unknown options (those being neither a caller's > > valid command line option or a helper valid command line option), > > and collecting the sementic of helper options. > > Then the caller shall parse the sementic of its own options, > > with the opterr variable set to zero (hence ignoring helper options). > > > > Signed-off-by: Christophe Milard <christophe.milard@linaro.org> > > --- > > helper/include/odp/helper/linux.h | 16 +++++++++++++++- > > helper/linux.c | 24 +++++++++++++++++++----- > > helper/test/odpthreads.c | 2 +- > > test/validation/common/odp_cunit_common.c | 2 +- > > 4 files changed, 36 insertions(+), 8 deletions(-) > > > > diff --git a/helper/include/odp/helper/linux.h > > b/helper/include/odp/helper/linux.h > > index 9767af4..71c8027 100644 > > --- a/helper/include/odp/helper/linux.h > > +++ b/helper/include/odp/helper/linux.h > > @@ -25,6 +25,7 @@ extern "C" { > > #include <odp_api.h> > > > > #include <pthread.h> > > +#include <getopt.h> > > #include <sys/types.h> > > > > /** Thread parameter for Linux pthreads and processes */ > > @@ -241,15 +242,28 @@ int odph_merge_getopt_options(const char > > *shortopts1, > > * Parse linux helper options > > * > > * Parse the command line options. Pick up options meant for the helper > > itself. > > + * If the caller is also having a set of option to parse, it should > > include > > + * their description here (shortopts desribes the short options and > > longopts > > + * describes the long options, as for getopt_long()). > > + * This function will issue errors on unknown arguments, so callers > > failing > > + * to pass their own command line options description here will see > their > > + * options rejected. > > + * (the caller wants to set opterr to zero when parsing its own stuff > > + * with getopts to avoid reacting on helper's options). > > * > > * @param argc argument count > > * @param argv argument values > > + * @param caller_shortopts caller's set of short options (string). or > > NULL. > > + * @param caller_longopts caller's set of long options (getopt option > > array). > > + * or NULL. > > * > > * @return On success: 0 > > * On failure: -1 (failure occurs only if a value passed for a > > helper > > * option is invalid. callers cannot have own > options) > > */ > > -int odph_parse_options(int argc, char *argv[]); > > +int odph_parse_options(int argc, char *argv[], > > + const char *caller_shortopts, > > + const struct option *caller_longopts); > > #ifdef __cplusplus > > } > > #endif > > diff --git a/helper/linux.c b/helper/linux.c > > index d1b7825..5fc09a1 100644 > > --- a/helper/linux.c > > +++ b/helper/linux.c > > @@ -16,7 +16,6 @@ > > #include <stdlib.h> > > #include <string.h> > > #include <stdio.h> > > -#include <getopt.h> > > > > #include <odp_api.h> > > #include <odp/helper/linux.h> > > @@ -576,27 +575,39 @@ int odph_merge_getopt_options(const char > > *shortopts1, > > /* > > * Parse command line options to extract options affecting helpers. > > */ > > -int odph_parse_options(int argc, char *argv[]) > > +int odph_parse_options(int argc, char *argv[], > > + const char *caller_shortopts, > > + const struct option *caller_longopts) > > { > > int c; > > + char *shortopts; > > + struct option *longopts; > > > > - static struct option long_options[] = { > > + static struct option helper_long_options[] = { > > /* These options set a flag. */ > > {"odph_proc", no_argument, &helper_options.proc, 1}, > > {"odph_thread", no_argument, &helper_options.thrd, 1}, > > {0, 0, 0, 0} > > }; > > > > + static char *helper_short_options = ""; > > + > > /* defaults: */ > > helper_options.proc = false; > > helper_options.thrd = false; > > > > + /* merge caller's command line options descriptions with helper's: > > */ > > + if (odph_merge_getopt_options(caller_shortopts, > > helper_short_options, > > + caller_longopts, helper_long_options, > > + &shortopts, &longopts) < 0) > > + return -1; > > + > > while (1) { > > /* getopt_long stores the option index here. */ > > int option_index = 0; > > > > - c = getopt_long (argc, argv, "", > > - long_options, &option_index); > > + c = getopt_long (argc, argv, > > + shortopts, longopts, &option_index); > > > > /* Detect the end of the options. */ > > if (c == -1) > > @@ -605,5 +616,8 @@ int odph_parse_options(int argc, char *argv[]) > > > > optind = 0; /* caller expects this to be zero if it parses too*/ > > > > + free(shortopts); > > + free(longopts); > > + > > return 0; > > } > > diff --git a/helper/test/odpthreads.c b/helper/test/odpthreads.c > > index 369da62..bba4fa5 100644 > > --- a/helper/test/odpthreads.c > > +++ b/helper/test/odpthreads.c > > @@ -39,7 +39,7 @@ int main(int argc, char *argv[]) > > char cpumaskstr[ODP_CPUMASK_STR_SIZE]; > > > > /* let helper collect its own arguments (e.g. --odph_proc) */ > > - odph_parse_options(argc, argv); > > + odph_parse_options(argc, argv, NULL, NULL); > > > > if (odp_init_global(&instance, NULL, NULL)) { > > LOG_ERR("Error: ODP global init failed.\n"); > > diff --git a/test/validation/common/odp_cunit_common.c > > b/test/validation/common/odp_cunit_common.c > > index be23c6b..7df9aa6 100644 > > --- a/test/validation/common/odp_cunit_common.c > > +++ b/test/validation/common/odp_cunit_common.c > > @@ -354,5 +354,5 @@ int odp_cunit_register(odp_suiteinfo_t testsuites[]) > > */ > > int odp_cunit_parse_options(int argc, char *argv[]) > > { > > - return odph_parse_options(argc, argv); > > + return odph_parse_options(argc, argv, NULL, NULL); > > } > > -- > > 2.5.0 > > > > > _______________________________________________ > > lng-odp mailing list > > lng-odp@lists.linaro.org > > https://lists.linaro.org/mailman/listinfo/lng-odp > > >
diff --git a/helper/include/odp/helper/linux.h b/helper/include/odp/helper/linux.h index 9767af4..71c8027 100644 --- a/helper/include/odp/helper/linux.h +++ b/helper/include/odp/helper/linux.h @@ -25,6 +25,7 @@ extern "C" { #include <odp_api.h> #include <pthread.h> +#include <getopt.h> #include <sys/types.h> /** Thread parameter for Linux pthreads and processes */ @@ -241,15 +242,28 @@ int odph_merge_getopt_options(const char *shortopts1, * Parse linux helper options * * Parse the command line options. Pick up options meant for the helper itself. + * If the caller is also having a set of option to parse, it should include + * their description here (shortopts desribes the short options and longopts + * describes the long options, as for getopt_long()). + * This function will issue errors on unknown arguments, so callers failing + * to pass their own command line options description here will see their + * options rejected. + * (the caller wants to set opterr to zero when parsing its own stuff + * with getopts to avoid reacting on helper's options). * * @param argc argument count * @param argv argument values + * @param caller_shortopts caller's set of short options (string). or NULL. + * @param caller_longopts caller's set of long options (getopt option array). + * or NULL. * * @return On success: 0 * On failure: -1 (failure occurs only if a value passed for a helper * option is invalid. callers cannot have own options) */ -int odph_parse_options(int argc, char *argv[]); +int odph_parse_options(int argc, char *argv[], + const char *caller_shortopts, + const struct option *caller_longopts); #ifdef __cplusplus } #endif diff --git a/helper/linux.c b/helper/linux.c index d1b7825..5fc09a1 100644 --- a/helper/linux.c +++ b/helper/linux.c @@ -16,7 +16,6 @@ #include <stdlib.h> #include <string.h> #include <stdio.h> -#include <getopt.h> #include <odp_api.h> #include <odp/helper/linux.h> @@ -576,27 +575,39 @@ int odph_merge_getopt_options(const char *shortopts1, /* * Parse command line options to extract options affecting helpers. */ -int odph_parse_options(int argc, char *argv[]) +int odph_parse_options(int argc, char *argv[], + const char *caller_shortopts, + const struct option *caller_longopts) { int c; + char *shortopts; + struct option *longopts; - static struct option long_options[] = { + static struct option helper_long_options[] = { /* These options set a flag. */ {"odph_proc", no_argument, &helper_options.proc, 1}, {"odph_thread", no_argument, &helper_options.thrd, 1}, {0, 0, 0, 0} }; + static char *helper_short_options = ""; + /* defaults: */ helper_options.proc = false; helper_options.thrd = false; + /* merge caller's command line options descriptions with helper's: */ + if (odph_merge_getopt_options(caller_shortopts, helper_short_options, + caller_longopts, helper_long_options, + &shortopts, &longopts) < 0) + return -1; + while (1) { /* getopt_long stores the option index here. */ int option_index = 0; - c = getopt_long (argc, argv, "", - long_options, &option_index); + c = getopt_long (argc, argv, + shortopts, longopts, &option_index); /* Detect the end of the options. */ if (c == -1) @@ -605,5 +616,8 @@ int odph_parse_options(int argc, char *argv[]) optind = 0; /* caller expects this to be zero if it parses too*/ + free(shortopts); + free(longopts); + return 0; } diff --git a/helper/test/odpthreads.c b/helper/test/odpthreads.c index 369da62..bba4fa5 100644 --- a/helper/test/odpthreads.c +++ b/helper/test/odpthreads.c @@ -39,7 +39,7 @@ int main(int argc, char *argv[]) char cpumaskstr[ODP_CPUMASK_STR_SIZE]; /* let helper collect its own arguments (e.g. --odph_proc) */ - odph_parse_options(argc, argv); + odph_parse_options(argc, argv, NULL, NULL); if (odp_init_global(&instance, NULL, NULL)) { LOG_ERR("Error: ODP global init failed.\n"); diff --git a/test/validation/common/odp_cunit_common.c b/test/validation/common/odp_cunit_common.c index be23c6b..7df9aa6 100644 --- a/test/validation/common/odp_cunit_common.c +++ b/test/validation/common/odp_cunit_common.c @@ -354,5 +354,5 @@ int odp_cunit_register(odp_suiteinfo_t testsuites[]) */ int odp_cunit_parse_options(int argc, char *argv[]) { - return odph_parse_options(argc, argv); + return odph_parse_options(argc, argv, NULL, NULL); }
The odph_parse_options() function is given the ability to receive getopt command line description parameter from it caller, hence allowing the caller to have some command line parameter(s). The caller shall first call odph_parse_options() with its own parameter description as parameter. odph_parse_options() is then checking the complete set of options, issuing error message for unknown options (those being neither a caller's valid command line option or a helper valid command line option), and collecting the sementic of helper options. Then the caller shall parse the sementic of its own options, with the opterr variable set to zero (hence ignoring helper options). Signed-off-by: Christophe Milard <christophe.milard@linaro.org> --- helper/include/odp/helper/linux.h | 16 +++++++++++++++- helper/linux.c | 24 +++++++++++++++++++----- helper/test/odpthreads.c | 2 +- test/validation/common/odp_cunit_common.c | 2 +- 4 files changed, 36 insertions(+), 8 deletions(-)