Re: [Linuxptp-devel] [PATCH RFC V2 08/11] port: Implement the NetSync Monitor protocol.

[Richard Cochran]

On Thu, Mar 08, 2018 at 08:07:26AM +, Anders Selhammer wrote:
> I reviewed it in outlook, but now when I open it in windows 10 built in 
> client, it all look as it should.

I strongly suggest getting a different email client.  If your MTA is
exchange, then you will have to create an outside email account,
because exchange cannot send plain text messages correctly.

See:

linux/Documentation/process/email-clients.rst

(Actually Gmail does work if you use the IMAP service. It is the web
interface that does not work for sending patches.)
 
> Is it possible to add the link to the patch in the mail archive in the end of 
> the mail also?

No, simply because the archive URL is not known at the time of
transmission.
 
Thanks,
Richard

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

Re: [Linuxptp-devel] [PATCH RFC V2 08/11] port: Implement the NetSync Monitor protocol.

[Anders Selhammer]

>I appreciate your taking the effort to review the series.  However, in the 
>future, please do not top-post!
>Instead, comment directly in-line, and trim the reply to remove any patch 
>hunks that are not being commented upon.

Ok, I will comment inline in the future, sorry about that.

>> +static void extract_address(struct ptp_message *m, struct PortAddress
>> +*paddr) {
>> static int msg_current(struct ptp_message *m, struct timespec now)  {
>> static struct follow_up_info_tlv *follow_up_info_extract(struct
>> ptp_message *m)  {
>> +static int port_nsm_reply(struct port *p, struct ptp_message *m) {
>> static int process_delay_req(struct port *p, struct ptp_message *m)  {

>^^^ no idea where you got this from.

I got them all from PATCH RFC V2 08/11 sent to me by mail, just did copy and 
pasted on the top. If there is something that is mismatched with your 
implementation I cannot say.
Normally this probably have slipped since I´ve seen a mixture of the two 
implementations, with or without linebreak, but since you last week sent a 
patch with these coding style updates, I spotted it.

>> no line break on this:
>> +memcpy(>foot->lastsync, _sync,
>> +sizeof(extra->foot->lastsync));

Above lines in a single line would be shorter than some other lines above. 
Easier to read without line break.


Btw, I was just looking at this Netsync Monitoring myself and was surprised 
that you, the same week, implemented it. Got a question from Meinberg to 
implement it if I could/had time and I was planning to start the coding this 
week
I think the solution overall was nicely implemented. I will have a closer look 
at it when I can have all the changes infront of me and a can switch between 
all the files. I think that’s easier.

Sorry if there were some incorrect comments.

//
Anders
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

Re: [Linuxptp-devel] [PATCH RFC V2 08/11] port: Implement the NetSync Monitor protocol.

[Richard Cochran]

On Mon, Mar 05, 2018 at 12:41:56PM +, Anders Selhammer wrote:
> We are programming C, not Java, and so opening braces of a function belong on 
> a line all by themselves.

And they all are.  Where is the problem?

> +static void extract_address(struct ptp_message *m, struct PortAddress 
> *paddr) {
> static int msg_current(struct ptp_message *m, struct timespec now)  {
> static struct follow_up_info_tlv *follow_up_info_extract(struct ptp_message 
> *m)  {
> +static int port_nsm_reply(struct port *p, struct ptp_message *m) {
> static int process_delay_req(struct port *p, struct ptp_message *m)  {

^^^ no idea where you got this from.
 
> no line break on this:
> + memcpy(>foot->lastsync, _sync, 
> +sizeof(extra->foot->lastsync));

I appreciate your taking the effort to review the series.  However, in
the future, please do not top-post!

Instead, comment directly in-line, and trim the reply to remove any
patch hunks that are not being commented upon.

Thanks,
Richard

--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

Re: [Linuxptp-devel] [PATCH RFC V2 08/11] port: Implement the NetSync Monitor protocol.

[Anders Selhammer]

We are programming C, not Java, and so opening braces of a function belong on a 
line all by themselves. 
+static void extract_address(struct ptp_message *m, struct PortAddress *paddr) {
static int msg_current(struct ptp_message *m, struct timespec now)  {
static struct follow_up_info_tlv *follow_up_info_extract(struct ptp_message *m) 
 {
+static int port_nsm_reply(struct port *p, struct ptp_message *m) {
static int process_delay_req(struct port *p, struct ptp_message *m)  {

no line break on this:
+   memcpy(>foot->lastsync, _sync, 
+sizeof(extra->foot->lastsync));



-Ursprungligt meddelande-
Från: Richard Cochran [mailto:richardcoch...@gmail.com] 
Skickat: den 1 mars 2018 20:41
Till: linuxptp-devel@lists.sourceforge.net
Ämne: [Linuxptp-devel] [PATCH RFC V2 08/11] port: Implement the NetSync Monitor 
protocol.

When NSM is enabled on a given port, that port always replies to a NSM delay 
request with a delay response, sync, and follow up, regardless of the current 
state of the port.

Signed-off-by: Richard Cochran <richardcoch...@gmail.com>
---
 port.c | 132 +++--
 1 file changed, 128 insertions(+), 4 deletions(-)

diff --git a/port.c b/port.c
index cc5a09e..52b9282 100644
--- a/port.c
+++ b/port.c
@@ -126,6 +126,7 @@ struct port {
int freq_est_interval;
int hybrid_e2e;
int min_neighbor_prop_delay;
+   int net_sync_monitor;
int path_trace_enabled;
int rx_timestamp_offset;
int tx_timestamp_offset;
@@ -184,6 +185,29 @@ static int clear_fault_asap(struct fault_interval *faint)
return 0;
 }
 
+static void extract_address(struct ptp_message *m, struct PortAddress 
+*paddr) {
+   int len = 0;
+
+   switch (paddr->networkProtocol) {
+   case TRANS_UDP_IPV4:
+   len = sizeof(m->address.sin.sin_addr.s_addr);
+   memcpy(paddr->address, >address.sin.sin_addr.s_addr, len);
+   break;
+   case TRANS_UDP_IPV6:
+   len = sizeof(m->address.sin6.sin6_addr.s6_addr);
+   memcpy(paddr->address, >address.sin6.sin6_addr.s6_addr, len);
+   break;
+   case TRANS_IEEE_802_3:
+   len = MAC_LEN;
+   memcpy(paddr->address, >address.sll.sll_addr, len);
+   break;
+   default:
+   return;
+   }
+   paddr->addressLength = len;
+}
+
 static int msg_current(struct ptp_message *m, struct timespec now)  {
int64_t t1, t2, tmo;
@@ -449,6 +473,67 @@ static int follow_up_info_append(struct port *p, struct 
ptp_message *m)
return 0;
 }
 
+static int net_sync_resp_append(struct port *p, struct ptp_message *m) 
+{
+   struct timePropertiesDS *tp = clock_time_properties(p->clock);
+   struct ClockIdentity cid = clock_identity(p->clock), pid;
+   struct currentDS *cds = clock_current_dataset(p->clock);
+   struct parent_ds *dad = clock_parent_ds(p->clock);
+   struct port *best = clock_best_port(p->clock);
+   struct nsm_resp_tlv_head *head;
+   struct Timestamp last_sync;
+   struct PortAddress paddr;
+   struct ptp_message *tmp;
+   struct tlv_extra *extra;
+   unsigned char *ptr;
+   int tlv_len;
+
+   last_sync = tmv_to_Timestamp(clock_ingress_time(p->clock));
+   pid = dad->pds.parentPortIdentity.clockIdentity;
+
+   if (best && memcmp(, , sizeof(cid))) {
+   /* Extract the parent's protocol address. */
+   paddr.networkProtocol = transport_type(best->trp);
+   paddr.addressLength =
+   transport_protocol_addr(best->trp, paddr.address);
+   if (best->best) {
+   tmp = TAILQ_FIRST(>best->messages);
+   extract_address(tmp, );
+   }
+   } else {
+   /* We are our own parent. */
+   paddr.networkProtocol = transport_type(p->trp);
+   paddr.addressLength =
+   transport_protocol_addr(p->trp, paddr.address);
+   }
+
+   tlv_len = sizeof(*head) + sizeof(*extra->foot) + paddr.addressLength;
+
+   extra = msg_tlv_append(m, tlv_len);
+   if (!extra) {
+   return -1;
+   }
+
+   head = (struct nsm_resp_tlv_head *) extra->tlv;
+   head->type = TLV_PTPMON_RESP;
+   head->length = tlv_len - sizeof(head->type) - sizeof(head->length);
+   head->port_state = p->state == PS_GRAND_MASTER ? PS_MASTER : p->state;
+   head->parent_addr.networkProtocol = paddr.networkProtocol;
+   head->parent_addr.addressLength = paddr.addressLength;
+   memcpy(head->parent_addr.address, paddr.address, paddr.addressLength);
+
+   pt

[Linuxptp-devel] [PATCH RFC V2 08/11] port: Implement the NetSync Monitor protocol.

[Richard Cochran]

When NSM is enabled on a given port, that port always replies to a NSM
delay request with a delay response, sync, and follow up, regardless
of the current state of the port.

Signed-off-by: Richard Cochran 
---
 port.c | 132 +++--
 1 file changed, 128 insertions(+), 4 deletions(-)

diff --git a/port.c b/port.c
index cc5a09e..52b9282 100644
--- a/port.c
+++ b/port.c
@@ -126,6 +126,7 @@ struct port {
int freq_est_interval;
int hybrid_e2e;
int min_neighbor_prop_delay;
+   int net_sync_monitor;
int path_trace_enabled;
int rx_timestamp_offset;
int tx_timestamp_offset;
@@ -184,6 +185,29 @@ static int clear_fault_asap(struct fault_interval *faint)
return 0;
 }
 
+static void extract_address(struct ptp_message *m, struct PortAddress *paddr)
+{
+   int len = 0;
+
+   switch (paddr->networkProtocol) {
+   case TRANS_UDP_IPV4:
+   len = sizeof(m->address.sin.sin_addr.s_addr);
+   memcpy(paddr->address, >address.sin.sin_addr.s_addr, len);
+   break;
+   case TRANS_UDP_IPV6:
+   len = sizeof(m->address.sin6.sin6_addr.s6_addr);
+   memcpy(paddr->address, >address.sin6.sin6_addr.s6_addr, len);
+   break;
+   case TRANS_IEEE_802_3:
+   len = MAC_LEN;
+   memcpy(paddr->address, >address.sll.sll_addr, len);
+   break;
+   default:
+   return;
+   }
+   paddr->addressLength = len;
+}
+
 static int msg_current(struct ptp_message *m, struct timespec now)
 {
int64_t t1, t2, tmo;
@@ -449,6 +473,67 @@ static int follow_up_info_append(struct port *p, struct 
ptp_message *m)
return 0;
 }
 
+static int net_sync_resp_append(struct port *p, struct ptp_message *m)
+{
+   struct timePropertiesDS *tp = clock_time_properties(p->clock);
+   struct ClockIdentity cid = clock_identity(p->clock), pid;
+   struct currentDS *cds = clock_current_dataset(p->clock);
+   struct parent_ds *dad = clock_parent_ds(p->clock);
+   struct port *best = clock_best_port(p->clock);
+   struct nsm_resp_tlv_head *head;
+   struct Timestamp last_sync;
+   struct PortAddress paddr;
+   struct ptp_message *tmp;
+   struct tlv_extra *extra;
+   unsigned char *ptr;
+   int tlv_len;
+
+   last_sync = tmv_to_Timestamp(clock_ingress_time(p->clock));
+   pid = dad->pds.parentPortIdentity.clockIdentity;
+
+   if (best && memcmp(, , sizeof(cid))) {
+   /* Extract the parent's protocol address. */
+   paddr.networkProtocol = transport_type(best->trp);
+   paddr.addressLength =
+   transport_protocol_addr(best->trp, paddr.address);
+   if (best->best) {
+   tmp = TAILQ_FIRST(>best->messages);
+   extract_address(tmp, );
+   }
+   } else {
+   /* We are our own parent. */
+   paddr.networkProtocol = transport_type(p->trp);
+   paddr.addressLength =
+   transport_protocol_addr(p->trp, paddr.address);
+   }
+
+   tlv_len = sizeof(*head) + sizeof(*extra->foot) + paddr.addressLength;
+
+   extra = msg_tlv_append(m, tlv_len);
+   if (!extra) {
+   return -1;
+   }
+
+   head = (struct nsm_resp_tlv_head *) extra->tlv;
+   head->type = TLV_PTPMON_RESP;
+   head->length = tlv_len - sizeof(head->type) - sizeof(head->length);
+   head->port_state = p->state == PS_GRAND_MASTER ? PS_MASTER : p->state;
+   head->parent_addr.networkProtocol = paddr.networkProtocol;
+   head->parent_addr.addressLength = paddr.addressLength;
+   memcpy(head->parent_addr.address, paddr.address, paddr.addressLength);
+
+   ptr = (unsigned char *) head;
+   ptr += sizeof(*head) + paddr.addressLength;
+   extra->foot = (struct nsm_resp_tlv_foot *) ptr;
+
+   memcpy(>foot->parent, >pds, sizeof(extra->foot->parent));
+   memcpy(>foot->current, cds, sizeof(extra->foot->current));
+   memcpy(>foot->timeprop, tp, sizeof(extra->foot->timeprop));
+   memcpy(>foot->lastsync, _sync, 
sizeof(extra->foot->lastsync));
+
+   return 0;
+}
+
 static struct follow_up_info_tlv *follow_up_info_extract(struct ptp_message *m)
 {
struct follow_up_info_tlv *f;
@@ -677,6 +762,27 @@ static int port_ignore(struct port *p, struct ptp_message 
*m)
return 0;
 }
 
+static int port_nsm_reply(struct port *p, struct ptp_message *m)
+{
+   struct tlv_extra *extra;
+
+   if (!p->net_sync_monitor) {
+   return 0;
+   }
+   if (!p->hybrid_e2e) {
+   return 0;
+   }
+   if (!(m->header.flagField[0] & UNICAST)) {
+   return 0;
+