Discussion:
[Linuxptp-devel] [RFC PATCH 0/2] fixes for phc2sys based on user mistake
Jacob Keller
2013-05-14 22:02:59 UTC
Permalink
The following series implements some fixes based on emails on the linux-ptp
mailing list. The first patch implements network interface detection so that
'-s' and '-c' will attempt to use sk_get_ts_info if the device isn't
CLOCK_REALTIME or a phc device. The '-i' option was kept for compatability with
scripts, but was marked deprecrated and warns the user that it shouldn't be
used. (it now just performs the same function as -s). The second patch modifies
the usage checking of phc2sys and prints out a specific warning that indicates
why the usage is being shown. This should enable slightly better notification of
what went wrong.

---

Jacob Keller (2):
phc2sys: update open_clock and deprecate '-i' option
phc2sys: update usage/error reporting


phc2sys.8 | 37 +++++++++++++++++------------------
phc2sys.c | 65 ++++++++++++++++++++++++++++++++++++++-----------------------
2 files changed, 58 insertions(+), 44 deletions(-)

--
Signature
Jacob Keller
2013-05-14 22:03:43 UTC
Permalink
This patch modifies phc2sys to enable the use of iface names in clock_open
rather than having to do that by hand. This enables cleaner use of the -s and -c
options as they can accept interface names. This also enables the user to set
the slave clock by iface as well. The '-i' option was kept (performing the same
function as '-s') in order for any scripts to not break if they update.

Signed-off-by: Jacob Keller <***@intel.com>
---
phc2sys.8 | 37 ++++++++++++++++++-------------------
phc2sys.c | 46 +++++++++++++++++++++++-----------------------
2 files changed, 41 insertions(+), 42 deletions(-)

diff --git a/phc2sys.8 b/phc2sys.8
index f5f72b4..1868919 100644
--- a/phc2sys.8
+++ b/phc2sys.8
@@ -4,18 +4,12 @@ phc2sys \- synchronize two clocks

