Thanks for taking the time to review the patch!
> > +static int has_prp_trailer(unsigned char *ptr, int cnt) {
> > + if (cnt < PRP_TRAILER_LEN)
> > + return -1;
> > + //verify suffix first since it is the best identifier
>
> Please use C-style comments.
>
Will do
> > + unsigned short suffix_id = ntohs(*(unsigned short*)(ptr + (cnt -
> > + 2)));
>
> This seems to be parsing the frame from the end. Couldn't that randomly
> match a non-PRP frame, even if you consider the length check below?
>
> To me it would make more sense to start after the PTP message according to
> the messageLength field.
>
This is a good point.
PRP supports also pure ethernet frames but in case of PTP over ethernet we
actually have the messageLength.
I will need to see if I can get it in a good way already in the raw receive
(seems like only the ethernet header is currently parsed)
> > + if (suffix_id != ETH_P_PRP)
> > + return -1;
> > +
> > + // size should also be verified
> > + unsigned short lane_size_field = ntohs(*(unsigned short*)(ptr +
> > + (cnt - 4)));
>
> Declarations should be at the beginning of the function.
>
Ok, impressive that you still support such old c-style, thanks for pointing it
out.
> > + // size is lower 12 bits
> > + unsigned short lsdu_size = (lane_size_field & 0x0FFF);
> > + if (lsdu_size == cnt)
> > + return 0;
> > +
> > + return -1;
> > +}
> > +
>
- Magnus Armholt
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
