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? Maybe 4 and 5, because the first introduces types that the other depends on. Maybe 5 and 12 (at least 12 should be before 5), if odph_parse_options() must be always called before odph_odpthreads_create(). -Petri From: lng-odp [mailto:[email protected]] On Behalf Of Christophe Milard Sent: Thursday, May 12, 2016 12:05 PM To: Savolainen, Petri (Nokia - FI/Espoo) <[email protected]> Cc: [email protected] 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) <[email protected]<mailto:[email protected]>> 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:[email protected]<mailto:[email protected]>] > On Behalf Of > Christophe Milard > Sent: Wednesday, May 11, 2016 7:42 PM > To: [email protected]<mailto:[email protected]>; > [email protected]<mailto:[email protected]>; lng- > [email protected]<mailto:[email protected]> > 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 > <[email protected]<mailto:[email protected]>> > --- > 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 > [email protected]<mailto:[email protected]> > https://lists.linaro.org/mailman/listinfo/lng-odp
_______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
