[Linuxptp-devel] [PATCH 1/5] phc2sys: Add option to set path to ptp4l UDS.

[Miroslav Lichvar]

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
phc2sys.8 | 4 ++++
phc2sys.c | 12 +++++++++++-
2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/phc2sys.8 b/phc2sys.8
index a906fb3..c4fb6f6 100644
--- a/phc2sys.8
+++ b/phc2sys.8
@@ -194,6 +194,10 @@ clock frequency (unless the
.B \-S
option is used).
.TP
+.BI \-z " uds-address"
+Specifies the address of the server's UNIX domain socket.
+The default is /var/run/ptp4l.
+.TP
.BI \-l " print-level"
Set the maximum syslog level of messages which should be printed or sent to
the system logger. The default is 6 (LOG_INFO).
diff --git a/phc2sys.c b/phc2sys.c
index 7815a8e..418f4ac 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -53,6 +53,7 @@
#include "stats.h"
#include "sysoff.h"
#include "tlv.h"
+#include "uds.h"
#include "util.h"
#include "version.h"

@@ -1158,6 +1159,7 @@ static void usage(char *progname)
" -u [num] number of clock updates in summary stats (0)\n"
" -n [num] domain number (0)\n"
" -x apply leap seconds by servo instead of kernel\n"
+ " -z [path] server address for UDS (/var/run/ptp4l)\n"
" -l [num] set the logging level to 'num' (6)\n"
" -m print messages to stdout\n"
" -q do not print messages to the syslog\n"
@@ -1192,7 +1194,7 @@ int main(int argc, char *argv[])
progname = strrchr(argv[0], '/');
progname = progname ? 1+progname : argv[0];
while (EOF != (c = getopt(argc, argv,
- "arc:d:s:E:P:I:S:F:R:N:O:L:i:u:wn:xl:mqvh"))) {
+ "arc:d:s:E:P:I:S:F:R:N:O:L:i:u:wn:xz:l:mqvh"))) {
switch (c) {
case 'a':
autocfg = 1;
@@ -1285,6 +1287,14 @@ int main(int argc, char *argv[])
case 'x':
node.kernel_leap = 0;
break;
+ case 'z':
+ if (strlen(optarg) > MAX_IFNAME_SIZE) {
+ fprintf(stderr, "path %s too long, max is %d\n",
+ optarg, MAX_IFNAME_SIZE);
+ return -1;
+ }
+ strncpy(uds_path, optarg, MAX_IFNAME_SIZE);
+ break;
case 'l':
if (get_arg_val_i(c, optarg, &print_level,
PRINT_LEVEL_MIN, PRINT_LEVEL_MAX))

[Miroslav Lichvar]

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
uds.c | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/uds.c b/uds.c
index e98e32c..4039aff 100644
--- a/uds.c
+++ b/uds.c
@@ -42,6 +42,14 @@ struct uds {

static int uds_close(struct transport *t, struct fdarray *fda)
{
+ struct sockaddr_un sa;
+ socklen_t len = sizeof(sa);
+
+ if (!getsockname(fda->fd[FD_GENERAL], &sa, &len) &&
+ sa.sun_family == AF_LOCAL) {
+ unlink(sa.sun_path);
+ }
+
close(fda->fd[FD_GENERAL]);
return 0;
}

[Richard Cochran]

This one introduces a new warning. I will fix it up with this.

Thanks,
Richard


diff --git a/uds.c b/uds.c
index 4039aff..97edb97 100644
--- a/uds.c
+++ b/uds.c
@@ -45,7 +45,7 @@ static int uds_close(struct transport *t, struct fdarray *fda)
struct sockaddr_un sa;
socklen_t len = sizeof(sa);

- if (!getsockname(fda->fd[FD_GENERAL], &sa, &len) &&
+ if (!getsockname(fda->fd[FD_GENERAL], (struct sockaddr *) &sa, &len) &&
sa.sun_family == AF_LOCAL) {
unlink(sa.sun_path);
}

[Miroslav Lichvar]

This will be useful in phc2sys and pmc.

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
ptp4l.c | 23 ++---------------------
util.c | 32 ++++++++++++++++++++++++++++++++
util.h | 14 ++++++++++++++
3 files changed, 48 insertions(+), 21 deletions(-)

diff --git a/ptp4l.c b/ptp4l.c
index 25270c6..5080a79 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -18,7 +18,6 @@
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
*/
#include <limits.h>
-#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
@@ -40,8 +39,6 @@

int assume_two_step = 0;

-static int running = 1;
-
static struct config cfg_settings = {
.dds = {
.dds = {
@@ -131,12 +128,6 @@ static struct config cfg_settings = {
.cfg_ignore = 0,
};

-static void handle_int_quit_term(int s)
-{
- pr_notice("caught signal %d", s);
- running = 0;
-}
-
static void usage(char *progname)
{
fprintf(stderr,
@@ -184,18 +175,8 @@ int main(int argc, char *argv[])
struct defaultDS *ds = &cfg_settings.dds.dds;
int phc_index = -1, required_modes = 0;

- if (SIG_ERR == signal(SIGINT, handle_int_quit_term)) {
- fprintf(stderr, "cannot handle SIGINT\n");
+ if (handle_term_signals())
return -1;
- }
- if (SIG_ERR == signal(SIGQUIT, handle_int_quit_term)) {
- fprintf(stderr, "cannot handle SIGQUIT\n");
- return -1;
- }
- if (SIG_ERR == signal(SIGTERM, handle_int_quit_term)) {
- fprintf(stderr, "cannot handle SIGTERM\n");
- return -1;
- }

/* Set fault timeouts to a default value */
for (i = 0; i < FT_CNT; i++) {
@@ -404,7 +385,7 @@ int main(int argc, char *argv[])
return -1;
}

- while (running) {
+ while (is_running()) {
if (clock_poll(clock))
break;
}
diff --git a/util.c b/util.c
index 1e86040..ae66bb1 100644
--- a/util.c
+++ b/util.c
@@ -17,11 +17,13 @@
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
*/
#include <errno.h>
+#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#include "address.h"
+#include "print.h"
#include "sk.h"
#include "util.h"

@@ -29,6 +31,8 @@
#define NS_PER_HOUR (3600 * NS_PER_SEC)
#define NS_PER_DAY (24 * NS_PER_HOUR)

+static int running = 1;
+
const char *ps_str[] = {
"NONE",
"INITIALIZING",
@@ -311,3 +315,31 @@ int get_arg_val_d(int op, const char *optarg, double *val,
}
return 0;
}
+
+static void handle_int_quit_term(int s)
+{
+ pr_notice("caught signal %d", s);
+ running = 0;
+}
+
+int handle_term_signals(void)
+{
+ if (SIG_ERR == signal(SIGINT, handle_int_quit_term)) {
+ fprintf(stderr, "cannot handle SIGINT\n");
+ return -1;
+ }
+ if (SIG_ERR == signal(SIGQUIT, handle_int_quit_term)) {
+ fprintf(stderr, "cannot handle SIGQUIT\n");
+ return -1;
+ }
+ if (SIG_ERR == signal(SIGTERM, handle_int_quit_term)) {
+ fprintf(stderr, "cannot handle SIGTERM\n");
+ return -1;
+ }
+ return 0;
+}
+
+int is_running(void)
+{
+ return running;
+}
diff --git a/util.h b/util.h
index 3fae51c..cf05e49 100644
--- a/util.h
+++ b/util.h
@@ -212,4 +212,18 @@ int get_arg_val_ui(int op, const char *optarg, unsigned int *val,
int get_arg_val_d(int op, const char *optarg, double *val,
double min, double max);

+/**
+ * Setup a handler for terminating signals (SIGINT, SIGQUIT, SIGTERM).
+ *
+ * @return 0 on success, -1 on error.
+ */
+int handle_term_signals(void);
+
+/**
+ * Check if a terminating signal was received.
+ *
+ * @return 1 if no terminating signal was received, 0 otherwise.
+ */
+int is_running(void);
+
#endif

[Keller, Jacob E]

Post by Miroslav Lichvar
This will be useful in phc2sys and pmc.
---
ptp4l.c | 23 ++---------------------
util.c | 32 ++++++++++++++++++++++++++++++++
util.h | 14 ++++++++++++++
3 files changed, 48 insertions(+), 21 deletions(-)
diff --git a/ptp4l.c b/ptp4l.c
index 25270c6..5080a79 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -18,7 +18,6 @@
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
*/
#include <limits.h>
-#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
@@ -40,8 +39,6 @@
int assume_two_step = 0;
-static int running = 1;
-
static struct config cfg_settings = {
.dds = {
.dds = {
@@ -131,12 +128,6 @@ static struct config cfg_settings = {
.cfg_ignore = 0,
};
-static void handle_int_quit_term(int s)
-{
- pr_notice("caught signal %d", s);
- running = 0;
-}
-
static void usage(char *progname)
{
fprintf(stderr,
@@ -184,18 +175,8 @@ int main(int argc, char *argv[])
struct defaultDS *ds = &cfg_settings.dds.dds;
int phc_index = -1, required_modes = 0;
- if (SIG_ERR == signal(SIGINT, handle_int_quit_term)) {
- fprintf(stderr, "cannot handle SIGINT\n");
+ if (handle_term_signals())
return -1;
- }
- if (SIG_ERR == signal(SIGQUIT, handle_int_quit_term)) {
- fprintf(stderr, "cannot handle SIGQUIT\n");
- return -1;
- }
- if (SIG_ERR == signal(SIGTERM, handle_int_quit_term)) {
- fprintf(stderr, "cannot handle SIGTERM\n");
- return -1;
- }
/* Set fault timeouts to a default value */
for (i = 0; i < FT_CNT; i++) {
@@ -404,7 +385,7 @@ int main(int argc, char *argv[])
return -1;
}
- while (running) {
+ while (is_running()) {

Is there a reason we no longer use the "is_running()" function and just
reference the variable directly?

Regards,

[Miroslav Lichvar]

Post by Keller, Jacob E

Post by Miroslav Lichvar
@@ -404,7 +385,7 @@ int main(int argc, char *argv[])
return -1;
}
- while (running) {
+ while (is_running()) {

Is there a reason we no longer use the "is_running()" function and just
reference the variable directly?

I'm not sure I understand your question. Would you prefer to export
the running variable and use it in other modules directly instead of
the is_running fuction?

[Keller, Jacob E]

Post by Miroslav Lichvar

Post by Keller, Jacob E

Post by Miroslav Lichvar
@@ -404,7 +385,7 @@ int main(int argc, char *argv[])
return -1;
}
- while (running) {
+ while (is_running()) {

Is there a reason we no longer use the "is_running()" function and just
reference the variable directly?

I'm not sure I understand your question. Would you prefer to export
the running variable and use it in other modules directly instead of
the is_running fuction?

Oh nope. I see now you use the variable inside the module, and function
outside.

I was just thinking for consistency to just always use the f

[Miroslav Lichvar]

Post by Keller, Jacob E

Post by Miroslav Lichvar
I'm not sure I understand your question. Would you prefer to export
the running variable and use it in other modules directly instead of
the is_running fuction?

Oh nope. I see now you use the variable inside the module, and function
outside.
I was just thinking for consistency to just always use the function.

I think that's what the patch does, or not? The running variable is
used only in is_running() in util.c and everything else uses
is_running().

[Keller, Jacob E]

Post by Miroslav Lichvar

Post by Keller, Jacob E

Post by Miroslav Lichvar
I'm not sure I understand your question. Would you prefer to export
the running variable and use it in other modules directly instead of
the is_running fuction?

Oh nope. I see now you use the variable inside the module, and function
outside.
I was just thinking for consistency to just always use the function.

I think that's what the patch does, or not? The running variable is
used only in is_running() in util.c and everything else uses
is_running().

Ah yes, you are right, I saw the - line and misread the patch.

Thanks,
Ja

[Miroslav Lichvar]

In pmc and phc2sys handle terminating signals and close the UDS
transport before exit to remove the sockets in /var/run.

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
phc2sys.c | 30 ++++++++++++++++++++----------
pmc.c | 10 +++++++---
2 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/phc2sys.c b/phc2sys.c
index 418f4ac..3039e51 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -526,7 +526,7 @@ static int do_pps_loop(struct node *node, struct clock *clock, int fd)
enable_pps_output(node->master->clkid);
}

- while (1) {
+ while (is_running()) {
if (!read_pps(fd, &pps_offset, &pps_ts)) {
continue;
}
@@ -570,7 +570,7 @@ static int do_loop(struct node *node, int subscriptions)
interval.tv_sec = node->phc_interval;
interval.tv_nsec = (node->phc_interval - interval.tv_sec) * 1e9;

- while (1) {
+ while (is_running()) {
clock_nanosleep(CLOCK_MONOTONIC, 0, &interval, NULL);
if (update_pmc(node, subscriptions) < 0)
continue;
@@ -611,7 +611,7 @@ static int do_loop(struct node *node, int subscriptions)
update_clock(node, clock, offset, ts, delay);
}
}
- return 0; /* unreachable */
+ return 0;
}

static int check_clock_identity(struct node *node, struct ptp_message *msg)
@@ -1187,6 +1187,8 @@ int main(int argc, char *argv[])
.kernel_leap = 1,
};

+ handle_term_signals();
+
configured_pi_kp = KP;
configured_pi_ki = KI;

@@ -1349,7 +1351,8 @@ int main(int argc, char *argv[])
return -1;
if (auto_init_ports(&node, rt) < 0)
return -1;
- return do_loop(&node, 1);
+ r = do_loop(&node, 1);
+ goto end;
}

src = clock_add(&node, src_name);
@@ -1377,14 +1380,16 @@ int main(int argc, char *argv[])
goto bad_usage;
}

+ r = -1;
+
if (wait_sync) {
if (init_pmc(&node, domain_number))
- return -1;
+ goto end;

- while (1) {
+ while (is_running()) {
r = run_pmc_wait_sync(&node, 1000);
if (r < 0)
- return -1;
+ goto end;
if (r > 0)
break;
else
@@ -1395,7 +1400,7 @@ int main(int argc, char *argv[])
r = run_pmc_get_utc_offset(&node, 1000);
if (r <= 0) {
pr_err("failed to get UTC offset");
- return -1;
+ goto end;
}
}

@@ -1409,11 +1414,16 @@ int main(int argc, char *argv[])
/* only one destination clock allowed with PPS until we
* implement a mean to specify PTP port to PPS mapping */
servo_sync_interval(dst->servo, 1.0);
- return do_pps_loop(&node, dst, pps_fd);
+ r = do_pps_loop(&node, dst, pps_fd);
+ } else {
+ r = do_loop(&node, 0);
}

- return do_loop(&node, 0);
+end:
+ if (node.pmc)
+ close_pmc(&node);

+ return r;
bad_usage:
usage(progname);
return -1;
diff --git a/pmc.c b/pmc.c
index ba06293..8cfae92 100644
--- a/pmc.c
+++ b/pmc.c
@@ -714,6 +714,7 @@ int main(int argc, char *argv[])
const char *iface_name = NULL;
char *progname;
int c, cnt, length, tmo = -1, batch_mode = 0, zero_datalen = 0;
+ int ret = 0;
char line[1024], *command = NULL;
enum transport_type transport_type = TRANS_UDP_IPV4;
UInteger8 boundary_hops = 1, domain_number = 0, transport_specific = 0;
@@ -721,6 +722,8 @@ int main(int argc, char *argv[])
#define N_FD 2
struct pollfd pollfd[N_FD];

+ handle_term_signals();
+
/* Process the command line arguments. */
progname = strrchr(argv[0], '/');
progname = progname ? 1+progname : argv[0];
@@ -798,7 +801,7 @@ int main(int argc, char *argv[])
pollfd[0].fd = batch_mode ? -1 : STDIN_FILENO;
pollfd[1].fd = pmc_get_transport_fd(pmc);

- while (1) {
+ while (is_running()) {
if (batch_mode && !command) {
if (optind < argc) {
command = argv[optind++];
@@ -823,7 +826,8 @@ int main(int argc, char *argv[])
continue;
} else {
pr_emerg("poll failed");
- return -1;
+ ret = -1;
+ break;
}
} else if (!cnt) {
break;
@@ -866,5 +870,5 @@ int main(int argc, char *argv[])

pmc_destroy(pmc);
msg_cleanup();
- return 0;
+ return ret;
}

[Miroslav Lichvar]

This allows running multiple phc2sys and pmc instances at the same time.

Signed-off-by: Miroslav Lichvar <***@redhat.com>
---
phc2sys.c | 7 +++++--
pmc.8 | 2 +-
pmc.c | 12 +++++++++---
3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/phc2sys.c b/phc2sys.c
index 3039e51..391ae62 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -738,8 +738,11 @@ static void send_subscription(struct node *node)

static int init_pmc(struct node *node, int domain_number)
{
- node->pmc = pmc_create(TRANS_UDS, "/var/run/phc2sys", 0,
- domain_number, 0, 1);
+ 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);
if (!node->pmc) {
pr_err("failed to create pmc");
return -1;
diff --git a/pmc.8 b/pmc.8
index 0d36354..b113c78 100644
--- a/pmc.8
+++ b/pmc.8
@@ -73,7 +73,7 @@ Specify the boundary hops value in sent messages. The default is 1.
Specify the domain number in sent messages. The default is 0.
.TP
.BI \-i " interface"
-Specify the network interface. The default is /var/run/pmc for the Unix Domain
+Specify the network interface. The default is /var/run/pmc.$pid for the Unix Domain
Socket transport and eth0 for the other transports.
.TP
.BI \-s " uds-address"
diff --git a/pmc.c b/pmc.c
index 8cfae92..d58e190 100644
--- a/pmc.c
+++ b/pmc.c
@@ -700,7 +700,7 @@ static void usage(char *progname)
" -d [num] domain number, default 0\n"
" -h prints this message and exits\n"
" -i [dev] interface device to use, default 'eth0'\n"
- " for network and '/var/run/pmc' for UDS.\n"
+ " for network and '/var/run/pmc.$pid' for UDS.\n"
" -s [path] server address for UDS, default '/var/run/ptp4l'.\n"
" -t [hex] transport specific field, default 0x0\n"
" -v prints the software version and exits\n"
@@ -715,7 +715,7 @@ int main(int argc, char *argv[])
char *progname;
int c, cnt, length, tmo = -1, batch_mode = 0, zero_datalen = 0;
int ret = 0;
- char line[1024], *command = NULL;
+ char line[1024], *command = NULL, uds_local[MAX_IFNAME_SIZE + 1];
enum transport_type transport_type = TRANS_UDP_IPV4;
UInteger8 boundary_hops = 1, domain_number = 0, transport_specific = 0;
struct ptp_message *msg;
@@ -781,7 +781,13 @@ int main(int argc, char *argv[])
}

if (!iface_name) {
- iface_name = transport_type == TRANS_UDS ? "/var/run/pmc" : "eth0";
+ if (transport_type == TRANS_UDS) {
+ snprintf(uds_local, sizeof(uds_local),
+ "/var/run/pmc.%d", getpid());
+ iface_name = uds_local;
+ } else {
+ iface_name = "eth0";
+ }
}
if (optind < argc) {
batch_mode = 1;

[Keller, Jacob E]

The first patch allows using phc2sys with non-default server UDS path.
The next three patches add signal handling to pmc/phc2sys and remove
the UDS socket before exit. The last patch appends the process ID to
the local UDS path so each pmc or phc2sys process has its own socket.
phc2sys: Add option to set path to ptp4l UDS.
Remove socket when closing UDS transport.
Move signal handling to util.c.
Close client UDS transport before exit.
Append PID to client UDS paths.
phc2sys.8 | 4 ++++
phc2sys.c | 49 ++++++++++++++++++++++++++++++++++++-------------
pmc.8 | 2 +-
pmc.c | 22 ++++++++++++++++------
ptp4l.c | 23 ++---------------------
uds.c | 8 ++++++++
util.c | 32 ++++++++++++++++++++++++++++++++
util.h | 14 ++++++++++++++
8 files changed, 113 insertions(+), 41 deletions(-)

Aside from my one nit regarding use if "while(running)" vs maintaining
the use of "while(is_running())",

[Richard Cochran]

The first patch allows using phc2sys with non-default server UDS path.
The next three patches add signal handling to pmc/phc2sys and remove
the UDS socket before exit. The last patch appends the process ID to
the local UDS path so each pmc or phc2sys process has its own socket.
phc2sys: Add option to set path to ptp4l UDS.
Remove socket when closing UDS transport.
Move signal handling to util.c.
Close client UDS transport before exit.
Append PID to client UDS paths.

Series applied.

Thanks,
Richard