On Wed, Jul 06, 2016 at 03:15:29PM +0000, Jesuiter, Henry (ALC NetworX GmbH)
wrote:
> Thank you very much for your fast reply and the notes. Please find below a
> modified patch according to your annotations:
The patch also needs a proper commit message explaining how these new
options are useful and a SoB tag. Except for a few minor nits, this
patch looks good to me.
> @@ -235,7 +237,7 @@
> GLOB_ITEM_STR("uds_address", "/var/run/ptp4l"),
> GLOB_ITEM_INT("use_syslog", 1, 0, 1),
> GLOB_ITEM_STR("userDescription", ""),
> - GLOB_ITEM_INT("verbose", 0, 0, 1),
> + GLOB_ITEM_INT("verbose", 0, 0, 1)
Please leave that comma alone.
> };
> diff -rwbBu a/ptp4l.8 b/ptp4l.8
> --- a/ptp4l.8 2015-09-19 16:25:11.000000000 +0200
> +++ b/ptp4l.8 2016-07-06 16:58:23.722698053 +0200
> @@ -446,6 +446,14 @@
> Specifies the address of the UNIX domain socket for receiving local
> management messages. The default is /var/run/ptp4l.
> .TP
> +.B tos_event
> +Defines the Differentiated Services Codepoint (DSCP) to be used for PTP
> +event messages. Must be a value between 0 and 63. The default is 0.
> +.TP
> +.B tos_general
> +Defines the Differentiated Services Codepoint (DSCP) to be used for PTP
> +general messages. Must be a value between 0 and 63. The default is 0.
> +.TP
It would be nice to keep the numbers consistent, either always decimal
or always hexidecimal. I guess hexidecimal makes more sense (but you
know that better than me).
Also, the man page should give at least one example. What values are
most commonly used here?
> diff -rwbBu a/sk.c b/sk.c
> --- a/sk.c 2015-09-19 16:25:11.000000000 +0200
> +++ b/sk.c 2016-07-06 16:40:27.000000000 +0200
> @@ -35,6 +35,7 @@
> #include "missing.h"
> #include "print.h"
> #include "sk.h"
> +#include "config.h"
>
> /* globals */
>
> @@ -78,6 +79,28 @@
> return 0;
> }
>
> +static int set_priority(int fd, uint8_t dscp) {
Opening brace goes on a line all by itself ------^
> + int tos;
> + socklen_t tos_len;
> +
> + tos_len = sizeof(tos);
> + if (getsockopt(fd, SOL_IP, IP_TOS, &tos, &tos_len) < 0) {
> + tos = 0;
> + }
> +
> + /* clear old DSCP value */
> + tos &= ~0xFC;
> +
> + /* set new DSCP value */
> + tos |= dscp<<2;
> + tos_len = sizeof(tos);
> + if (setsockopt(fd, SOL_IP, IP_TOS, &tos, tos_len) < 0) {
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> /* public methods */
>
> int sk_interface_index(int fd, const char *name)
> @@ -105,6 +128,24 @@
> return 0;
> }
>
> +int sk_set_priority(struct config *c, int event_fd, int general_fd) {
same here
> + uint8_t event_dscp = config_get_int(c, NULL, "tos_event");
> + uint8_t general_dscp = config_get_int(c, NULL, "tos_general");
Hm, does it make sense to let this option be per-port?
In that case, you want to pass the interface name instead of NULL in
argument #2.
> +
> + int err = 0;
> +
> + if(event_dscp != 0) {
space before that (
> + err = set_priority(event_fd, event_dscp);
> + }
> +
> + if(!err && general_dscp) {
ditto
> + err = set_priority(general_fd, general_dscp);
> + }
> +
> + return err;
> +}
> diff -rwbBu a/udp6.c b/udp6.c
> --- a/udp6.c 2015-09-19 16:25:11.000000000 +0200
> +++ b/udp6.c 2016-07-06 16:39:48.000000000 +0200
> @@ -196,6 +196,10 @@
> if (sk_general_init(gfd))
> goto no_timestamping;
>
> + if(sk_set_priority(t->cfg, efd, gfd) < 0) {
ditto
> + pr_warning("Failure on setting DSCP priority.");
> + }
> +
> fda->fd[FD_EVENT] = efd;
> fda->fd[FD_GENERAL] = gfd;
> return 0;
> diff -rwbBu a/udp.c b/udp.c
> --- a/udp.c 2015-09-19 16:25:11.000000000 +0200
> +++ b/udp.c 2016-07-06 16:39:27.000000000 +0200
> @@ -186,6 +186,10 @@
> if (sk_general_init(gfd))
> goto no_timestamping;
>
> + if(sk_set_priority(t->cfg, efd, gfd) < 0) {
ditto
> + pr_warning("Failure on setting DSCP priority.");
> + }
> +
> fda->fd[FD_EVENT] = efd;
> fda->fd[FD_GENERAL] = gfd;
> return 0;
Thanks,
Richard
------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
