[Linuxptp-devel] [PATCH] ptp4l: allow path to phc-device (-p) to be relative (possibly containing symlinks)

[Ivan Oleynikov]

This is simply icquired by using `realpath(3)` of value passed with -p
parameter instead of its original value.

Signed-off-by: Ivan Oleynikov <***@metrotek.spb.ru>
---
ptp4l.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/ptp4l.c b/ptp4l.c
index a87e7e6..090b75a 100644
--- a/ptp4l.c
+++ b/ptp4l.c
@@ -22,6 +22,8 @@
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
+#include <limits.h>
+#include <errno.h>

#include "clock.h"
#include "config.h"
@@ -72,7 +74,8 @@ static void usage(char *progname)

int main(int argc, char *argv[])
{
- char *config = NULL, *req_phc = NULL, *progname;
+ char req_phc[PATH_MAX];
+ char *config = NULL, *progname;
int c, err = -1, print_level;
struct clock *clock = NULL;
struct config *cfg;
@@ -137,7 +140,11 @@ int main(int argc, char *argv[])
goto out;
break;
case 'p':
- req_phc = optarg;
+ if (!realpath(optarg, req_phc)) {
+ pr_err("realpath(%s): %s", optarg, strerror(errno));
+ goto out;
+ }
+ pr_debug("using %s as realpath of %s", req_phc, optarg);
break;
case 's':
if (config_set_int(cfg, "slaveOnly", 1)) {

[Richard Cochran]

Post by Ivan Oleynikov
This is simply icquired by using `realpath(3)` of value passed with -p
parameter instead of its original value.

Can you please explain what problem this patch fixes?

Thanks,
Richard

[Richard Cochran]

Post by Richard Cochran
Can you please explain what problem this patch fixes?

Never mind, I understand what this is about. The code scans
"/dev/ptp%d", and that fails on symlinks to /dev/ptpX.

Having said that, I am not too wild about this patch. Why can't users
with strange symlinks teach their scripts to use REALPATH(1) ?


Thanks,
Richard

[Ivan Oleynikov]

Post by Richard Cochran
Having said that, I am not too wild about this patch. Why can't users
with strange symlinks teach their scripts to use REALPATH(1) ?

Why would some strange ptp4l require char devices to be named as /dev/ptp%d
through the kernel doesn't put such restrictions on device names (as far as i
know)?

Why can't I rename a PHC char device in udev and use ptp4l after that?

The users can call ptp4l not only from strange script, but also directly. Other
utilities in linuxptp (phc2sys, phc_ctl) work well with symlinks.

--
Ivan Oleynikov
STC Metrotek
St.Petersburg

[Ivan Oleynikov]

Post by Richard Cochran

Post by Ivan Oleynikov
This is simply icquired by using `realpath(3)` of value passed with -p
parameter instead of its original value.

Can you please explain what problem this patch fixes?

Imagine i have a symlink to my /dev/ptp0:

ln -s /dev/ptp0 /dev/my_ptp

If i try to syncronize that PHC over the network interface gbe0 i get:

# ptp4l -p /dev/my_ptp -i gbe0 -m -s
ptp4l[2117.975]: bad ptp device string
failed to create a clock

It seems strange that ptp4l expects path to ptp device to be in the form
"/dev/ptp%d". This also forbids me to specify a relative path to PHC
device. E.g:

# ptp4l -p /dev/../dev/ptp0 -i gbe0 -m -s
ptp4l[2268.015]: bad ptp device string
failed to create a clock

Working with PHC through a symlink instead of directly passing /dev/ptp0 to
`ptl4l -p` can be useful when you have multiple PTP clocks in your system and
the order in which they are registered is unpredictable (as well as name of your
desired PHC file in /dev/). So you have to look at
/sys/class/ptp/ptp*/{clock_name, device} to find the name of your device.

Alternative solution would be to create udev rule matching ptp device with
specific DRIVERS, KERNELS, etc. that will add a symlink (e.g. /dev/my_ptp in the
first example) to your device.

This patch makes ptp4l find full path to PHC using `realpath(3)` before doing
anything else with it. Thus allowing me to:

# ptp4l -p /dev/../dev/ptp0 -i gbe0 -m -s
ptp4l[5346.225]: selected /dev/ptp0 as PTP clock

And also to:

# ptp4l -p /dev/my_ptp -i gbe0 -m -s
ptp4l[5368.645]: selected /dev/ptp0 as PTP clock

Better solution would be to determine phc_index by given PHC char device path
without parsing its path (maybe by looking at sysfs or by some ioctl), but I
couldn't find a way to do that. This would allow me to create char device with
arbitrary name for my PHC (with mknod or by renaming it in udev).

--
Ivan Oleynikov
STC Metrotek
St.Petersburg

[Richard Cochran]

Post by Ivan Oleynikov
Working with PHC through a symlink instead of directly passing /dev/ptp0 to
`ptl4l -p` can be useful when you have multiple PTP clocks in your system and
the order in which they are registered is unpredictable (as well as name of your
desired PHC file in /dev/).

Right, they are unpredictable. Because we have the ethtool info,
ptp4l doesn't care and neither should you.

Post by Ivan Oleynikov
So you have to look at
/sys/class/ptp/ptp*/{clock_name, device} to find the name of your device.

No, don't do that...

Post by Ivan Oleynikov
Alternative solution would be to create udev rule matching ptp device with
specific DRIVERS, KERNELS, etc. that will add a symlink (e.g. /dev/my_ptp in the
first example) to your device.

and don't do that either.

Just let your MAC driver advertise the associated PHC index, and leave
udev, etc, alone.

Thanks,
Richard

[Ivan Oleynikov]

Post by Richard Cochran
Just let your MAC driver advertise the associated PHC index, and leave
udev, etc, alone.

Thank you for your feedback. It was helpful.

--
Ivan Oleynikov
STC Metrotek
St.Petersburg

[Richard Cochran]

Post by Ivan Oleynikov
Thank you for your feedback. It was helpful.

No problem! I just checked the man page and saw that the
documentation of the -p option is misleading and wrong.
That will be fixed now.

Thanks,
Richard