From: EXT Christophe Milard [mailto:christophe.mil...@linaro.org] Sent: Thursday, May 12, 2016 6:07 PM To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com> Cc: lng-odp@lists.linaro.org Subject: Re: [lng-odp] [PATCHv6 12/38] helper: parsing the complete set of options
On 12 May 2016 at 12:54, Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com<mailto:petri.savolai...@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. The thing is that you are not forced to introduce the parse function in two parts (first with one prototype/spec and then with another), right? Because it’s a new function you can bring it to every example/test in the same, final form (regardless if app has its own options or not). I’m not interested to see (== review) the development process, but the final result of the development (== the final helper specification). 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. It should not be a large rebase effort to merge 1 and 12, and then squash/rebase those patches that first modify test for patch 1 and then the same examples for patch 12. For example, patch 2 would directly contain return odph_parse_options(argc, argv, NULL, NULL); instead of return odph_parse_options(argc, argv); -Petri Christophe. -Petri From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org<mailto:lng-odp-boun...@lists.linaro.org>] On Behalf Of Christophe Milard Sent: Thursday, May 12, 2016 12:05 PM To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolai...@nokia.com<mailto:petri.savolai...@nokia.com>> Cc: lng-odp@lists.linaro.org<mailto: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.savolai...@nokia.com<mailto:petri.savolai...@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-boun...@lists.linaro.org<mailto:lng-odp-boun...@lists.linaro.org>] > On Behalf Of > Christophe Milard > Sent: Wednesday, May 11, 2016 7:42 PM > To: brian.bro...@linaro.org<mailto:brian.bro...@linaro.org>; > mike.hol...@linaro.org<mailto:mike.hol...@linaro.org>; lng- > o...@lists.linaro.org<mailto:o...@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.mil...@linaro.org<mailto:christophe.mil...@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<mailto:lng-odp@lists.linaro.org> > https://lists.linaro.org/mailman/listinfo/lng-odp
_______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp
