[Linuxptp-devel] [PATCH RFC v2 00/16] New configuration implementation

[Richard Cochran]

Changes in V2:
- API is much simpler to use
- numerous details in the implementation have been improved
- half dozen legacy options converted

The config code was simplistic at first and has grown many warts over
time. The pointers to global variables are rather gross, and it is
hard to add new options, especially port specific ones. The number of
configuration options is sure to grow larger over time.

This series addresses the issues by implementing a hash table based
implementation. Adding a new option is as simple as placing the item
into the config_tab[] table (one line), and then calling
var=config_get_TYPE() at the usage site.

This series add one new option and converts six legacy options. Many
if not all of the remaining legacy options can be converted to the new
system. Once this is done, the config code will be much more compact
and maintainable. In addition, the phc2sys and pmc programs will also
be able to use a configuration file.

Patches 1-4 and 12 provide changes allowing passing of the configuration.
Patch 5 and 6 add a simple hash table.
Patch 7 adds the bulk of the new implementation.
Patch 8 lets the UDP transport use the new TTL option.
Patches 9-11 and 14-16 convert some legacy options.
Patch 13 adds a way to "lock" options from the command line.

Review and comments are most welcome.

Thanks,
Richard


Richard Cochran (16):
clock: store the configuration in the clock data structure.
clock: add a method to obtain the configuration.
pmc: require a configuration for creating a PMC instance.
transport: store the configuration in the transport data structure.
Introduce a simple hash table implementation.
config: Add a hash table into the data structure.
config: introduce a new API for reading configuration settings.
udp: configure the socket with the TTL option.
config: convert the 'assume_two_step' option to the new scheme.
config: convert 'tx_timestamp_timeout' to the new scheme.
config: convert the 'check_fup_sync' option to the new scheme.
servo: store the configuration in the servo data structure.
config: add methods to set values taken from the command line.
config: convert the 'step_threshold' option to the new scheme.
config: convert the 'first_step_threshold' option to the new scheme.
config: convert the 'max_frequency' option to the new scheme.

clock.c | 18 ++-
clock.h | 15 ++-
config.c | 353 +++++++++++++++++++++++++++++++++++++++++++++-------
config.h | 28 +++--
hash.c | 113 +++++++++++++++++
hash.h | 59 +++++++++
makefile | 12 +-
phc2sys.c | 31 +++--
pmc.c | 7 +-
pmc_common.c | 9 +-
pmc_common.h | 8 +-
port.c | 2 +-
ptp4l.c | 21 ++--
servo.c | 16 ++-
servo.h | 27 +---
transport.c | 7 +-
transport.h | 6 +-
transport_private.h | 1 +
udp.c | 15 ++-
19 files changed, 619 insertions(+), 129 deletions(-)
create mode 100644 hash.c
create mode 100644 hash.h

[Richard Cochran]

This function allows the ports to read configuration variables without
changing the port method signatures.

Signed-off-by: Richard Cochran <***@gmail.com>
---
clock.c | 5 +++++
clock.h | 7 +++++++
2 files changed, 12 insertions(+)

diff --git a/clock.c b/clock.c
index 1569108..a47e1e9 100644
--- a/clock.c
+++ b/clock.c
@@ -755,6 +755,11 @@ UInteger8 clock_class(struct clock *c)
return c->dds.clockQuality.clockClass;
}

+struct config *clock_config(struct clock *c)
+{
+ return c->config;
+}
+
static int clock_add_port(struct clock *c, int phc_index,
enum timestamp_type timestamping,
struct interface *iface)
diff --git a/clock.h b/clock.h
index 1e6cd98..d7377fe 100644
--- a/clock.h
+++ b/clock.h
@@ -58,6 +58,13 @@ struct port *clock_best_port(struct clock *c);
UInteger8 clock_class(struct clock *c);

/**
+ * Obtains a reference to the configuration database.
+ * @param c The clock instance.
+ * @return A pointer to the configuration, without fail.
+ */
+struct config *clock_config(struct clock *c);
+
+/**
* Create a clock instance. There can only be one clock in any system,
* so subsequent calls will destroy the previous clock instance.
*

[Richard Cochran]

This will help us to simplify the passing of parameters between the main
program. clock, and ports.

Signed-off-by: Richard Cochran <***@gmail.com>
---
clock.c | 9 ++++++---
clock.h | 8 +++++---
ptp4l.c | 3 ++-
3 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/clock.c b/clock.c
index 9e9feba..1569108 100644
--- a/clock.c
+++ b/clock.c
@@ -73,6 +73,7 @@ struct clock_subscriber {
};

struct clock {
+ struct config *config;
clockid_t clkid;
struct servo *servo;
enum servo_type servo_type;
@@ -792,9 +793,10 @@ static void clock_remove_port(struct clock *c, struct port *p)
port_close(p);
}

-struct clock *clock_create(int phc_index, struct interfaces_head *ifaces,
- enum timestamp_type timestamping, struct default_ds *dds,
- enum servo_type servo)
+struct clock *clock_create(struct config *config, int phc_index,
+ struct interfaces_head *ifaces,
+ enum timestamp_type timestamping,
+ struct default_ds *dds, enum servo_type servo)
{
int fadj = 0, max_adj = 0, sw_ts = timestamping == TS_SOFTWARE ? 1 : 0;
struct clock *c = &the_clock;
@@ -814,6 +816,7 @@ struct clock *clock_create(int phc_index, struct interfaces_head *ifaces,
udsif->transport = TRANS_UDS;
udsif->delay_filter_length = 1;

+ c->config = config;
c->free_running = dds->free_running;
c->freq_est_interval = dds->freq_est_interval;
c->grand_master_capable = dds->grand_master_capable;
diff --git a/clock.h b/clock.h
index a8286dd..1e6cd98 100644
--- a/clock.h
+++ b/clock.h
@@ -61,6 +61,7 @@ UInteger8 clock_class(struct clock *c);
* Create a clock instance. There can only be one clock in any system,
* so subsequent calls will destroy the previous clock instance.
*
+ * @param config Pointer to the configuration database.
* @param phc_index PTP hardware clock device to use.
* Pass -1 to select CLOCK_REALTIME.
* @param ifaces A queue of network interfaces.
@@ -69,9 +70,10 @@ UInteger8 clock_class(struct clock *c);
* @param servo The servo that this clock will use.
* @return A pointer to the single global clock instance.
*/
-struct clock *clock_create(int phc_index, struct interfaces_head *ifaces,
- enum timestamp_type timestamping, struct default_ds *dds,
- enum servo_type servo);
+struct clock *clock_create(struct config *config, int phc_index,
+ struct interfaces_head *ifaces,
+ enum timestamp_type timestamping,
+ struct default_ds *dds, enum servo_type servo);

