Re: [Linuxptp-devel] [PATCH] ptp4l: Accept any configuration option as a command line argument.

[Miroslav Lichvar]

On Tue, Dec 13, 2016 at 05:34:03PM +0100, Richard Cochran wrote:
> On Tue, Dec 13, 2016 at 04:47:23PM +0100, Miroslav Lichvar wrote:
> > > + if (commandline) {
> > > + dst->flags &= CFG_ITEM_LOCKED;
> > 
> > Should this be dst->flags |= CFG_ITEM_LOCKED ?
> 
> Ouch.  Yes of course.

Ok, with this change, valgrind reports no errors and the options seem
to be working as expected. This is a nice feature! Could you please
mention it in the man page?

Thanks,

-- 
Miroslav Lichvar

--
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

Re: [Linuxptp-devel] [PATCH] ptp4l: Accept any configuration option as a command line argument.

[Richard Cochran]

On Tue, Dec 13, 2016 at 04:47:23PM +0100, Miroslav Lichvar wrote:
> I'm testing the latest code from git which includes this feature. It
> doesn't seem to work correctly for me. The program accepts long
> options, but the behaviour doesn't change as if they were not
> specified. Also, valgrind reports an error about invalid free on exit.

I'll check it.

> > +   if (commandline) {
> > +   dst->flags &= CFG_ITEM_LOCKED;
> 
> Should this be dst->flags |= CFG_ITEM_LOCKED ?

Ouch.  Yes of course.

Thanks,
Richard

--
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

Re: [Linuxptp-devel] [PATCH] ptp4l: Accept any configuration option as a command line argument.

[Miroslav Lichvar]

On Tue, Dec 06, 2016 at 07:49:56PM +0100, Richard Cochran wrote:
> This patch provides a way to use the entire table of configuration options
> as "long" command line switches.

I'm testing the latest code from git which includes this feature. It
doesn't seem to work correctly for me. The program accepts long
options, but the behaviour doesn't change as if they were not
specified. Also, valgrind reports an error about invalid free on exit.

==4584== Invalid free() / delete / delete[] / realloc()
==4584==at 0x4C2ED4A: free (vg_replace_malloc.c:530)
==4584==by 0x4077DD: config_item_free (config.c:310)
==4584==by 0x4091F3: hash_destroy (hash.c:62)
==4584==by 0x4085F9: config_destroy (config.c:704)
==4584==by 0x40228A: main (ptp4l.c:221)
==4584==  Address 0x622d20 is 2240 bytes inside data symbol "config_tab"

> +
> + if (commandline) {
> + dst->flags &= CFG_ITEM_LOCKED;

Should this be dst->flags |= CFG_ITEM_LOCKED ?

-- 
Miroslav Lichvar

--
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

Re: [Linuxptp-devel] [PATCH] ptp4l: Accept any configuration option as a command line argument.

[Keller, Jacob E]

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 
> ---
>  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 = _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();
> 
> - 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(>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