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

[Richard Cochran]

This patch provides a way to use the entire table of configuration options
as "long" command line switches.

Signed-off-by: Richard Cochran <***@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,
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) {
/* 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;
+ }
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;
+}
+
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);
+
+ 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);

/* 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))) {
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;

[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-----
Sent: Tuesday, December 06, 2016 10:50 AM
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!

---
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;
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) {
+ return 0;
+ fprintf(stderr, "unknown option %s\n", opt);
+ break;
+ fprintf(stderr, "%s is a bad value for option %s\n", val, opt);
+ break;
+ fprintf(stderr, "%s is a malformed value for option %s\n",
+ val, opt);
+ break;
+ 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) {
+ if (config_parse_option(cfg, opts[index].name, optarg))
+ goto out;
+ break;
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
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

[Richard Cochran]

Post by Keller, Jacob E
Neat, I didn't think it would be so little code! Thanks!

Yeah, sometimes you get lucky.

Thanks,
Richard

[Miroslav Lichvar]

Post by Richard Cochran
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"

Post by Richard Cochran
+
+ if (commandline) {
+ dst->flags &= CFG_ITEM_LOCKED;

Should this be dst->flags |= CFG_ITEM_LOCKED ?

[Richard Cochran]

Post by Miroslav Lichvar
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.

Post by Miroslav Lichvar

Post by Richard Cochran
+ if (commandline) {
+ dst->flags &= CFG_ITEM_LOCKED;

Should this be dst->flags |= CFG_ITEM_LOCKED ?

Ouch. Yes of course.

Thanks,
Richard

[Miroslav Lichvar]

Post by Richard Cochran

Post by Miroslav Lichvar

Post by Richard Cochran
+ 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,