/**
* Obtains a clock's default data set.
diff --git a/ptp4l.c b/ptp4l.c
index 61c5854..56cb8bd 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -377,7 +377,8 @@ int main(int argc, char *argv[])
return -1;
}

- clock = clock_create(phc_index, &cfg_settings.interfaces,
+ clock = clock_create(&cfg_settings,
+ phc_index, &cfg_settings.interfaces,
*timestamping, &cfg_settings.dds,
cfg_settings.clock_servo);
if (!clock) {

[Richard Cochran]

In the near future, the transports will need to consult the configuration
database in order to obtain various options. This patch also introduces
the idea of a configuration file into the phc2sys and pmc programs.

Signed-off-by: Richard Cochran <***@gmail.com>
---
phc2sys.c | 10 ++++++----
pmc.c | 3 ++-
pmc_common.c | 7 ++++---
pmc_common.h | 8 +++++---
4 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/phc2sys.c b/phc2sys.c
index 9ff5bf9..5ba99c3 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -119,6 +119,8 @@ struct node {
struct clock *master;
};

+static struct config phc2sys_config;
+
static int update_pmc(struct node *node, int subscribe);
static int clock_handle_leap(struct node *node, struct clock *clock,
int64_t offset, uint64_t ts);
@@ -777,13 +779,13 @@ static void send_subscription(struct node *node)
pmc_send_set_action(node->pmc, TLV_SUBSCRIBE_EVENTS_NP, &sen, sizeof(sen));
}

-static int init_pmc(struct node *node, int domain_number)
+static int init_pmc(struct config *cfg, struct node *node, int domain_number)
{
char uds_local[MAX_IFNAME_SIZE + 1];

snprintf(uds_local, sizeof(uds_local), "/var/run/phc2sys.%d",
getpid());
- node->pmc = pmc_create(TRANS_UDS, uds_local, 0, domain_number, 0, 1);
+ node->pmc = pmc_create(cfg, TRANS_UDS, uds_local, 0, domain_number, 0, 1);
if (!node->pmc) {
pr_err("failed to create pmc");
return -1;
@@ -1395,7 +1397,7 @@ int main(int argc, char *argv[])
print_set_level(print_level);

if (autocfg) {
- if (init_pmc(&node, domain_number))
+ if (init_pmc(&phc2sys_config, &node, domain_number))
return -1;
if (auto_init_ports(&node, rt) < 0)
return -1;
@@ -1431,7 +1433,7 @@ int main(int argc, char *argv[])
r = -1;

if (wait_sync) {
- if (init_pmc(&node, domain_number))
+ if (init_pmc(&phc2sys_config, &node, domain_number))
goto end;

while (is_running()) {
diff --git a/pmc.c b/pmc.c
index 0774f88..1fe8183 100644
--- a/pmc.c
+++ b/pmc.c
@@ -42,6 +42,7 @@
#define P41 ((double)(1ULL << 41))

static struct pmc *pmc;
+static struct config pmc_config;

static void do_get_action(int action, int index, char *str);
static void do_set_action(int action, int index, char *str);
@@ -816,7 +817,7 @@ int main(int argc, char *argv[])
print_set_syslog(1);
print_set_verbose(1);

- pmc = pmc_create(transport_type, iface_name, boundary_hops,
+ pmc = pmc_create(&pmc_config, transport_type, iface_name, boundary_hops,
domain_number, transport_specific, zero_datalen);
if (!pmc) {
fprintf(stderr, "failed to create pmc\n");
diff --git a/pmc_common.c b/pmc_common.c
index afd6e5f..0a365f5 100644
--- a/pmc_common.c
+++ b/pmc_common.c
@@ -61,9 +61,10 @@ struct pmc {
int zero_length_gets;
};

-struct pmc *pmc_create(enum transport_type transport_type, const char *iface_name,
- UInteger8 boundary_hops, UInteger8 domain_number,
- UInteger8 transport_specific, int zero_datalen)
+struct pmc *pmc_create(struct config *cfg, enum transport_type transport_type,
+ const char *iface_name, UInteger8 boundary_hops,
+ UInteger8 domain_number, UInteger8 transport_specific,
+ int zero_datalen)
{
struct pmc *pmc;

diff --git a/pmc_common.h b/pmc_common.h
index 9adb9d1..bb3a1f1 100644
--- a/pmc_common.h
+++ b/pmc_common.h
@@ -21,14 +21,16 @@
#ifndef HAVE_PMC_COMMON_H
#define HAVE_PMC_COMMON_H

+#include "config.h"
#include "msg.h"
#include "transport.h"

struct pmc;

-struct pmc *pmc_create(enum transport_type transport_type, const char *iface_name,
- UInteger8 boundary_hops, UInteger8 domain_number,
- UInteger8 transport_specific, int zero_datalen);
+struct pmc *pmc_create(struct config *cfg, enum transport_type transport_type,
+ const char *iface_name, UInteger8 boundary_hops,
+ UInteger8 domain_number, UInteger8 transport_specific,
+ int zero_datalen);

void pmc_destroy(struct pmc *pmc);

[Richard Cochran]

This will allow modules to read out various user options.

Signed-off-by: Richard Cochran <***@gmail.com>
---
pmc_common.c | 2 +-
port.c | 2 +-
transport.c | 7 +++++--
transport.h | 6 +++++-
transport_private.h | 1 +
5 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/pmc_common.c b/pmc_common.c
index 0a365f5..d92b0cd 100644
--- a/pmc_common.c
+++ b/pmc_common.c
@@ -85,7 +85,7 @@ struct pmc *pmc_create(struct config *cfg, enum transport_type transport_type,
pmc->domain_number = domain_number;
pmc->transport_specific = transport_specific;

- pmc->transport = transport_create(transport_type);
+ pmc->transport = transport_create(cfg, transport_type);
if (!pmc->transport) {
pr_err("failed to create transport");
goto failed;
diff --git a/port.c b/port.c
index 3ed5e7b..e5d62b9 100644
--- a/port.c
+++ b/port.c
@@ -2524,7 +2524,7 @@ struct port *port_open(int phc_index,
p->pod = interface->pod;
p->name = interface->name;
p->clock = clock;
- p->trp = transport_create(interface->transport);
+ p->trp = transport_create(clock_config(clock), interface->transport);
if (!p->trp)
goto err_port;
p->timestamping = timestamping;
diff --git a/transport.c b/transport.c
index fc18740..d24c05b 100644
--- a/transport.c
+++ b/transport.c
@@ -87,7 +87,8 @@ enum transport_type transport_type(struct transport *t)
return t->type;
}

-struct transport *transport_create(enum transport_type type)
+struct transport *transport_create(struct config *cfg,
+ enum transport_type type)
{
struct transport *t = NULL;
switch (type) {
@@ -108,8 +109,10 @@ struct transport *transport_create(enum transport_type type)
case TRANS_PROFINET:
break;
}
- if (t)
+ if (t) {
t->type = type;
+ t->cfg = cfg;
+ }
return t;
}

diff --git a/transport.h b/transport.h
index 8e0d421..5d6ba98 100644
--- a/transport.h
+++ b/transport.h
@@ -26,6 +26,8 @@
#include "fd.h"
#include "msg.h"

+struct config;
+
/* Values from networkProtocol enumeration 7.4.1 Table 3 */
enum transport_type {
/* 0 is Reserved in spec. Use it for UDS */
@@ -123,10 +125,12 @@ int transport_protocol_addr(struct transport *t, uint8_t *addr);

/**
* Allocate an instance of the specified transport.
+ * @param config Pointer to the configuration database.
* @param type Which transport to obtain.
* @return Pointer to a transport instance on success, NULL otherwise.
*/
-struct transport *transport_create(enum transport_type type);
+struct transport *transport_create(struct config *cfg,
+ enum transport_type type);

/**
* Free an instance of a transport.
diff --git a/transport_private.h b/transport_private.h
index 3e0ba9a..b54f32a 100644
--- a/transport_private.h
+++ b/transport_private.h
@@ -28,6 +28,7 @@

struct transport {
enum transport_type type;
+ struct config *cfg;

int (*close)(struct transport *t, struct fdarray *fda);

[Richard Cochran]

Signed-off-by: Richard Cochran <***@gmail.com>
---
config.c | 11 +++++++++++
config.h | 4 ++++
2 files changed, 15 insertions(+)

diff --git a/config.c b/config.c
index 934caa2..9ef8be1 100644
--- a/config.c
+++ b/config.c
@@ -24,6 +24,7 @@
#include <string.h>
#include "config.h"
#include "ether.h"
+#include "hash.h"
#include "print.h"
#include "util.h"

@@ -803,6 +804,15 @@ void config_init_interface(struct interface *iface, struct config *cfg)
iface->boundary_clock_jbod = cfg->dds.boundary_clock_jbod;
}

+int config_init(struct config *cfg)
+{
+ cfg->htab = hash_create();
+ if (!cfg->htab) {
+ return -1;
+ }
+ return 0;
+}
+
void config_destroy(struct config *cfg)
{
struct interface *iface;
@@ -811,4 +821,5 @@ void config_destroy(struct config *cfg)
STAILQ_REMOVE_HEAD(&cfg->interfaces, list);
free(iface);
}
+ hash_destroy(cfg->htab, free);
}
diff --git a/config.h b/config.h
index 7bff11f..222883a 100644
--- a/config.h
+++ b/config.h
@@ -60,6 +60,10 @@ struct config {
/* configured interfaces */
STAILQ_HEAD(interfaces_head, interface) interfaces;

+ /* hash of all non-legacy items */
+ struct hash *htab;
+
+ /* the rest are legacy fields */
enum timestamp_type timestamping;
enum transport_type transport;
enum delay_mechanism dm;

[Richard Cochran]

Signed-off-by: Richard Cochran <***@gmail.com>
---
hash.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
hash.h | 59 +++++++++++++++++++++++++++++++++
makefile | 2 +-
3 files changed, 173 insertions(+), 1 deletion(-)
create mode 100644 hash.c
create mode 100644 hash.h

diff --git a/hash.c b/hash.c
new file mode 100644
index 0000000..e768692
--- /dev/null
+++ b/hash.c
@@ -0,0 +1,113 @@
+/**
+ * @file hash.c
+ * @brief Implements a simple hash table.
+ * @note Copyright (C) 2015 Richard Cochran <***@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+#include <stdlib.h>
+#include <string.h>
+
+#include "hash.h"
+
+#define HASH_TABLE_SIZE 200
+
+struct node {
+ char *key;
+ void *data;
+ struct node *next;
+};
+
+struct hash {
+ struct node *table[HASH_TABLE_SIZE];
+};
+
+static unsigned int hash_function(const char* s)
+{
+ unsigned int i;
+
+ for (i = 0; *s; s++) {
+ i = 131 * i + *s;
+ }
+ return i % HASH_TABLE_SIZE;
+}
+
+struct hash *hash_create(void)
+{
+ struct hash *ht = calloc(1, sizeof(*ht));
+ return ht;
+}
+
+void hash_destroy(struct hash *ht, void (*func)(void *))
+{
+ unsigned int i;
+ struct node *n, *next, **table = ht->table;
+
+ for (i = 0; i < HASH_TABLE_SIZE; i++) {
+ for (n = table[i] ; n; n = next) {
+ next = n->next;
+ if (func) {
+ func(n->data);
+ }
+ free(n->key);
+ free(n);
+ }
+ }
+
+ free(ht);
+}
+
+int hash_insert(struct hash *ht, const char* key, void *data)
+{
+ unsigned int h;
+ struct node *n, **table = ht->table;
+
+ h = hash_function(key);
+
+ for (n = table[h] ; n; n = n->next) {
+ if (!strcmp(n->key, key)) {
+ /* reject duplicate keys */
+ return -1;
+ }
+ }
+ n = calloc(1, sizeof(*n));
+ if (!n) {
+ return -1;
+ }
+ n->key = strdup(key);
+ if (!n->key) {
+ free(n);
+ return -1;
+ }
+ n->data = data;
+ n->next = table[h];
+ table[h] = n;
+ return 0;
+}
+
+void *hash_lookup(struct hash *ht, const char* key)
+{
+ unsigned int h;
+ struct node *n, **table = ht->table;
+
+ h = hash_function(key);
+
+ for (n = table[h] ; n; n = n->next) {
+ if (!strcmp(n->key, key)) {
+ return n->data;
+ }
+ }
+ return NULL;
+}
diff --git a/hash.h b/hash.h
new file mode 100644
index 0000000..74aae0b
--- /dev/null
+++ b/hash.h
@@ -0,0 +1,59 @@
+/**
+ * @file hash.h
+ * @brief Implements a simple hash table.
+ * @note Copyright (C) 2015 Richard Cochran <***@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+#ifndef HAVE_HASH_H
+#define HAVE_HASH_H
+
+struct hash;
+
+/**
+ * Create a new hash table.
+ * @return A pointer to a new hash table on success, NULL otherwise.
+ */
+struct hash *hash_create(void);
+
+/**
+ * Destroy an instance of a hash table.
+ * @param ht Pointer to a hash table obtained via @ref hash_create().
+ * @param func Callback function, possibly NULL, to apply to the
+ * data of each element in the table.
+ */
+void hash_destroy(struct hash *ht, void (*func)(void *));
+
+/**
+ * Inserts an element into a hash table.
+ * @param ht Hash table into which the element is to be stored.
+ * @param key Key that identifies the element.
+ * @param data Pointer to the user data to be stored.
+ * @return Zero on success and non-zero on error. Attempting to
+ * insert a duplicate key will fail with an error.
+ */
+int hash_insert(struct hash *ht, const char* key, void *data);
+
+/**
+ * Looks up an element from the hash table.
+ * @param ht Hash table to consult.
+ * @param key Key identifying the element of interest.
+ * @return Pointer to the element's data, or NULL if the key is not found.
+ */
+void *hash_lookup(struct hash *ht, const char* key);
+
+#endif
+
+
diff --git a/makefile b/makefile
index a8e5ce8..922769b 100644
--- a/makefile
+++ b/makefile
@@ -24,7 +24,7 @@ CFLAGS = -Wall $(VER) $(incdefs) $(DEBUG) $(EXTRA_CFLAGS)
LDLIBS = -lm -lrt $(EXTRA_LDFLAGS)
PRG = ptp4l pmc phc2sys hwstamp_ctl phc_ctl timemaster
OBJ = bmc.o clock.o clockadj.o clockcheck.o config.o fault.o \
- filter.o fsm.o linreg.o mave.o mmedian.o msg.o ntpshm.o nullf.o phc.o \
+ filter.o fsm.o hash.o linreg.o mave.o mmedian.o msg.o ntpshm.o nullf.o phc.o \
pi.o port.o print.o ptp4l.o raw.o servo.o sk.o stats.o tlv.o \
transport.o tsproc.o udp.o udp6.o uds.o util.o version.o