.SH SYNOPSIS
.B phc2sys
-{
-.BI \-d " pps-device"
[
-.BI \-s " phc-device"
-|
-.BI \-i " interface"
-] |
-.BI \-s " phc-device"
-|
-.BI \-i " interface"
-} [
-.BI \-c " phc-device"
+.BI \-d " pps-device"
+] [
+.BI \-s " device"
+] [
+.BI \-c " device"
] [
.BI \-P " kp"
] [
@@ -71,21 +65,26 @@ is started or the
option should be used too. This option can be used only with the system clock as
the slave clock.
.TP
-.BI \-s " phc-device"
-Specify the master clock by device (e.g. /dev/ptp0) or name (e.g. CLOCK_REALTIME
-for the system clock). When this option is used together with the
+.BI \-s " device"
+Specify the master clock by device (e.g. dev/ptp0) or interface (e.g. eth0) or
+by name (e.g. CLOCK_REALTIME for the system clock). When this option is used
+together with the
.B \-d
option, the master clock is used only to correct the offset by whole number of
seconds, which cannot be fixed with PPS alone.
.TP
.BI \-i " interface"
-Similar to the
+Performs the exact same function as
+.B \-s
+for compatability reasons. Previously enabled specifying master clock by network
+interface. However, this can now be done using
.B \-s
-option, but specified by the interface which provides the master clock.
+and this option is no longer necessary. As such it has been deprecated, and
+should no longer be used.
.TP
-.BI \-c " phc-device"
-Specify the slave clock by device (e.g. /dev/ptp1) or name. The default is
-CLOCK_REALTIME (the system clock).
+.BI \-c " device"
+Specify the slave clock by device (e.g. /dev/ptp1) or interface (e.g. eth1) or
+by name. The default is CLOCK_REALTIME (the system clock).
.TP
.BI \-P " kp"
Specify the proportional constant of the PI controller. The default is 0.7.
diff --git a/phc2sys.c b/phc2sys.c
index dbe2d9a..9c3482d 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -61,16 +61,31 @@ static int update_sync_offset(struct clock *clock, int64_t offset, uint64_t ts);

static clockid_t clock_open(char *device)
{
+ struct sk_ts_info ts_info;
+ char phc_device[16];
int clkid;

- if (device[0] != '/') {
- if (!strcasecmp(device, "CLOCK_REALTIME"))
- return CLOCK_REALTIME;
+ /* check if device is CLOCK_REALTIME */
+ if (!strcasecmp(device, "CLOCK_REALTIME"))
+ return CLOCK_REALTIME;

- fprintf(stderr, "unknown clock %s\n", device);
+ /* check if device is valid phc device */
+ clkid = phc_open(device);
+ if (clkid != CLOCK_INVALID)
+ return clkid;
+
+ /* check if device is a valid ethernet device */
+ if (sk_get_ts_info(device, &ts_info) || !ts_info.valid) {
+ fprintf(stderr, "unknown clock %s: %m\n", device);
return CLOCK_INVALID;
}

+ if (ts_info.phc_index < 0) {
+ fprintf(stderr, "interface %s does not have a PHC\n", device);
+ return CLOCK_INVALID;
+ }
+
+ sprintf(phc_device, "/dev/ptp%d", ts_info.phc_index);
clkid = phc_open(device);
if (clkid == CLOCK_INVALID)
fprintf(stderr, "cannot open %s: %m\n", device);
@@ -535,7 +550,6 @@ static void usage(char *progname)
" -c [dev|name] slave clock (CLOCK_REALTIME)\n"
" -d [dev] master PPS device\n"
" -s [dev|name] master clock\n"
- " -i [iface] master clock by network interface\n"
" -P [kp] proportional constant (0.7)\n"
" -I [ki] integration constant (0.3)\n"
" -S [step] step threshold (disabled)\n"
@@ -557,7 +571,7 @@ static void usage(char *progname)

int main(int argc, char *argv[])
{
- char *progname, *ethdev = NULL;
+ char *progname;
clockid_t src = CLOCK_INVALID;
int c, domain_number = 0, phc_readings = 5, pps_fd = -1;
int max_ppb, r, wait_sync = 0, forced_sync_offset = 0;
@@ -589,6 +603,9 @@ int main(int argc, char *argv[])
return -1;
}
break;
+ case 'i':
+ fprintf(stderr,
+ "'-i' has been deprecated. please use '-s' instead.\n");
case 's':
src = clock_open(optarg);
break;
@@ -612,9 +629,6 @@ int main(int argc, char *argv[])
dst_clock.sync_offset_direction = -1;
forced_sync_offset = 1;
break;
- case 'i':
- ethdev = optarg;
- break;
case 'u':
dst_clock.stats_max_count = atoi(optarg);
break;
@@ -648,20 +662,6 @@ int main(int argc, char *argv[])
}
}

- if (src == CLOCK_INVALID && ethdev) {
- struct sk_ts_info ts_info;
- char phc_device[16];
- if (sk_get_ts_info(ethdev, &ts_info) || !ts_info.valid) {
- fprintf(stderr, "can't autodiscover PHC device\n");
- return -1;
- }
- if (ts_info.phc_index < 0) {
- fprintf(stderr, "interface %s doesn't have a PHC\n", ethdev);
- return -1;
- }
- sprintf(phc_device, "/dev/ptp%d", ts_info.phc_index);
- src = clock_open(phc_device);
- }
if (!(pps_fd >= 0 || src != CLOCK_INVALID) ||
dst_clock.clkid == CLOCK_INVALID ||
(pps_fd >= 0 && dst_clock.clkid != CLOCK_REALTIME)) {
Keller, Jacob E
2013-05-14 22:10:10 UTC
Permalink
This patch had a mistake. I am sending out a v2 now.

- Jake
-----Original Message-----
Sent: Tuesday, May 14, 2013 3:04 PM
Subject: [Linuxptp-devel] [RFC PATCH 1/2] phc2sys: update open_clock
and deprecate '-i' option
This patch modifies phc2sys to enable the use of iface names in clock_open
rather than having to do that by hand. This enables cleaner use of the -s and -c
options as they can accept interface names. This also enables the user to set
the slave clock by iface as well. The '-i' option was kept (performing the same
function as '-s') in order for any scripts to not break if they update.
---
phc2sys.8 | 37 ++++++++++++++++++-------------------
phc2sys.c | 46 +++++++++++++++++++++++-----------------------
2 files changed, 41 insertions(+), 42 deletions(-)
diff --git a/phc2sys.8 b/phc2sys.8
index f5f72b4..1868919 100644
--- a/phc2sys.8
+++ b/phc2sys.8
@@ -4,18 +4,12 @@ phc2sys \- synchronize two clocks
.SH SYNOPSIS
.B phc2sys
-{
-.BI \-d " pps-device"
[
-.BI \-s " phc-device"
-|
-.BI \-i " interface"
-] |
-.BI \-s " phc-device"
-|
-.BI \-i " interface"
-} [
-.BI \-c " phc-device"
+.BI \-d " pps-device"
+] [
+.BI \-s " device"
+] [
+.BI \-c " device"
] [
.BI \-P " kp"
] [
@@ -71,21 +65,26 @@ is started or the
option should be used too. This option can be used only with the system clock as
the slave clock.
.TP
-.BI \-s " phc-device"
-Specify the master clock by device (e.g. /dev/ptp0) or name (e.g. CLOCK_REALTIME
-for the system clock). When this option is used together with the
+.BI \-s " device"
+Specify the master clock by device (e.g. dev/ptp0) or interface (e.g. eth0) or
+by name (e.g. CLOCK_REALTIME for the system clock). When this
option is used
+together with the
.B \-d
option, the master clock is used only to correct the offset by whole number of
seconds, which cannot be fixed with PPS alone.
.TP
.BI \-i " interface"
-Similar to the
+Performs the exact same function as
+.B \-s
+for compatability reasons. Previously enabled specifying master clock by network
+interface. However, this can now be done using
.B \-s
-option, but specified by the interface which provides the master clock.
+and this option is no longer necessary. As such it has been deprecated, and
+should no longer be used.
.TP
-.BI \-c " phc-device"
-Specify the slave clock by device (e.g. /dev/ptp1) or name. The default is
-CLOCK_REALTIME (the system clock).
+.BI \-c " device"
+Specify the slave clock by device (e.g. /dev/ptp1) or interface (e.g. eth1) or
+by name. The default is CLOCK_REALTIME (the system clock).
.TP
.BI \-P " kp"
Specify the proportional constant of the PI controller. The default is 0.7.
diff --git a/phc2sys.c b/phc2sys.c
index dbe2d9a..9c3482d 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -61,16 +61,31 @@ static int update_sync_offset(struct clock *clock,
int64_t offset, uint64_t ts);
static clockid_t clock_open(char *device)
{
+ struct sk_ts_info ts_info;
+ char phc_device[16];
int clkid;
- if (device[0] != '/') {
- if (!strcasecmp(device, "CLOCK_REALTIME"))
- return CLOCK_REALTIME;
+ /* check if device is CLOCK_REALTIME */
+ if (!strcasecmp(device, "CLOCK_REALTIME"))
+ return CLOCK_REALTIME;
- fprintf(stderr, "unknown clock %s\n", device);
+ /* check if device is valid phc device */
+ clkid = phc_open(device);
+ if (clkid != CLOCK_INVALID)
+ return clkid;
+
+ /* check if device is a valid ethernet device */
+ if (sk_get_ts_info(device, &ts_info) || !ts_info.valid) {
+ fprintf(stderr, "unknown clock %s: %m\n", device);
return CLOCK_INVALID;
}
+ if (ts_info.phc_index < 0) {
+ fprintf(stderr, "interface %s does not have a PHC\n",
device);
+ return CLOCK_INVALID;
+ }
+
+ sprintf(phc_device, "/dev/ptp%d", ts_info.phc_index);
clkid = phc_open(device);
if (clkid == CLOCK_INVALID)
fprintf(stderr, "cannot open %s: %m\n", device);
@@ -535,7 +550,6 @@ static void usage(char *progname)
" -c [dev|name] slave clock (CLOCK_REALTIME)\n"
" -d [dev] master PPS device\n"
" -s [dev|name] master clock\n"
- " -i [iface] master clock by network interface\n"
" -P [kp] proportional constant (0.7)\n"
" -I [ki] integration constant (0.3)\n"
" -S [step] step threshold (disabled)\n"
@@ -557,7 +571,7 @@ static void usage(char *progname)
int main(int argc, char *argv[])
{
- char *progname, *ethdev = NULL;
+ char *progname;
clockid_t src = CLOCK_INVALID;
int c, domain_number = 0, phc_readings = 5, pps_fd = -1;
int max_ppb, r, wait_sync = 0, forced_sync_offset = 0;
@@ -589,6 +603,9 @@ int main(int argc, char *argv[])
return -1;
}
break;
+ fprintf(stderr,
+ "'-i' has been deprecated. please use '-s'
instead.\n");
src = clock_open(optarg);
break;
@@ -612,9 +629,6 @@ int main(int argc, char *argv[])
dst_clock.sync_offset_direction = -1;
forced_sync_offset = 1;
break;
- ethdev = optarg;
- break;
dst_clock.stats_max_count = atoi(optarg);
break;
@@ -648,20 +662,6 @@ int main(int argc, char *argv[])
}
}
- if (src == CLOCK_INVALID && ethdev) {
- struct sk_ts_info ts_info;
- char phc_device[16];
- if (sk_get_ts_info(ethdev, &ts_info) || !ts_info.valid) {
- fprintf(stderr, "can't autodiscover PHC
device\n");
- return -1;
- }
- if (ts_info.phc_index < 0) {
- fprintf(stderr, "interface %s doesn't have a
PHC\n", ethdev);
- return -1;
- }
- sprintf(phc_device, "/dev/ptp%d", ts_info.phc_index);
- src = clock_open(phc_device);
- }
if (!(pps_fd >= 0 || src != CLOCK_INVALID) ||
dst_clock.clkid == CLOCK_INVALID ||
(pps_fd >= 0 && dst_clock.clkid != CLOCK_REALTIME)) {
------------------------------------------------------------------------------
AlienVault Unified Security Management (USM) platform delivers
complete
security visibility with the essential security capabilities. Easily and
efficiently configure, manage, and operate all of your security controls
from a single console and one unified framework. Download a free trial.
http://p.sf.net/sfu/alienvault_d2d
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Libor Pechacek
2013-05-17 09:14:54 UTC
Permalink
Jacob,
Post by Jacob Keller
@@ -589,6 +603,9 @@ int main(int argc, char *argv[])
return -1;
}
break;
+ fprintf(stderr,
+ "'-i' has been deprecated. please use '-s' instead.\n");
Wouldn't it be better to send this message to system log with pr_warn()? In
case phc2sys is invoked during boot the deprecation message can easily pass by
while in the system log there are higher chances the sysadmin will notice.

Libor
Miroslav Lichvar
2013-05-17 09:22:01 UTC
Permalink
Post by Libor Pechacek
Post by Jacob Keller
+ fprintf(stderr,
+ "'-i' has been deprecated. please use '-s' instead.\n");
Wouldn't it be better to send this message to system log with pr_warn()? In
case phc2sys is invoked during boot the deprecation message can easily pass by
while in the system log there are higher chances the sysadmin will notice.
At that point, the printing facility is not configured yet, see the
print_set_* functions later in the code. The admin might want all
messages on stdout/stderr.
--
Miroslav Lichvar
Libor Pechacek
2013-05-17 09:56:17 UTC
Permalink
Post by Miroslav Lichvar
Post by Libor Pechacek
Post by Jacob Keller
+ fprintf(stderr,
+ "'-i' has been deprecated. please use '-s' instead.\n");
Wouldn't it be better to send this message to system log with pr_warn()? In
case phc2sys is invoked during boot the deprecation message can easily pass by
while in the system log there are higher chances the sysadmin will notice.
At that point, the printing facility is not configured yet, see the
print_set_* functions later in the code. The admin might want all
messages on stdout/stderr.
Good point, -m will have no effect in this case, the message will appear only
in system log.

Libor
Keller, Jacob E
2013-05-17 18:51:30 UTC
Permalink
-----Original Message-----
Sent: Friday, May 17, 2013 2:56 AM
To: Miroslav Lichvar
Subject: Re: [Linuxptp-devel] [RFC PATCH 1/2] phc2sys: update
open_clock and deprecate '-i' option
Post by Miroslav Lichvar
Post by Libor Pechacek
Post by Jacob Keller
+ fprintf(stderr,
+ "'-i' has been deprecated. please use '-s'
instead.\n");
Post by Miroslav Lichvar
Post by Libor Pechacek
Wouldn't it be better to send this message to system log with
pr_warn()? In
Post by Miroslav Lichvar
Post by Libor Pechacek
case phc2sys is invoked during boot the deprecation message can
easily pass by
Post by Miroslav Lichvar
Post by Libor Pechacek
while in the system log there are higher chances the sysadmin will
notice.
Post by Miroslav Lichvar
At that point, the printing facility is not configured yet, see the
print_set_* functions later in the code. The admin might want all
messages on stdout/stderr.
Good point, -m will have no effect in this case, the message will appear only
in system log.
Libor
You're probably right, but it won't have a huge impact because the -I does at least what it did before..

- Jake

Jacob Keller
2013-05-14 22:03:56 UTC
Permalink
This patch updates phc2sys usage reporting to give a slightly better indication
of why the program was unable to run.

Signed-off-by: Jacob Keller <***@intel.com>
---
phc2sys.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/phc2sys.c b/phc2sys.c
index 9c3482d..75531dc 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -662,9 +662,23 @@ int main(int argc, char *argv[])
}
}

- if (!(pps_fd >= 0 || src != CLOCK_INVALID) ||
- dst_clock.clkid == CLOCK_INVALID ||
- (pps_fd >= 0 && dst_clock.clkid != CLOCK_REALTIME)) {
+ if (pps_fd < 0 && src == CLOCK_INVALID) {
+ fprintf(stderr,
+ "valid source clock must be selected.\n");
+ usage(progname);
+ return -1;
+ }
+
+ if (dst_clock.clkid == CLOCK_INVALID) {
+ fprintf(stderr,
+ "valid destination clock must be selected.\n");
+ usage(progname);
+ return -1;
+ }
+
+ if (pps_fd >= 0 && dst_clock.clkid != CLOCK_REALTIME) {
+ fprintf(stderr,
+ "cannot use a pps device unless destination is CLOCK_REALTIME\n");
usage(progname);
return -1;
}
Loading...