On Thu, Jul 14, 2022 at 11:45:04AM +0200, Miroslav Lichvar wrote:
> On Thu, Jul 14, 2022 at 12:24:39PM +0300, Magnus Armholt wrote:
> > Strip the IEC62439-3 PRP trailer if it is present
> > to support PTP over PRP.
> > The implementation is very pedantic about
> > trailing bytes and will indicate bad message if the
> > PRP trailer bytes are present when parsing the PTP message.
>
> > +static int has_prp_trailer(unsigned char *ptr, int cnt, int eth_hlen, int
> > *trailer_len)
> > +{
> > + unsigned short suffix_id, lane_size_field, lsdu_size;
> > + struct ptp_header* hdr;
> > + int ptp_msg_len, trailer_start, padding_len;
>
> Just some minor coding style issues. Please order the declarations by
> length (reverse christmas tree) and write hdr as "*hdr" instead of
> "* hdr".
Magnus, please incorporate this and the following feedback and post a v4.
Thanks,
Richard
>
> > + /* try to parse like a PTP message to find out the message length */
> > + if (cnt < sizeof(struct ptp_header))
> > + return 0;
> > +
> > + hdr = (struct ptp_header*)ptr;
>
> "ptp_header *".
>
> > + lane_size_field = ntohs(*(unsigned short*)(ptr + (trailer_start + 2)));
>
> Unnecessary parenthesis around "trailer_start + 2".
>
> > + lsdu_size = (lane_size_field & 0x0FFF);
>
> Same here.
>
> > + if (lsdu_size != cnt)
> > + return 0;
> > +
> > + /* Verify the suffix */
> > + suffix_id = ntohs(*(unsigned short*)(ptr + (trailer_start + 4)));
>
> And here.
>
> > + if (suffix_id == ETH_P_PRP)
> > + {
> > + *trailer_len = PRP_TRAILER_LEN + padding_len;
> > + return 1;
>
> Is it necessary to return the padding length here? If I understand it
> correctly it's not specific to PRP. It's just the original Ethernet
> minimum length of 64 plus 6 bytes for the trailer. If you remove the
> padding_len variable, the function can be simplified a bit.
>
> --
> Miroslav Lichvar
>
>
>
> _______________________________________________
> Linuxptp-devel mailing list
> Linuxptp-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
