On Wed, Jul 06, 2016 at 01:32:26PM +0000, Jesuiter, Henry (ALC NetworX GmbH)
wrote:
> Oh yes, my fault. Please find it below:
>
> ---
> Index: config.c
> ===================================================================
> --- config.c (Revision 827)
> +++ config.c (Revision 830)
> @@ -236,6 +236,8 @@
> GLOB_ITEM_INT("use_syslog", 1, 0, 1),
> GLOB_ITEM_STR("userDescription", ""),
> GLOB_ITEM_INT("verbose", 0, 0, 1),
> + GLOB_ITEM_INT("tosEventMessage", 56, 0, 63),
> + GLOB_ITEM_INT("tosGeneralMessage", 46, 0, 46)
Default should be zero.
Since this is a bit field (isn't it?), max would make more sense as
hexadecimal value.
Keep the table in alphabetical order please.
The options should be all lower case, like 'tos_event'. (We only use
lowerCamelCase when the option comes from a published standard.)
> };
>
> static enum parser_result
> Index: udp.c
> ===================================================================
> --- udp.c (Revision 827)
> +++ udp.c (Revision 830)
> @@ -149,6 +149,29 @@
> return -1;
> }
>
> +// setting IP DSCP value here
> +static int set_priority(int sock_fd, uint8_t dscp) {
No C++ comments, K&R style. Please check CODING_STYLE.org.
> + int tos;
> + socklen_t tos_len;
> +
> + if (!sock_fd) {
This test is useless, because the caller ensures that the FD is valid.
> + return 0;
> + }
> +
> + tos_len = sizeof(tos);
> + if (getsockopt(sock_fd, SOL_IP, IP_TOS, &tos, &tos_len) < 0) {
> + tos = 0;
> + }
> +
> + tos |= dscp<<2;
You need to clear the bit field first.
> + tos_len = sizeof(tos);
> + if (setsockopt(sock_fd, SOL_IP, IP_TOS, &tos, tos_len) < 0) {
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> enum { MC_PRIMARY, MC_PDELAY };
>
> static struct in_addr mcast_addr[2];
> @@ -176,11 +199,23 @@
> if (efd < 0)
> goto no_event;
>
> + // set DSCP priority for event messages
> + uint8_t event_dscp = config_get_int(t->cfg, NULL, "tosEventMessage");
> + if(set_priority(efd, event_dscp) < 0) {
^
Coding style.
Only call set_priority if dscp != 0.
Both of these calls should happen *after* sk_general_init. (It not
wrong as is, but it would fit better to the existing code which does
first opens the sockets, then configures them.)
> + pr_warning("Could not set COS value for PTP event messages.");
> + }
> +
> gfd = open_socket(name, mcast_addr, GENERAL_PORT, ttl);
> if (gfd < 0)
> goto no_general;
>
> - if (sk_timestamping_init(efd, name, ts_type, TRANS_UDP_IPV4))
> + // set DSCP priority for general messages
> + uint8_t general_dscp = config_get_int(t->cfg, NULL,
> "tosGeneralMessage");
> + if(set_priority(gfd, general_dscp) < 0) {
> + pr_warning("Could not set DHCP value for PTP general
> messages.");
DSCP.
> + }
> +
> + if (sk_timestamping_init(efd, name, ts_type, TRANS_UDP_IPV4))
> goto no_timestamping;
>
> if (sk_general_init(gfd))
>
> Index: udp6.c
> ===================================================================
> --- udp6.c (Revision 827)
> +++ udp6.c (Revision 830)
> @@ -157,6 +157,29 @@
> return -1;
> }
>
> +// setting IP DSCP value here
> +static int set_priority(int sock_fd, uint8_t dscp) {
This function is a duplicate of the one above, so it should go into
sk.c with the other shared code.
> + int tos;
> + socklen_t tos_len;
> +
> + if (!sock_fd) {
> + return 0;
> + }
> +
> + tos_len = sizeof(tos);
> + if (getsockopt(sock_fd, SOL_IP, IP_TOS, &tos, &tos_len) < 0) {
> + tos = 0;
> + }
> +
> + tos |= dscp<<2;
> + tos_len = sizeof(tos);
> + if (setsockopt(sock_fd, SOL_IP, IP_TOS, &tos, tos_len) < 0) {
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> enum { MC_PRIMARY, MC_PDELAY };
>
> static struct in6_addr mc6_addr[2];
> @@ -186,11 +209,23 @@
> if (efd < 0)
> goto no_event;
>
> + // set DSCP priority for event messages
> + uint8_t event_dscp = config_get_int(t->cfg, NULL, "tosEventMessage");
> + if(set_priority(efd, event_dscp) < 0) {
> + pr_warning("Could not set COS value for PTP event messages.");
> + }
> +
> gfd = open_socket_ipv6(name, mc6_addr, GENERAL_PORT, &udp6->index,
> hop_limit);
> if (gfd < 0)
> goto no_general;
>
> - if (sk_timestamping_init(efd, name, ts_type, TRANS_UDP_IPV6))
> + // set DSCP priority for general messages
> + uint8_t general_dscp = config_get_int(t->cfg, NULL,
> "tosGeneralMessage");
> + if(set_priority(gfd, general_dscp) < 0) {
> + pr_warning("Could not set DHCP value for PTP general
> messages.");
> + }
> +
> + if (sk_timestamping_init(efd, name, ts_type, TRANS_UDP_IPV6))
> goto no_timestamping;
>
> if (sk_general_init(gfd))
This would look better if you had a helper in sk.c:
int sk_set_priority(int efd, int gfd);
Then you would have in udp.c/udp6.c:
if (sk_set_priority(efd, gfd)) {
pr_warning("Failed to set the DSCP priority.");
}
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