[Richard Cochran]

Signed-off-by: Richard Cochran <***@gmail.com>
---
udp.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/udp.c b/udp.c
index b100dbf..b8aa76a 100644
--- a/udp.c
+++ b/udp.c
@@ -30,6 +30,7 @@
#include <unistd.h>

#include "address.h"
+#include "config.h"
#include "contain.h"
#include "print.h"
#include "sk.h"
@@ -92,7 +93,8 @@ static int udp_close(struct transport *t, struct fdarray *fda)
return 0;
}

-static int open_socket(const char *name, struct in_addr mc_addr[2], short port)
+static int open_socket(const char *name, struct in_addr mc_addr[2], short port,
+ int ttl)
{
struct sockaddr_in addr;
int fd, index, on = 1;
@@ -123,6 +125,10 @@ static int open_socket(const char *name, struct in_addr mc_addr[2], short port)
pr_err("setsockopt SO_BINDTODEVICE failed: %m");
goto no_option;
}
+ if (setsockopt(fd, IPPROTO_IP, IP_MULTICAST_TTL, &ttl, sizeof(ttl))) {
+ pr_err("setsockopt IP_MULTICAST_TTL failed: %m");
+ goto no_option;
+ }
addr.sin_addr = mc_addr[0];
if (mcast_join(fd, index, (struct sockaddr *) &addr, sizeof(addr))) {
pr_err("mcast_join failed");
@@ -151,8 +157,9 @@ static int udp_open(struct transport *t, const char *name, struct fdarray *fda,
enum timestamp_type ts_type)
{
struct udp *udp = container_of(t, struct udp, t);
- int efd, gfd;
+ int efd, gfd, ttl;

+ ttl = config_get_int(t->cfg, name, "udp_ttl");
udp->mac.len = 0;
sk_interface_macaddr(name, &udp->mac);

@@ -165,11 +172,11 @@ static int udp_open(struct transport *t, const char *name, struct fdarray *fda,
if (!inet_aton(PTP_PDELAY_MCAST_IPADDR, &mcast_addr[MC_PDELAY]))
return -1;

- efd = open_socket(name, mcast_addr, EVENT_PORT);
+ efd = open_socket(name, mcast_addr, EVENT_PORT, ttl);
if (efd < 0)
goto no_event;

- gfd = open_socket(name, mcast_addr, GENERAL_PORT);
+ gfd = open_socket(name, mcast_addr, GENERAL_PORT, ttl);
if (gfd < 0)
goto no_general;

[Richard Cochran]

Signed-off-by: Richard Cochran <***@gmail.com>
---
config.c | 7 +------
config.h | 1 -
ptp4l.c | 3 ++-
3 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/config.c b/config.c
index 7cb503d..ce3378a 100644
--- a/config.c
+++ b/config.c
@@ -80,6 +80,7 @@ struct config_item {

struct config_item config_tab[] = {
GLOB_ITEM_INT("assume_two_step", 0, 0, 1),
+ GLOB_ITEM_INT("tx_timestamp_timeout", 1, 1, INT_MAX),
PORT_ITEM_INT("udp_ttl", 1, 1, 255),
};

@@ -528,12 +529,6 @@ static enum parser_result parse_global_setting(const char *option,
cfg->dds.freq_est_interval = val;
pod->freq_est_interval = val;

- } else if (!strcmp(option, "tx_timestamp_timeout")) {
- r = get_ranged_int(value, &val, 1, INT_MAX);
- if (r != PARSED_OK)
- return r;
- *cfg->tx_timestamp_timeout = val;
-
} else if (!strcmp(option, "check_fup_sync")) {
r = get_ranged_int(value, &val, 0, 1);
if (r != PARSED_OK)
diff --git a/config.h b/config.h
index a96a88f..3cd279c 100644
--- a/config.h
+++ b/config.h
@@ -70,7 +70,6 @@ struct config {

struct default_ds dds;
struct port_defaults pod;
- int *tx_timestamp_timeout;
int *check_fup_sync;

enum servo_type clock_servo;
diff --git a/ptp4l.c b/ptp4l.c
index 042e5aa..f8972a6 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -102,7 +102,6 @@ static struct config cfg_settings = {
.dm = DM_E2E,
.transport = TRANS_UDP_IPV4,

- .tx_timestamp_timeout = &sk_tx_timeout,
.check_fup_sync = &sk_check_fupsync,

.clock_servo = CLOCK_SERVO_PI,
@@ -280,6 +279,8 @@ int main(int argc, char *argv[])
}

assume_two_step = config_get_int(&cfg_settings, NULL, "assume_two_step");
+ sk_tx_timeout = config_get_int(&cfg_settings, NULL, "tx_timestamp_timeout");
+
if (!cfg_settings.dds.grand_master_capable &&
ds->flags & DDS_SLAVE_ONLY) {
fprintf(stderr,

[Richard Cochran]

This patch introduces generic code for adding and parsing new options.
The public 'get' methods return option values directly. Although the
API is easy to use, it does not provide error checking in case the
option does not exist or if there is a type mismatch.

Therefore the code performs a BIST to ensure that the options are
properly populated. In addition, the code terminates the program in
case of missing options or type mismatches. This heavy handed
approach is meant to catch errors during development and should never
trigger during normal usage.

As a first element, we include an option for specifying the UDP TTL.

Users are required to call 'config_init', and so this patch add that into
all three programs, ptp4l, phc2sys and pmc.

Signed-off-by: Richard Cochran <***@gmail.com>
---
config.c | 260 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
config.h | 13 ++++
makefile | 10 +--
phc2sys.c | 4 +
pmc.c | 4 +
ptp4l.c | 4 +
6 files changed, 283 insertions(+), 12 deletions(-)

diff --git a/config.c b/config.c
index 9ef8be1..c092d82 100644
--- a/config.c
+++ b/config.c
@@ -34,6 +34,118 @@ enum config_section {
UNKNOWN_SECTION,
};

+enum config_type {
+ CFG_TYPE_INT,
+ CFG_TYPE_UINT,
+ CFG_TYPE_DOUBLE,
+};
+
+typedef union {
+ int i;
+ unsigned int u;
+ double d;
+} any_t;
+
+#define CONFIG_LABEL_SIZE 32
+
+#define CFG_ITEM_STATIC (1 << 0) /* statically allocated, not to be freed */
+#define CFG_ITEM_LOCKED (1 << 1) /* command line value, may not be changed */
+#define CFG_ITEM_PORT (1 << 2) /* item may appear in port sections */
+
+struct config_item {
+ char label[CONFIG_LABEL_SIZE];
+ enum config_type type;
+ unsigned int flags;
+ any_t val;
+ any_t min;
+ any_t max;
+};
+
+#define N_CONFIG_ITEMS (sizeof(config_tab) / sizeof(config_tab[0]))
+
+#define CONFIG_ITEM_INT(_label, _port, _default, _min, _max) { \
+ .label = _label, \
+ .type = CFG_TYPE_INT, \
+ .flags = _port ? CFG_ITEM_PORT : 0, \
+ .val.i = _default, \
+ .min.i = _min, \
+ .max.i = _max, \
+}
+
+#define GLOB_ITEM_INT(label, _default, min, max) \
+ CONFIG_ITEM_INT(label, 0, _default, min, max)
+
+#define PORT_ITEM_INT(label, _default, min, max) \
+ CONFIG_ITEM_INT(label, 1, _default, min, max)
+
+struct config_item config_tab[] = {
+ PORT_ITEM_INT("udp_ttl", 1, 1, 255),
+};
+
+static struct config_item *config_section_item(struct config *cfg,
+ const char *section,
+ const char *name)
+{
+ char buf[CONFIG_LABEL_SIZE + MAX_IFNAME_SIZE];
+
+ snprintf(buf, sizeof(buf), "%s.%s", section, name);
+ return hash_lookup(cfg->htab, buf);
+}
+
+static struct config_item *config_global_item(struct config *cfg,
+ const char *name)
+{
+ return config_section_item(cfg, "global", name);
+}
+
+static struct config_item *config_find_item(struct config *cfg,
+ const char *port,
+ const char *name)
+{
+ struct config_item *ci;
+ if (port) {
+ ci = config_section_item(cfg, port, name);
+ if (ci) {
+ return ci;
+ }
+ }
+ return config_global_item(cfg, name);
+}
+
+static struct config_item *config_item_alloc(struct config *cfg,
+ const char *section,
+ const char *name,
+ enum config_type type)
+{
+ struct config_item *ci;
+ char buf[CONFIG_LABEL_SIZE + MAX_IFNAME_SIZE];
+
+ ci = calloc(1, sizeof(*ci));
+ if (!ci) {
+ fprintf(stderr, "low memory\n");
+ return NULL;
+ }
+ strncpy(ci->label, name, CONFIG_LABEL_SIZE - 1);
+ ci->type = type;
+
+ snprintf(buf, sizeof(buf), "%s.%s", section, ci->label);
+ if (hash_insert(cfg->htab, buf, ci)) {
+ fprintf(stderr, "low memory or duplicate item %s\n", name);
+ free(ci);
+ return NULL;
+ }
+
+ return ci;
+}
+
+static void config_item_free(void *ptr)
+{
+ struct config_item *ci = ptr;
+ if (ci->flags & CFG_ITEM_STATIC)
+ return;
+ free(ci);
+}
+
static enum parser_result parse_section_line(char *s, enum config_section *section)
{
if (!strcasecmp(s, "[global]")) {
@@ -52,6 +164,72 @@ static enum parser_result parse_section_line(char *s, enum config_section *secti
return PARSED_OK;
}

+static enum parser_result parse_item(struct config *cfg,
+ const char *port,
+ const char *option,
+ const char *value)
+{
+ struct config_item *cgi, *dst;
+ enum parser_result r;
+ unsigned int uval;
+ double df;
+ int val;
+
+ /* If there is no default value, then the option is bogus. */
+ cgi = config_global_item(cfg, option);
+ if (!cgi) {
+ return NOT_PARSED;
+ }
+
+ switch (cgi->type) {
+ case CFG_TYPE_INT:
+ r = get_ranged_int(value, &val, cgi->min.i, cgi->max.i);
+ break;
+ case CFG_TYPE_UINT:
+ r = get_ranged_uint(value, &uval, cgi->min.u, cgi->max.u);
+ break;
+ case CFG_TYPE_DOUBLE:
+ r = get_ranged_double(value, &df, cgi->min.d, cgi->max.d);
+ break;
+ }
+ if (r != PARSED_OK) {
+ return r;
+ }
+
+ if (port) {
+ if (!(cgi->flags & CFG_ITEM_PORT)) {
+ return NOT_PARSED;
+ }
+ /* Create or update this port specific item. */
+ dst = config_section_item(cfg, port, option);
+ if (!dst) {
+ dst = config_item_alloc(cfg, port, option, cgi->type);
+ if (!dst) {
+ return NOT_PARSED;
+ }
+ }
+ } else if (cgi->flags & CFG_ITEM_LOCKED) {
+ /* This global option was set on the command line. */
+ return PARSED_OK;
+ } else {
+ /* Update the global default value. */
+ dst = cgi;
+ }
+
+ switch (dst->type) {
+ case CFG_TYPE_INT:
+ dst->val.i = val;
+ break;
+ case CFG_TYPE_UINT:
+ dst->val.u = uval;
+ break;
+ case CFG_TYPE_DOUBLE:
+ dst->val.d = df;
+ break;
+ }
+ return PARSED_OK;
+}
+
static enum parser_result parse_pod_setting(const char *option,
const char *value,
struct port_defaults *pod)
@@ -173,9 +351,10 @@ static enum parser_result parse_pod_setting(const char *option,
return PARSED_OK;
}

-static enum parser_result parse_port_setting(const char *option,
- const char *value,
- struct interface *iface)
+static enum parser_result parse_port_setting(struct config *cfg,
+ const char *option,
+ const char *value,
+ struct interface *iface)
{
enum parser_result r;
int val;
@@ -237,7 +416,7 @@ static enum parser_result parse_port_setting(const char *option,
iface->boundary_clock_jbod = val;

} else
- return NOT_PARSED;
+ return parse_item(cfg, iface->name, option, value);

return PARSED_OK;
}
@@ -614,7 +793,7 @@ static enum parser_result parse_global_setting(const char *option,
cfg->dds.boundary_clock_jbod = val;

} else
- return NOT_PARSED;
+ return parse_item(cfg, NULL, option, value);

return PARSED_OK;
}
@@ -727,7 +906,7 @@ int config_read(char *name, struct config *cfg)
if (current_section == GLOBAL_SECTION)
parser_res = parse_global_setting(option, value, cfg);
else
- parser_res = parse_port_setting(option, value, current_port);
+ parser_res = parse_port_setting(cfg, option, value, current_port);

switch (parser_res) {
case PARSED_OK:
@@ -806,11 +985,40 @@ void config_init_interface(struct interface *iface, struct config *cfg)

int config_init(struct config *cfg)
{
+ char buf[CONFIG_LABEL_SIZE + 8];
+ struct config_item *ci;
+ int i;
+
cfg->htab = hash_create();
if (!cfg->htab) {
return -1;
}
+
+ /* Populate the hash table with global defaults. */
+ for (i = 0; i < N_CONFIG_ITEMS; i++) {
+ ci = &config_tab[i];
+ ci->flags |= CFG_ITEM_STATIC;
+ snprintf(buf, sizeof(buf), "global.%s", ci->label);
+ if (hash_insert(cfg->htab, buf, ci)) {
+ fprintf(stderr, "duplicate item %s\n", ci->label);
+ goto fail;
+ }
+ }
+
+ /* Perform a Built In Self Test.*/
+ for (i = 0; i < N_CONFIG_ITEMS; i++) {
+ ci = &config_tab[i];
+ ci = config_global_item(cfg, ci->label);
+ if (ci != &config_tab[i]) {
+ fprintf(stderr, "config BIST failed at %s\n",
+ config_tab[i].label);
+ goto fail;
+ }
+ }
return 0;
+fail:
+ hash_destroy(cfg->htab, NULL);
+ return -1;
}

void config_destroy(struct config *cfg)
@@ -821,5 +1029,43 @@ void config_destroy(struct config *cfg)
STAILQ_REMOVE_HEAD(&cfg->interfaces, list);
free(iface);
}
- hash_destroy(cfg->htab, free);
+ hash_destroy(cfg->htab, config_item_free);
+}
+
+double config_get_double(struct config *cfg, const char *port,
+ const char *option)
+{
+ struct config_item *ci = config_find_item(cfg, port, option);
+
+ if (!ci || ci->type != CFG_TYPE_DOUBLE) {
+ pr_err("bug: config option %s missing or invalid!", option);
+ exit(-1);
+ }
+ pr_debug("config item %s.%s is %f", port, option, ci->val.d);
+ return ci->val.d;
+}
+
+int config_get_int(struct config *cfg, const char *port, const char *option)
+{
+ struct config_item *ci = config_find_item(cfg, port, option);
+
+ if (!ci || ci->type != CFG_TYPE_INT) {
+ pr_err("bug: config option %s missing or invalid!", option);
+ exit(-1);
+ }
+ pr_debug("config item %s.%s is %d", port, option, ci->val.i);
+ return ci->val.i;
+}
+
+unsigned int config_get_uint(struct config *cfg, const char *port,
+ const char *option)
+{
+ struct config_item *ci = config_find_item(cfg, port, option);
+
+ if (!ci || ci->type != CFG_TYPE_UINT) {
+ pr_err("bug: config option %s missing or invalid!", option);
+ exit(-1);
+ }
+ pr_debug("config item %s.%s is %u", port, option, ci->val.u);
+ return ci->val.u;
}
diff --git a/config.h b/config.h
index 222883a..b2b8865 100644
--- a/config.h
+++ b/config.h
@@ -105,4 +105,17 @@ struct interface *config_create_interface(char *name, struct config *cfg);
void config_init_interface(struct interface *iface, struct config *cfg);
void config_destroy(struct config *cfg);

+/* New, hash table based methods: */
+
+int config_init(struct config *cfg);
+
+double config_get_double(struct config *cfg, const char *port,
+ const char *option);
+
+int config_get_int(struct config *cfg, const char *port,
+ const char *option);
+
+unsigned int config_get_uint(struct config *cfg, const char *port,
+ const char *option);
+
#endif
diff --git a/makefile b/makefile
index 922769b..bcdbb92 100644
--- a/makefile
+++ b/makefile
@@ -46,13 +46,13 @@ all: $(PRG)

ptp4l: $(OBJ)

-pmc: msg.o pmc.o pmc_common.o print.o raw.o sk.o tlv.o transport.o udp.o \
- udp6.o uds.o util.o version.o
-
-phc2sys: clockadj.o clockcheck.o linreg.o msg.o ntpshm.o nullf.o phc.o \
- phc2sys.o pi.o pmc_common.o print.o raw.o servo.o sk.o stats.o sysoff.o tlv.o \
+pmc: config.o hash.o msg.o pmc.o pmc_common.o print.o raw.o sk.o tlv.o \
transport.o udp.o udp6.o uds.o util.o version.o

+phc2sys: clockadj.o clockcheck.o config.o hash.o linreg.o msg.o ntpshm.o \
+ nullf.o phc.o phc2sys.o pi.o pmc_common.o print.o raw.o servo.o sk.o stats.o \
+ sysoff.o tlv.o transport.o udp.o udp6.o uds.o util.o version.o
+
hwstamp_ctl: hwstamp_ctl.o version.o

phc_ctl: phc_ctl.o phc.o sk.o util.o clockadj.o sysoff.o print.o version.o
diff --git a/phc2sys.c b/phc2sys.c
index 5ba99c3..92af7ad 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -1236,6 +1236,10 @@ int main(int argc, char *argv[])

handle_term_signals();

+ if (config_init(&phc2sys_config)) {
+ return -1;
+ }
+
configured_pi_kp = KP;
configured_pi_ki = KI;

diff --git a/pmc.c b/pmc.c
index 1fe8183..65ea0cf 100644
--- a/pmc.c
+++ b/pmc.c
@@ -744,6 +744,10 @@ int main(int argc, char *argv[])

handle_term_signals();

+ if (config_init(&pmc_config)) {
+ return -1;
+ }
+
/* Process the command line arguments. */
progname = strrchr(argv[0], '/');
progname = progname ? 1+progname : argv[0];
diff --git a/ptp4l.c b/ptp4l.c
index 56cb8bd..e2482c8 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -182,6 +182,10 @@ int main(int argc, char *argv[])
if (handle_term_signals())
return -1;

+ if (config_init(&cfg_settings)) {
+ return -1;
+ }
+
/* Set fault timeouts to a default value */
for (i = 0; i < FT_CNT; i++) {
cfg_settings.pod.flt_interval_pertype[i].type = FTMO_LOG2_SECONDS;

[Richard Cochran]

Signed-off-by: Richard Cochran <***@gmail.com>
---
config.c | 7 +------
config.h | 1 -
ptp4l.c | 3 ++-
3 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/config.c b/config.c
index c092d82..7cb503d 100644
--- a/config.c
+++ b/config.c
@@ -79,6 +79,7 @@ struct config_item {
CONFIG_ITEM_INT(label, 1, _default, min, max)

struct config_item config_tab[] = {
+ GLOB_ITEM_INT("assume_two_step", 0, 0, 1),
PORT_ITEM_INT("udp_ttl", 1, 1, 255),
};

@@ -527,12 +528,6 @@ static enum parser_result parse_global_setting(const char *option,
cfg->dds.freq_est_interval = val;
pod->freq_est_interval = val;

- } else if (!strcmp(option, "assume_two_step")) {
- r = get_ranged_int(value, &val, 0, 1);
- if (r != PARSED_OK)
- return r;
- *cfg->assume_two_step = val;
-
} else if (!strcmp(option, "tx_timestamp_timeout")) {
r = get_ranged_int(value, &val, 1, INT_MAX);
if (r != PARSED_OK)
diff --git a/config.h b/config.h
index b2b8865..a96a88f 100644
--- a/config.h
+++ b/config.h
@@ -70,7 +70,6 @@ struct config {

struct default_ds dds;
struct port_defaults pod;
- int *assume_two_step;
int *tx_timestamp_timeout;
int *check_fup_sync;

diff --git a/ptp4l.c b/ptp4l.c
index e2482c8..042e5aa 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -102,7 +102,6 @@ static struct config cfg_settings = {
.dm = DM_E2E,
.transport = TRANS_UDP_IPV4,

- .assume_two_step = &assume_two_step,
.tx_timestamp_timeout = &sk_tx_timeout,
.check_fup_sync = &sk_check_fupsync,

@@ -279,6 +278,8 @@ int main(int argc, char *argv[])
if (config && (c = config_read(config, &cfg_settings))) {
return c;
}
+
+ assume_two_step = config_get_int(&cfg_settings, NULL, "assume_two_step");
if (!cfg_settings.dds.grand_master_capable &&
ds->flags & DDS_SLAVE_ONLY) {
fprintf(stderr,

[Richard Cochran]

Signed-off-by: Richard Cochran <***@gmail.com>
---
config.c | 7 +------
config.h | 2 --
ptp4l.c | 4 +---
3 files changed, 2 insertions(+), 11 deletions(-)

diff --git a/config.c b/config.c
index ce3378a..9a240d7 100644
--- a/config.c
+++ b/config.c
@@ -80,6 +80,7 @@ struct config_item {

struct config_item config_tab[] = {
GLOB_ITEM_INT("assume_two_step", 0, 0, 1),
+ GLOB_ITEM_INT("check_fup_sync", 0, 0, 1),
GLOB_ITEM_INT("tx_timestamp_timeout", 1, 1, INT_MAX),
PORT_ITEM_INT("udp_ttl", 1, 1, 255),
};
@@ -529,12 +530,6 @@ static enum parser_result parse_global_setting(const char *option,
cfg->dds.freq_est_interval = val;
pod->freq_est_interval = val;

- } else if (!strcmp(option, "check_fup_sync")) {
- r = get_ranged_int(value, &val, 0, 1);
- if (r != PARSED_OK)
- return r;
- *cfg->check_fup_sync = val;
-
} else if (!strcmp(option, "pi_proportional_const")) {
r = get_ranged_double(value, &df, 0.0, DBL_MAX);
if (r != PARSED_OK)
diff --git a/config.h b/config.h
index 3cd279c..1b9192d 100644
--- a/config.h
+++ b/config.h
@@ -70,8 +70,6 @@ struct config {

struct default_ds dds;
struct port_defaults pod;
- int *check_fup_sync;
-
enum servo_type clock_servo;

double *step_threshold;
diff --git a/ptp4l.c b/ptp4l.c
index f8972a6..4c6415b 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -101,9 +101,6 @@ static struct config cfg_settings = {
.timestamping = TS_HARDWARE,
.dm = DM_E2E,
.transport = TRANS_UDP_IPV4,
-
- .check_fup_sync = &sk_check_fupsync,
-
.clock_servo = CLOCK_SERVO_PI,

.step_threshold = &servo_step_threshold,
@@ -279,6 +276,7 @@ int main(int argc, char *argv[])
}

assume_two_step = config_get_int(&cfg_settings, NULL, "assume_two_step");
+ sk_check_fupsync = config_get_int(&cfg_settings, NULL, "check_fup_sync");
sk_tx_timeout = config_get_int(&cfg_settings, NULL, "tx_timestamp_timeout");

if (!cfg_settings.dds.grand_master_capable &&

[Richard Cochran]

This will allow removing the code that passes configuration options via
global variables.

Signed-off-by: Richard Cochran <***@gmail.com>
---
clock.c | 4 ++--
phc2sys.c | 3 ++-
servo.c | 3 ++-
servo.h | 5 ++++-
4 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/clock.c b/clock.c
index a47e1e9..8a5ef05 100644
--- a/clock.c
+++ b/clock.c
@@ -866,7 +866,7 @@ struct clock *clock_create(struct config *config, int phc_index,
the actual frequency of the clock. */
clockadj_set_freq(c->clkid, fadj);
}
- c->servo = servo_create(servo, -fadj, max_adj, sw_ts);
+ c->servo = servo_create(c->config, servo, -fadj, max_adj, sw_ts);
if (!c->servo) {
pr_err("Failed to create clock servo");
return NULL;
@@ -1356,7 +1356,7 @@ int clock_switch_phc(struct clock *c, int phc_index)
}
fadj = (int) clockadj_get_freq(clkid);
clockadj_set_freq(clkid, fadj);
- servo = servo_create(c->servo_type, -fadj, max_adj, 0);
+ servo = servo_create(c->config, c->servo_type, -fadj, max_adj, 0);
if (!servo) {
pr_err("Switching PHC, failed to create clock servo");
phc_close(clkid);
diff --git a/phc2sys.c b/phc2sys.c
index 92af7ad..45ae363 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -224,7 +224,8 @@ static struct clock *clock_add(struct node *node, char *device)
}
}

- c->servo = servo_create(node->servo_type, -ppb, max_ppb, 0);
+ c->servo = servo_create(&phc2sys_config, node->servo_type,
+ -ppb, max_ppb, 0);
servo_sync_interval(c->servo, node->phc_interval);

if (clkid != CLOCK_REALTIME)
diff --git a/servo.c b/servo.c
index b936a85..aa71b21 100644
--- a/servo.c
+++ b/servo.c
@@ -30,7 +30,8 @@ double servo_step_threshold = 0.0;
double servo_first_step_threshold = 0.00002; /* 20 microseconds */
int servo_max_frequency = 900000000;

-struct servo *servo_create(enum servo_type type, int fadj, int max_ppb, int sw_ts)
+struct servo *servo_create(struct config *cfg, enum servo_type type,
+ int fadj, int max_ppb, int sw_ts)
{
struct servo *servo;

diff --git a/servo.h b/servo.h
index 0a182a4..cc0b098 100644
--- a/servo.h
+++ b/servo.h
@@ -22,6 +22,8 @@

#include <stdint.h>

+struct config;
+
/**
* When set to a non-zero value, this variable controls the maximum allowed
* offset before a clock jump occurs instead of the default clock-slewing
@@ -92,7 +94,8 @@ enum servo_state {
* and the servo should use more aggressive filtering.
* @return A pointer to a new servo on success, NULL otherwise.
*/
-struct servo *servo_create(enum servo_type type, int fadj, int max_ppb, int sw_ts);
+struct servo *servo_create(struct config *cfg, enum servo_type type,
+ int fadj, int max_ppb, int sw_ts);

/**
* Destroy an instance of a clock servo.

[Richard Cochran]

This patch adds functions that will set and lock a certain value. The
intended use of these methods is to give command line options priority
over the configuration file.

Signed-off-by: Richard Cochran <***@gmail.com>
---
config.c | 28 ++++++++++++++++++++++++++++
config.h | 3 +++
2 files changed, 31 insertions(+)

diff --git a/config.c b/config.c
index 9a240d7..817f625 100644
--- a/config.c
+++ b/config.c
@@ -1054,3 +1054,31 @@ unsigned int config_get_uint(struct config *cfg, const char *port,
pr_debug("config item %s.%s is %u", port, option, ci->val.u);
return ci->val.u;
}
+
+int config_set_double(struct config *cfg, const char *option, double val)
+{
+ struct config_item *ci = config_find_item(cfg, NULL, option);
+
+ if (!ci || ci->type != CFG_TYPE_DOUBLE) {
+ pr_err("bug: config option %s missing or invalid!", option);
+ return -1;
+ }
+ ci->flags |= CFG_ITEM_LOCKED;
+ ci->val.d = val;
+ pr_debug("config item global.%s locked as %f", option, ci->val.d);
+ return 0;
+}
+
+int config_set_int(struct config *cfg, const char *option, int val)
+{
+ struct config_item *ci = config_find_item(cfg, NULL, option);
+
+ if (!ci || ci->type != CFG_TYPE_INT) {
+ pr_err("bug: config option %s missing or invalid!", option);
+ return -1;
+ }
+ ci->flags |= CFG_ITEM_LOCKED;
+ ci->val.i = val;
+ pr_debug("config item global.%s locked as %d", option, ci->val.i);
+ return 0;
+}
diff --git a/config.h b/config.h
index 1b9192d..dd45d7e 100644
--- a/config.h
+++ b/config.h
@@ -114,4 +114,7 @@ int config_get_int(struct config *cfg, const char *port,
unsigned int config_get_uint(struct config *cfg, const char *port,
const char *option);

+int config_set_double(struct config *cfg, const char *option, double val);
+int config_set_int(struct config *cfg, const char *option, int val);
+
#endif

[Richard Cochran]

Signed-off-by: Richard Cochran <***@gmail.com>
---
config.c | 21 +++++++++++++++------
config.h | 1 -
phc2sys.c | 9 ++++++---
ptp4l.c | 1 -
servo.c | 4 +++-
servo.h | 9 ---------
6 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/config.c b/config.c
index 817f625..16388f3 100644
--- a/config.c
+++ b/config.c
@@ -63,6 +63,14 @@ struct config_item {

#define N_CONFIG_ITEMS (sizeof(config_tab) / sizeof(config_tab[0]))

+#define CONFIG_ITEM_DBL(_label, _port, _default, _min, _max) { \
+ .label = _label, \
+ .type = CFG_TYPE_DOUBLE, \
+ .flags = _port ? CFG_ITEM_PORT : 0, \
+ .val.d = _default, \
+ .min.d = _min, \
+ .max.d = _max, \
+}
#define CONFIG_ITEM_INT(_label, _port, _default, _min, _max) { \
.label = _label, \
.type = CFG_TYPE_INT, \
@@ -72,15 +80,22 @@ struct config_item {
.max.i = _max, \
}

+#define GLOB_ITEM_DBL(label, _default, min, max) \
+ CONFIG_ITEM_DBL(label, 0, _default, min, max)
+
#define GLOB_ITEM_INT(label, _default, min, max) \
CONFIG_ITEM_INT(label, 0, _default, min, max)

+#define PORT_ITEM_DBL(label, _default, min, max) \
+ CONFIG_ITEM_DBL(label, 1, _default, min, max)
+
#define PORT_ITEM_INT(label, _default, min, max) \
CONFIG_ITEM_INT(label, 1, _default, min, max)

struct config_item config_tab[] = {
GLOB_ITEM_INT("assume_two_step", 0, 0, 1),
GLOB_ITEM_INT("check_fup_sync", 0, 0, 1),
+ GLOB_ITEM_DBL("step_threshold", 0.0, 0.0, DBL_MAX),
GLOB_ITEM_INT("tx_timestamp_timeout", 1, 1, INT_MAX),
PORT_ITEM_INT("udp_ttl", 1, 1, 255),
};
@@ -578,12 +593,6 @@ static enum parser_result parse_global_setting(const char *option,
return r;
*cfg->pi_integral_norm_max = df;

- } else if (!strcmp(option, "step_threshold")) {
- r = get_ranged_double(value, &df, 0.0, DBL_MAX);
- if (r != PARSED_OK)
- return r;
- *cfg->step_threshold = df;
-
} else if (!strcmp(option, "first_step_threshold")) {
r = get_ranged_double(value, &df, 0.0, DBL_MAX);
if (r != PARSED_OK)
diff --git a/config.h b/config.h
index dd45d7e..4834f1d 100644
--- a/config.h
+++ b/config.h
@@ -72,7 +72,6 @@ struct config {
struct port_defaults pod;
enum servo_type clock_servo;

- double *step_threshold;
double *first_step_threshold;
int *max_frequency;

diff --git a/phc2sys.c b/phc2sys.c
index 45ae363..fa75607 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -1222,11 +1222,12 @@ int main(int argc, char *argv[])
char *progname;
char *src_name = NULL, *dst_name = NULL;
struct clock *src, *dst;
+ struct config *cfg;
int autocfg = 0, rt = 0;
int c, domain_number = 0, pps_fd = -1;
int r, wait_sync = 0;
int print_level = LOG_INFO, use_syslog = 1, verbose = 0;
- double phc_rate;
+ double phc_rate, tmp;
struct node node = {
.sanity_freq_limit = 200000000,
.servo_type = CLOCK_SERVO_PI,
@@ -1240,6 +1241,7 @@ int main(int argc, char *argv[])
if (config_init(&phc2sys_config)) {
return -1;
}
+ cfg = &phc2sys_config;

configured_pi_kp = KP;
configured_pi_ki = KI;
@@ -1297,8 +1299,9 @@ int main(int argc, char *argv[])
return -1;
break;
case 'S':
- if (get_arg_val_d(c, optarg, &servo_step_threshold,
- 0.0, DBL_MAX))
+ if (get_arg_val_d(c, optarg, &tmp, 0.0, DBL_MAX))
+ return -1;
+ if (config_set_double(cfg, "step_threshold", tmp))
return -1;
break;
case 'F':
diff --git a/ptp4l.c b/ptp4l.c
index 4c6415b..994cfb4 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -103,7 +103,6 @@ static struct config cfg_settings = {
.transport = TRANS_UDP_IPV4,
.clock_servo = CLOCK_SERVO_PI,

- .step_threshold = &servo_step_threshold,
.first_step_threshold = &servo_first_step_threshold,
.max_frequency = &servo_max_frequency,

diff --git a/servo.c b/servo.c
index aa71b21..f1eddc8 100644
--- a/servo.c
+++ b/servo.c
@@ -18,6 +18,7 @@
*/
#include <string.h>

+#include "config.h"
#include "linreg.h"
#include "ntpshm.h"
#include "nullf.h"
@@ -26,13 +27,13 @@

#define NSEC_PER_SEC 1000000000

-double servo_step_threshold = 0.0;
double servo_first_step_threshold = 0.00002; /* 20 microseconds */
int servo_max_frequency = 900000000;

struct servo *servo_create(struct config *cfg, enum servo_type type,
int fadj, int max_ppb, int sw_ts)
{
+ double servo_step_threshold;
struct servo *servo;

switch (type) {
@@ -52,6 +53,7 @@ struct servo *servo_create(struct config *cfg, enum servo_type type,
return NULL;
}

+ servo_step_threshold = config_get_double(cfg, NULL, "step_threshold");
if (servo_step_threshold > 0.0) {
servo->step_threshold = servo_step_threshold * NSEC_PER_SEC;
} else {
diff --git a/servo.h b/servo.h
index cc0b098..88d926b 100644
--- a/servo.h
+++ b/servo.h
@@ -25,15 +25,6 @@
struct config;

/**
- * When set to a non-zero value, this variable controls the maximum allowed
- * offset before a clock jump occurs instead of the default clock-slewing
- * mechanism.
- *
- * Note that this variable is measured in seconds, and allows fractional values.
- */
-extern double servo_step_threshold;
-
-/**
* When set to zero, the clock is not stepped on start. When set to a non-zero
* value, the value bahaves as a threshold and the clock is stepped on start if
* the offset is bigger than the threshold.

[Richard Cochran]

Signed-off-by: Richard Cochran <***@gmail.com>
---
config.c | 7 +------
config.h | 1 -
phc2sys.c | 5 +++--
ptp4l.c | 1 -
servo.c | 5 ++++-
servo.h | 9 ---------
6 files changed, 8 insertions(+), 20 deletions(-)

diff --git a/config.c b/config.c
index 16388f3..08c89a0 100644
--- a/config.c
+++ b/config.c
@@ -95,6 +95,7 @@ struct config_item {
struct config_item config_tab[] = {
GLOB_ITEM_INT("assume_two_step", 0, 0, 1),
GLOB_ITEM_INT("check_fup_sync", 0, 0, 1),
+ GLOB_ITEM_DBL("first_step_threshold", 0.00002, 0.0, DBL_MAX),
GLOB_ITEM_DBL("step_threshold", 0.0, 0.0, DBL_MAX),
GLOB_ITEM_INT("tx_timestamp_timeout", 1, 1, INT_MAX),
PORT_ITEM_INT("udp_ttl", 1, 1, 255),
@@ -593,12 +594,6 @@ static enum parser_result parse_global_setting(const char *option,
return r;
*cfg->pi_integral_norm_max = df;

- } else if (!strcmp(option, "first_step_threshold")) {
- r = get_ranged_double(value, &df, 0.0, DBL_MAX);
- if (r != PARSED_OK)
- return r;
- *cfg->first_step_threshold = df;
-
} else if (!strcmp(option, "max_frequency")) {
r = get_ranged_int(value, &val, 0, INT_MAX);
if (r != PARSED_OK)
diff --git a/config.h b/config.h
index 4834f1d..775a79e 100644
--- a/config.h
+++ b/config.h
@@ -72,7 +72,6 @@ struct config {
struct port_defaults pod;
enum servo_type clock_servo;

- double *first_step_threshold;
int *max_frequency;

double *pi_proportional_const;
diff --git a/phc2sys.c b/phc2sys.c
index fa75607..44ba1fc 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -1305,8 +1305,9 @@ int main(int argc, char *argv[])
return -1;
break;
case 'F':
- if (get_arg_val_d(c, optarg, &servo_first_step_threshold,
- 0.0, DBL_MAX))
+ if (get_arg_val_d(c, optarg, &tmp, 0.0, DBL_MAX))
+ return -1;
+ if (config_set_double(cfg, "first_step_threshold", tmp))
return -1;
break;
case 'R':
diff --git a/ptp4l.c b/ptp4l.c
index 994cfb4..08ea3cd 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -103,7 +103,6 @@ static struct config cfg_settings = {
.transport = TRANS_UDP_IPV4,
.clock_servo = CLOCK_SERVO_PI,

- .first_step_threshold = &servo_first_step_threshold,
.max_frequency = &servo_max_frequency,

.pi_proportional_const = &configured_pi_kp,
diff --git a/servo.c b/servo.c
index f1eddc8..4f26ea6 100644
--- a/servo.c
+++ b/servo.c
@@ -27,12 +27,12 @@

#define NSEC_PER_SEC 1000000000

-double servo_first_step_threshold = 0.00002; /* 20 microseconds */
int servo_max_frequency = 900000000;

struct servo *servo_create(struct config *cfg, enum servo_type type,
int fadj, int max_ppb, int sw_ts)
{
+ double servo_first_step_threshold;
double servo_step_threshold;
struct servo *servo;

@@ -60,6 +60,9 @@ struct servo *servo_create(struct config *cfg, enum servo_type type,
servo->step_threshold = 0.0;
}

+ servo_first_step_threshold =
+ config_get_double(cfg, NULL, "first_step_threshold");
+
if (servo_first_step_threshold > 0.0) {
servo->first_step_threshold =
servo_first_step_threshold * NSEC_PER_SEC;
diff --git a/servo.h b/servo.h
index 88d926b..b5252df 100644
--- a/servo.h
+++ b/servo.h
@@ -25,15 +25,6 @@
struct config;

/**
- * When set to zero, the clock is not stepped on start. When set to a non-zero
- * value, the value bahaves as a threshold and the clock is stepped on start if
- * the offset is bigger than the threshold.
- *
- * Note that this variable is measured in seconds, and allows fractional values.
- */
-extern double servo_first_step_threshold;
-
-/**
* When set to a non-zero value, this variable sets an additional limit for
* the frequency adjustment of the clock. It's in ppb.
*/

[Richard Cochran]

Signed-off-by: Richard Cochran <***@gmail.com>
---
config.c | 7 +------
config.h | 2 --
ptp4l.c | 2 --
servo.c | 4 ++--
servo.h | 6 ------
5 files changed, 3 insertions(+), 18 deletions(-)

diff --git a/config.c b/config.c
index 08c89a0..f2ce71e 100644
--- a/config.c
+++ b/config.c
@@ -96,6 +96,7 @@ struct config_item config_tab[] = {
GLOB_ITEM_INT("assume_two_step", 0, 0, 1),
GLOB_ITEM_INT("check_fup_sync", 0, 0, 1),
GLOB_ITEM_DBL("first_step_threshold", 0.00002, 0.0, DBL_MAX),
+ GLOB_ITEM_INT("max_frequency", 900000000, 0, INT_MAX),
GLOB_ITEM_DBL("step_threshold", 0.0, 0.0, DBL_MAX),
GLOB_ITEM_INT("tx_timestamp_timeout", 1, 1, INT_MAX),
PORT_ITEM_INT("udp_ttl", 1, 1, 255),
@@ -594,12 +595,6 @@ static enum parser_result parse_global_setting(const char *option,
return r;
*cfg->pi_integral_norm_max = df;

- } else if (!strcmp(option, "max_frequency")) {
- r = get_ranged_int(value, &val, 0, INT_MAX);
- if (r != PARSED_OK)
- return r;
- *cfg->max_frequency = val;
-
} else if (!strcmp(option, "sanity_freq_limit")) {
r = get_ranged_int(value, &val, 0, INT_MAX);
if (r != PARSED_OK)
diff --git a/config.h b/config.h
index 775a79e..86d153a 100644
--- a/config.h
+++ b/config.h
@@ -72,8 +72,6 @@ struct config {
struct port_defaults pod;
enum servo_type clock_servo;

- int *max_frequency;
-
double *pi_proportional_const;
double *pi_integral_const;
double *pi_proportional_scale;
diff --git a/ptp4l.c b/ptp4l.c
index 08ea3cd..3985beb 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -103,8 +103,6 @@ static struct config cfg_settings = {
.transport = TRANS_UDP_IPV4,
.clock_servo = CLOCK_SERVO_PI,

- .max_frequency = &servo_max_frequency,
-
.pi_proportional_const = &configured_pi_kp,
.pi_integral_const = &configured_pi_ki,
.pi_proportional_scale = &configured_pi_kp_scale,
diff --git a/servo.c b/servo.c
index 4f26ea6..2e82ebe 100644
--- a/servo.c
+++ b/servo.c
@@ -27,13 +27,12 @@

#define NSEC_PER_SEC 1000000000

-int servo_max_frequency = 900000000;
-
struct servo *servo_create(struct config *cfg, enum servo_type type,
int fadj, int max_ppb, int sw_ts)
{
double servo_first_step_threshold;
double servo_step_threshold;
+ int servo_max_frequency;
struct servo *servo;

switch (type) {
@@ -70,6 +69,7 @@ struct servo *servo_create(struct config *cfg, enum servo_type type,
servo->first_step_threshold = 0.0;
}

+ servo_max_frequency = config_get_int(cfg, NULL, "max_frequency");
servo->max_frequency = max_ppb;
if (servo_max_frequency && servo->max_frequency > servo_max_frequency) {
servo->max_frequency = servo_max_frequency;
diff --git a/servo.h b/servo.h
index b5252df..f90befd 100644
--- a/servo.h
+++ b/servo.h
@@ -24,12 +24,6 @@

struct config;

-/**
- * When set to a non-zero value, this variable sets an additional limit for
- * the frequency adjustment of the clock. It's in ppb.
- */
-extern int servo_max_frequency;
-
/** Opaque type */
struct servo;

[Keller, Jacob E]

Hi Richard,

Post by Richard Cochran
- API is much simpler to use
- numerous details in the implementation have been improved
- half dozen legacy options converted

Nice changes. Looks very straight forward to use.

Post by Richard Cochran
The config code was simplistic at first and has grown many warts over
time. The pointers to global variables are rather gross, and it is
hard to add new options, especially port specific ones. The number of
configuration options is sure to grow larger over time.

Yep.

Post by Richard Cochran
This series addresses the issues by implementing a hash table based
implementation. Adding a new option is as simple as placing the item
into the config_tab[] table (one line), and then calling
var=config_get_TYPE() at the usage site.

Nice. Super easy to add, and it looks like the various modes support
most of our current options.

Post by Richard Cochran
This series add one new option and converts six legacy options. Many
if not all of the remaining legacy options can be converted to the new
system. Once this is done, the config code will be much more compact
and maintainable. In addition, the phc2sys and pmc programs will also
be able to use a configuration file.

Very useful. Will take some time to setup all the options for each
file.

Post by Richard Cochran
Patches 1-4 and 12 provide changes allowing passing of the
configuration.
Patch 5 and 6 add a simple hash table.
Patch 7 adds the bulk of the new implementation.
Patch 8 lets the UDP transport use the new TTL option.
Patches 9-11 and 14-16 convert some legacy options.
Patch 13 adds a way to "lock" options from the command line.
Review and comments are most welcome.

General comments:

it doesn't appear to enforce any specific change on configuration file
format. I know my earlier proposal was maybe to at some point update
the format. Specifically, change the per-port configurations to use
"port:" or some prefix which might help ensure possible future
expansion of sections. The primary reasoning being separating out
options used by differing programs.

It seems that is something we could handle in the future, though.

Are there any options you think might be more difficult to convert? I
think some of the string options might not move as cleanly to the new
API.. each call site would have to parse the string instead of parsing
an enumeration. Maybe that can be solved by having a GLOB_ITEM_ENUM or
something where the setup passes in a map of string->value that could
be used instead, so that callsites get an enum/int of values instead of
strings?

Other comment is whether we want phc2sys and ptp4l to read from the
same configuration file or should they just read separate ones.

Aside from my general comments, I looked over the series and it looks
great. Most of my comments here wouldn't sway me to say don't apply it
now. We can always continue to work towards moving all the options from
legacy into the new format.

Regards,
Jake
------------------------------------------------------------------------------

[Richard Cochran]

Post by Keller, Jacob E
it doesn't appear to enforce any specific change on configuration file
format. I know my earlier proposal was maybe to at some point update
the format. Specifically, change the per-port configurations to use
"port:" or some prefix which might help ensure possible future
expansion of sections. The primary reasoning being separating out
options used by differing programs.

Although I have "port" as an identifier, the new code does not care
about the section names at all. It would work with files that had:

[phc2sys]
someOption 1

[pmc]
somethingElse 0

[future-section]
whatever 1

It would be up to the caller to specify the desired section. For
example:

x = config_get_int(cfg, "phc2sys", "someOption");

The only built in assumption is that every option can appear under
[global] as a default. It does *not* support options that can only
appear in a non-global section.

I will replace "port" with "section" in conifg.h and config.c.

Post by Keller, Jacob E
Are there any options you think might be more difficult to convert? I
think some of the string options might not move as cleanly to the new
API.. each call site would have to parse the string instead of parsing
an enumeration. Maybe that can be solved by having a GLOB_ITEM_ENUM or
something where the setup passes in a map of string->value that could
be used instead, so that callsites get an enum/int of values instead of
strings?

Yes, I was thinking of ENUM options with little string tables
attached, but I haven't yet worked out the details.

Post by Keller, Jacob E
Other comment is whether we want phc2sys and ptp4l to read from the
same configuration file or should they just read separate ones.

I am not sure what is better. We had some discussion on this point
that I should look up again. The new code doesn't really care.

If we wanted distinct sets of legal options, then we would need
separate config_tab[] instances and different config_init() functions
(or one config_init(flavor) with switch/case inside). That change
will be easy to add, if we decide we need it later on.

Post by Keller, Jacob E
Aside from my general comments, I looked over the series and it looks
great. Most of my comments here wouldn't sway me to say don't apply it
now. We can always continue to work towards moving all the options from
legacy into the new format.

Thanks for the review!

Richard

------------------------------------------------------------------------------

[Richard Cochran]

Post by Richard Cochran
The only built in assumption is that every option can appear under
[global] as a default. It does *not* support options that can only
appear in a non-global section.

No, I am mistaken. The new code doesn't support overriding arbitrary
global options in a section. This won't work in general:

[global]
someOption 1

[phc2sys]
someOption 2

That only works if "someOption" is marked as port-specific item.
However, this can be done later on. Something like this:

enum config_section {
GLOBAL_SECTION,
PORT_SECTION,
+ PROGRAM_SECTION,
UNKNOWN_SECTION,
};

#define CFG_ITEM_STATIC (1 << 0) /* statically allocated, not to be freed */
#define CFG_ITEM_LOCKED (1 << 1) /* command line value, may not be changed */
#define CFG_ITEM_PORT (1 << 2) /* item may appear in port sections */
+ #define CFG_ITEM_PROG (1 << 3) /* default may be overriden in a program section */

The parse_item() code would have to change. But that will be easier
to do, once all of the manual parsing in parse_port_setting() and
parse_global_setting() is gone.

Whether we implement this or not depends on whether we want a
single configuration file for all three programs or whether we just
say, each program must have its own file.

Thanks,
Richard


------------------------------------------------------------------------------

[Keller, Jacob E]

Post by Richard Cochran

Post by Richard Cochran
The only built in assumption is that every option can appear under
[global] as a default. It does *not* support options that can only
appear in a non-global section.

No, I am mistaken. The new code doesn't support overriding arbitrary

I am not sure we really need this.

Post by Richard Cochran
[global]
someOption 1
[phc2sys]
someOption 2
That only works if "someOption" is marked as port-specific item.
enum config_section {
GLOBAL_SECTION,
PORT_SECTION,
+ PROGRAM_SECTION,
UNKNOWN_SECTION,
};
#define CFG_ITEM_STATIC (1 << 0) /* statically allocated, not
to be freed */
#define CFG_ITEM_LOCKED (1 << 1) /* command line value, may not be changed */
#define CFG_ITEM_PORT (1 << 2) /* item may appear in port
sections */
+ #define CFG_ITEM_PROG (1 << 3) /* default may be overriden
in a program section */
The parse_item() code would have to change. But that will be easier
to do, once all of the manual parsing in parse_port_setting() and
parse_global_setting() is gone.
Whether we implement this or not depends on whether we want a
single configuration file for all three programs or whether we just
say, each program must have its own file.

I think we can work on this when/if we decide programs need it.

Regards,
Jake

Post by Richard Cochran
Thanks,
Richard

------------------------------------------------------------------------------

[Keller, Jacob E]

Post by Richard Cochran

Post by Keller, Jacob E
it doesn't appear to enforce any specific change on configuration file
format. I know my earlier proposal was maybe to at some point update
the format. Specifically, change the per-port configurations to use
"port:" or some prefix which might help ensure possible future
expansion of sections. The primary reasoning being separating out
options used by differing programs.

Although I have "port" as an identifier, the new code does not care

Ok. What I meant was that we today have per-port via:

[enp2s0f0]
<options>

which indicates a port, and not a section. Maybe we should change this
to be:

[port:enp2s0f0] or something in the oddball case where someone uses
udev et.el to rename their device "global" or similar.. maybe we don't
really care about this though.

Post by Richard Cochran
[phc2sys]
someOption 1
[pmc]
somethingElse 0
[future-section]
whatever 1
It would be up to the caller to specify the desired section. For
x = config_get_int(cfg, "phc2sys", "someOption");

That's fine, and makes sense to me.

Post by Richard Cochran
The only built in assumption is that every option can appear under
[global] as a default. It does *not* support options that can only
appear in a non-global section.
I will replace "port" with "section" in conifg.h and config.c.

Post by Keller, Jacob E
Are there any options you think might be more difficult to convert? I
think some of the string options might not move as cleanly to the new
API.. each call site would have to parse the string instead of parsing
an enumeration. Maybe that can be solved by having a GLOB_ITEM_ENUM or
something where the setup passes in a map of string->value that could
be used instead, so that callsites get an enum/int of values
instead of
strings?

Yes, I was thinking of ENUM options with little string tables
attached, but I haven't yet worked out the details.

This seems the most straight forward way.

Post by Richard Cochran

Post by Keller, Jacob E
Other comment is whether we want phc2sys and ptp4l to read from the
same configuration file or should they just read separate ones.

I am not sure what is better. We had some discussion on this point
that I should look up again. The new code doesn't really care.

I think we probably can make them read from the same one.

Post by Richard Cochran
If we wanted distinct sets of legal options, then we would need
separate config_tab[] instances and different config_init() functions
(or one config_init(flavor) with switch/case inside). That change
will be easy to add, if we decide we need it later on.

Ya that's what I thought. I don't think we need to worry too much as
long as each option takes the same values for each configuration, as
this way you could use the same file and only the options each program
needs are read/used.

Regards,
Jake
------------------------------------------------------------------------------

[Richard Cochran]

Post by Keller, Jacob E
[enp2s0f0]
<options>
which indicates a port, and not a section. Maybe we should change this
[port:enp2s0f0] or something in the oddball case where someone uses
udev et.el to rename their device "global" or similar.. maybe we don't
really care about this though.

Oh, I never imagined that. Here is an alternate solution:

[port]
interface_device eth0

I am inclined to wait until somebody actually complains about linuxptp
not working on their weird system.

Thanks,
Richard



------------------------------------------------------------------------------