On 12 May 2016 at 12:54, Savolainen, Petri (Nokia - FI/Espoo) < 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. > > > 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-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> > *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.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] On Behalf Of > > Christophe Milard > > Sent: Wednesday, May 11, 2016 7:42 PM > > To: brian.bro...@linaro.org; mike.hol...@linaro.org; lng- > > 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> > > --- > > 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 > > >
_______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp
