[Linuxptp-devel] [PATCH RFC 0/3] Configurable UDS server path

[Richard Cochran]

This patch series shows a way how to make the address of the server's
UNIX domain socket configurable at run time. This can be useful when
running multiple ptp4l instances. There are at least two uses cases for
this, namely when testing over a virtual tuntap network, and when
running multiple grand masters on different ports with different
settings.

Warning: compile tested only. Comments and testing are appreciated.

Thanks,
Richard


Richard Cochran (3):
Convert the hard coded UDS server path into a variable.
Introduce a configuration file option for the server's UDS address.
pmc: add a command line option to select the server's UDS address.

clock.c | 2 +-
config.c | 5 +++++
config.h | 1 +
default.cfg | 1 +
pmc.c | 12 +++++++++++-
ptp4l.c | 2 ++
uds.c | 6 ++++--
uds.h | 3 ++-
8 files changed, 27 insertions(+), 5 deletions(-)

[Richard Cochran]

This patch changes the macro for the server socket address into a global
variable so that a subsequent patch can provide a way to set the variable.

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

diff --git a/clock.c b/clock.c
index cf65fff..419a6b7 100644
--- a/clock.c
+++ b/clock.c
@@ -576,7 +576,7 @@ struct clock *clock_create(int phc_index, struct interface *iface, int count,
struct interface udsif;

memset(&udsif, 0, sizeof(udsif));
- snprintf(udsif.name, sizeof(udsif.name), UDS_PATH);
+ snprintf(udsif.name, sizeof(udsif.name), "%s", uds_path);
udsif.transport = TRANS_UDS;

srandom(time(NULL));
diff --git a/uds.c b/uds.c
index 0d114c3..1dbfd6c 100644
--- a/uds.c
+++ b/uds.c
@@ -30,6 +30,8 @@
#include "transport_private.h"
#include "uds.h"

+char uds_path[MAX_IFNAME_SIZE + 1] = "/var/run/ptp4l";
+
#define UDS_FILEMODE (S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP) /*0660*/

struct uds {
@@ -58,7 +60,7 @@ static int uds_open(struct transport *t, char *name, struct fdarray *fda,
}
memset(&sa, 0, sizeof(sa));
sa.sun_family = AF_LOCAL;
- strcpy(sa.sun_path, name);
+ strncpy(sa.sun_path, name, sizeof(sa.sun_path) - 1);

unlink(name);

@@ -72,7 +74,7 @@ static int uds_open(struct transport *t, char *name, struct fdarray *fda,
/* For client use, pre load the server path. */
memset(&sa, 0, sizeof(sa));
sa.sun_family = AF_LOCAL;
- strcpy(sa.sun_path, UDS_PATH);
+ strncpy(sa.sun_path, uds_path, sizeof(sa.sun_path) - 1);
uds->sa = sa;
uds->len = sizeof(sa);

diff --git a/uds.h b/uds.h
index 24effa8..d7f6626 100644
--- a/uds.h
+++ b/uds.h
@@ -20,13 +20,14 @@
#ifndef HAVE_UDS_H
#define HAVE_UDS_H

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

/**
* Address of the server.
*/
-#define UDS_PATH "/var/run/ptp4l"
+extern char uds_path[MAX_IFNAME_SIZE + 1];

/**
* Allocate an instance of a UDS transport.

[Richard Cochran]

Signed-off-by: Richard Cochran <***@gmail.com>
---
config.c | 5 +++++
config.h | 1 +
default.cfg | 1 +
ptp4l.c | 2 ++
4 files changed, 9 insertions(+)

diff --git a/config.c b/config.c
index 8cae57f..fe4c0f3 100644
--- a/config.c
+++ b/config.c
@@ -393,6 +393,11 @@ static enum parser_result parse_global_setting(const char *option,
return r;
*cfg->udp6_scope = uval;

+ } else if (!strcmp(option, "uds_address")) {
+ if (strlen(value) > MAX_IFNAME_SIZE)
+ return OUT_OF_RANGE;
+ strncpy(cfg->uds_address, value, MAX_IFNAME_SIZE);
+
} else if (!strcmp(option, "logging_level")) {
r = get_ranged_int(value, &val,
PRINT_LEVEL_MIN, PRINT_LEVEL_MAX);
diff --git a/config.h b/config.h
index 94704a4..4a2cbaa 100644
--- a/config.h
+++ b/config.h
@@ -89,6 +89,7 @@ struct config {
unsigned char *ptp_dst_mac;
unsigned char *p2p_dst_mac;
unsigned char *udp6_scope;
+ char *uds_address;

int print_level;
int use_syslog;
diff --git a/default.cfg b/default.cfg
index 72665a6..0a42a92 100644
--- a/default.cfg
+++ b/default.cfg
@@ -59,6 +59,7 @@ transportSpecific 0x0
ptp_dst_mac 01:1B:19:00:00:00
p2p_dst_mac 01:80:C2:00:00:0E
udp6_scope 0x0E
+uds_address /var/run/ptp4l
#
# Default interface options
#
diff --git a/ptp4l.c b/ptp4l.c
index b0d1c9c..dac303a 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -32,6 +32,7 @@
#include "sk.h"
#include "transport.h"
#include "udp6.h"
+#include "uds.h"
#include "util.h"
#include "version.h"

@@ -111,6 +112,7 @@ static struct config cfg_settings = {
.ptp_dst_mac = ptp_dst_mac,
.p2p_dst_mac = p2p_dst_mac,
.udp6_scope = &udp6_scope,
+ .uds_address = uds_path,

.print_level = LOG_INFO,
.use_syslog = 1,

[Keller, Jacob E]

Comments inlined,

Post by Richard Cochran
---
config.c | 5 +++++
config.h | 1 +
default.cfg | 1 +
ptp4l.c | 2 ++
4 files changed, 9 insertions(+)
diff --git a/config.c b/config.c
index 8cae57f..fe4c0f3 100644
--- a/config.c
+++ b/config.c
@@ -393,6 +393,11 @@ static enum parser_result parse_global_setting(const char *option,
return r;
*cfg->udp6_scope = uval;
+ } else if (!strcmp(option, "uds_address")) {
+ if (strlen(value) > MAX_IFNAME_SIZE)
+ return OUT_OF_RANGE;

What's the reasoning on the restriction to MAX_IFNAME_SIZE? That puts a
pretty short limit on path name sizing that seems really arbitrary..

Post by Richard Cochran
+ strncpy(cfg->uds_address, value, MAX_IFNAME_SIZE);
+
} else if (!strcmp(option, "logging_level")) {
r = get_ranged_int(value, &val,
PRINT_LEVEL_MIN, PRINT_LEVEL_MAX);
diff --git a/config.h b/config.h
index 94704a4..4a2cbaa 100644
--- a/config.h
+++ b/config.h
@@ -89,6 +89,7 @@ struct config {
unsigned char *ptp_dst_mac;
unsigned char *p2p_dst_mac;
unsigned char *udp6_scope;
+ char *uds_address;
int print_level;
int use_syslog;
diff --git a/default.cfg b/default.cfg
index 72665a6..0a42a92 100644
--- a/default.cfg
+++ b/default.cfg
@@ -59,6 +59,7 @@ transportSpecific 0x0
ptp_dst_mac 01:1B:19:00:00:00
p2p_dst_mac 01:80:C2:00:00:0E
udp6_scope 0x0E
+uds_address /var/run/ptp4l

You forgot to add this to the gPTP.cfg file as well.

Post by Richard Cochran
#
# Default interface options
#
diff --git a/ptp4l.c b/ptp4l.c
index b0d1c9c..dac303a 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -32,6 +32,7 @@
#include "sk.h"
#include "transport.h"
#include "udp6.h"
+#include "uds.h"
#include "util.h"
#include "version.h"
@@ -111,6 +112,7 @@ static struct config cfg_settings = {
.ptp_dst_mac = ptp_dst_mac,
.p2p_dst_mac = p2p_dst_mac,
.udp6_scope = &udp6_scope,
+ .uds_address = uds_path,
.print_level = LOG_INFO,
.use_syslog = 1,

Regar

[Richard Cochran]

Post by Keller, Jacob E

Post by Richard Cochran
+ } else if (!strcmp(option, "uds_address")) {
+ if (strlen(value) > MAX_IFNAME_SIZE)
+ return OUT_OF_RANGE;

What's the reasoning on the restriction to MAX_IFNAME_SIZE? That puts a
pretty short limit on path name sizing that seems really arbitrary..

No special reason. We have one field for both interface names (like
eth0) and the uds (previously hard coded /var/run/ptp4l). The existing
length of 16 allows having /var/run/ptp4l00 to /var/run/ptp4l99, for
example. That seems like plenty to me.

But we can also surely afford longer paths, if anyone want them. IIRC,
the maximum uds address is 108 bytes.

Post by Keller, Jacob E
You forgot to add this to the gPTP.cfg file as well.

Okay, thanks.

Richard

[Keller, Jacob E]

Post by Richard Cochran

Post by Keller, Jacob E

Post by Richard Cochran
+ } else if (!strcmp(option, "uds_address")) {
+ if (strlen(value) > MAX_IFNAME_SIZE)
+ return OUT_OF_RANGE;

What's the reasoning on the restriction to MAX_IFNAME_SIZE? That puts a
pretty short limit on path name sizing that seems really arbitrary..

No special reason. We have one field for both interface names (like
eth0) and the uds (previously hard coded /var/run/ptp4l). The existing
length of 16 allows having /var/run/ptp4l00 to /var/run/ptp4l99, for
example. That seems like plenty to me.

That is, but maybe someone wanted to use a more descriptive name for the
UDS? At least we should document how long it is..

Personally I'm for extending it simply because we have the option of
doing so, and so much of the 16 characters is eaten by /var/run/... path
settings.

But it isn't really a huge issue either way.

Regards,
Jake

Post by Richard Cochran
But we can also surely afford longer paths, if anyone want them. IIRC,
the maximum uds address is 108 bytes.

Post by Keller, Jacob E
You forgot to add this to the gPTP.cfg file as well.

Okay, thanks.
Richar

[Richard Cochran]

Signed-off-by: Richard Cochran <***@gmail.com>
---
pmc.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/pmc.c b/pmc.c
index 3a6d697..c9851cd 100644
--- a/pmc.c
+++ b/pmc.c
@@ -31,6 +31,7 @@
#include "pmc_common.h"
#include "print.h"
#include "tlv.h"
+#include "uds.h"
#include "util.h"
#include "version.h"

@@ -676,6 +677,7 @@ static void usage(char *progname)
" -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"
+ " -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"
" -z send zero length TLV values with the GET actions\n"
@@ -697,7 +699,7 @@ int main(int argc, char *argv[])
/* Process the command line arguments. */
progname = strrchr(argv[0], '/');
progname = progname ? 1+progname : argv[0];
- while (EOF != (c = getopt(argc, argv, "246u""b:d:hi:t:vz"))) {
+ while (EOF != (c = getopt(argc, argv, "246u""b:d:hi:s:t:vz"))) {
switch (c) {
case '2':
transport_type = TRANS_IEEE_802_3;
@@ -720,6 +722,14 @@ int main(int argc, char *argv[])
case 'i':
iface_name = optarg;
break;
+ case 's':
+ 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 't':
if (1 == sscanf(optarg, "%x", &c))
transport_specific = c << 4;

[Miroslav Lichvar]

Post by Richard Cochran
This patch series shows a way how to make the address of the server's
UNIX domain socket configurable at run time. This can be useful when
running multiple ptp4l instances. There are at least two uses cases for
this, namely when testing over a virtual tuntap network, and when
running multiple grand masters on different ports with different
settings.
Warning: compile tested only. Comments and testing are appreciated.

It seems to be working fine here. But I agree with Jacob, the maximum
allowed length of the path could be longer. Can you please also add a
description of the new option to the ptp4l man page?