Hi Richard, This patch looks great. My comments below are just my process for understanding what the code does. I really like this approach, it's a pretty small amount of code and lets us easily override any option from the command line, without having to continuously add more short options for esoteric configurations.
Regards, Jake > -----Original Message----- > From: Richard Cochran [mailto:richardcoch...@gmail.com] > Sent: Tuesday, December 06, 2016 10:50 AM > To: linuxptp-devel@lists.sourceforge.net > Subject: [Linuxptp-devel] [PATCH] ptp4l: Accept any configuration option as a > command line argument. > > This patch provides a way to use the entire table of configuration options > as "long" command line switches. > Neat, I didn't think it would be so little code! Thanks! > Signed-off-by: Richard Cochran <richardcoch...@gmail.com> > --- > config.c | 64 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > -- > config.h | 11 +++++++++++ > ptp4l.c | 11 +++++++++-- > 3 files changed, 82 insertions(+), 4 deletions(-) > > diff --git a/config.c b/config.c > index 5da5ecc..384b437 100644 > --- a/config.c > +++ b/config.c > @@ -329,6 +329,7 @@ static enum parser_result parse_section_line(char *s, > enum config_section *secti > } > > static enum parser_result parse_item(struct config *cfg, > + int commandline, Ok so we add a boolean to set whether we're calling parse_item from command line code. > const char *section, > const char *option, > const char *value) > @@ -387,7 +388,7 @@ static enum parser_result parse_item(struct config *cfg, > return NOT_PARSED; > } > } > - } else if (cgi->flags & CFG_ITEM_LOCKED) { > + } else if (!commandline && cgi->flags & CFG_ITEM_LOCKED) { And here, we do allow over-writing a previous assignment with a later assignment from the command line. Good. > /* This global option was set on the command line. */ > return PARSED_OK; > } else { > @@ -415,6 +416,10 @@ static enum parser_result parse_item(struct config *cfg, > dst->flags |= CFG_ITEM_DYNSTR; > break; > } > + > + if (commandline) { > + dst->flags &= CFG_ITEM_LOCKED; > + } Finally, if we run from the command line, we make sure we lock the item now so that it doesn't change again. This makes sense. > return PARSED_OK; > } > > @@ -490,6 +495,25 @@ static void check_deprecated_options(const char > **option) > } > } > > +static struct option *config_alloc_longopts(struct config *cfg) > +{ > + struct config_item *ci; > + struct option *opts; > + int i; > + > + opts = calloc(1, (1 + N_CONFIG_ITEMS) * sizeof(*opts)); > + if (!opts) { > + return NULL; > + } > + for (i = 0; i < N_CONFIG_ITEMS; i++) { > + ci = &config_tab[i]; > + opts[i].name = ci->label; > + opts[i].has_arg = required_argument; > + } > + > + return opts; > +} > + A new function here to generate long option structures from the configuration data. Ok. > int config_read(char *name, struct config *cfg) > { > enum config_section current_section = UNKNOWN_SECTION; > @@ -554,7 +578,7 @@ int config_read(char *name, struct config *cfg) > > check_deprecated_options(&option); > > - parser_res = parse_item(cfg, current_section == > GLOBAL_SECTION ? > + parser_res = parse_item(cfg, 0, current_section == > GLOBAL_SECTION ? > NULL : current_port->name, option, > value); > > switch (parser_res) { > @@ -627,8 +651,15 @@ struct config *config_create(void) > } > STAILQ_INIT(&cfg->interfaces); > > + cfg->opts = config_alloc_longopts(cfg); > + if (!cfg->opts) { > + free(cfg); > + return NULL; > + } > + > cfg->htab = hash_create(); > if (!cfg->htab) { > + free(cfg->opts); > free(cfg); > return NULL; > } > @@ -657,6 +688,7 @@ struct config *config_create(void) > return cfg; > fail: > hash_destroy(cfg->htab, NULL); > + free(cfg->opts); > free(cfg); > return NULL; > } > @@ -670,6 +702,7 @@ void config_destroy(struct config *cfg) > free(iface); > } > hash_destroy(cfg->htab, config_item_free); > + free(cfg->opts); > free(cfg); > } > > @@ -720,6 +753,33 @@ char *config_get_string(struct config *cfg, const char > *section, > return ci->val.s; > } > > +int config_parse_option(struct config *cfg, const char *opt, const char *val) > +{ > + enum parser_result result; > + > + result = parse_item(cfg, 1, NULL, opt, val); > + Right so we can re-use the parse item code but setting command line to true here. > + switch (result) { > + case PARSED_OK: > + return 0; > + case NOT_PARSED: > + fprintf(stderr, "unknown option %s\n", opt); > + break; > + case BAD_VALUE: > + fprintf(stderr, "%s is a bad value for option %s\n", val, opt); > + break; > + case MALFORMED: > + fprintf(stderr, "%s is a malformed value for option %s\n", > + val, opt); > + break; > + case OUT_OF_RANGE: > + fprintf(stderr, "%s is an out of range value for option %s\n", > + val, opt); > + break; > + } > + return -1; > +} > + > int config_set_double(struct config *cfg, const char *option, double val) > { > struct config_item *ci = config_find_item(cfg, NULL, option); > diff --git a/config.h b/config.h > index b02bde6..1cc7051 100644 > --- a/config.h > +++ b/config.h > @@ -20,6 +20,7 @@ > #ifndef HAVE_CONFIG_H > #define HAVE_CONFIG_H > > +#include <getopt.h> > #include <sys/queue.h> > > #include "ds.h" > @@ -43,6 +44,9 @@ struct config { > STAILQ_HEAD(interfaces_head, interface) interfaces; > int n_interfaces; > > + /* for parsing command line options */ > + struct option *opts; > + > /* hash of all non-legacy items */ > struct hash *htab; > }; > @@ -64,6 +68,13 @@ int config_get_int(struct config *cfg, const char *section, > char *config_get_string(struct config *cfg, const char *section, > const char *option); > > +static inline struct option *config_long_options(struct config *cfg) > +{ > + return cfg->opts; > +} > + > +int config_parse_option(struct config *cfg, const char *opt, const char > *val); > + > int config_set_double(struct config *cfg, const char *option, double val); > > int config_set_section_int(struct config *cfg, const char *section, > diff --git a/ptp4l.c b/ptp4l.c > index a87e7e6..e90fcb2 100644 > --- a/ptp4l.c > +++ b/ptp4l.c > @@ -73,8 +73,9 @@ static void usage(char *progname) > int main(int argc, char *argv[]) > { > char *config = NULL, *req_phc = NULL, *progname; > - int c, err = -1, print_level; > + int c, err = -1, index, print_level; > struct clock *clock = NULL; > + struct option *opts; > struct config *cfg; > > if (handle_term_signals()) > @@ -84,12 +85,18 @@ int main(int argc, char *argv[]) > if (!cfg) { > return -1; > } > + opts = config_long_options(cfg); > So in the config code we have method for generating the options structure, and we get that here. Nice. > /* Process the command line arguments. */ > progname = strrchr(argv[0], '/'); > progname = progname ? 1+progname : argv[0]; > - while (EOF != (c = getopt(argc, argv, "AEP246HSLf:i:p:sl:mqvh"))) { > + while (EOF != (c = getopt_long(argc, argv, "AEP246HSLf:i:p:sl:mqvh", > + opts, &index))) { Finally, we switch to using getopt_long, which will grab the index of the matching long option.. So we can then parse its values next. > switch (c) { > + case 0: > + if (config_parse_option(cfg, opts[index].name, optarg)) > + goto out; > + break; > case 'A': > if (config_set_int(cfg, "delay_mechanism", DM_AUTO)) > goto out; > -- > 2.1.4 > > > ------------------------------------------------------------------------------ > Developer Access Program for Intel Xeon Phi Processors > Access to Intel Xeon Phi processor-based developer platforms. > With one year of Intel Parallel Studio XE. > Training and support from Colfax. > Order your platform today.http://sdm.link/xeonphi > _______________________________________________ > Linuxptp-devel mailing list > Linuxptp-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linuxptp-devel ------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today.http://sdm.link/xeonphi _______________________________________________ Linuxptp-devel mailing list Linuxptp-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
