Re: [Linuxptp-devel] [PATCH v6 05/11] tlv: Encode and decode alternate time offset indicator TLVs.

[Hal Murray]

richardcoch...@gmail.com said:
>> +NTOHS(atoi->type);
>> +NTOHS(atoi->length);
> These two lines are wrong.  The net/host conversion of type and length
> already happened in suffix_post_recv() in msg.c 

They may be wrong in the sense of duplicating work that has already been done, 
but they are also wrong in the sense of not doing anything.

NTOHS doesn't update it's argument.  It returns a result which isn't being 
stored anyplace.


-- 
These are my opinions.  I hate spam.





___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

Re: [Linuxptp-devel] [PATCH v6 05/11] tlv: Encode and decode alternate time offset indicator TLVs.

[Richard Cochran]

On Mon, Feb 20, 2023 at 12:57:53PM -0800, Richard Cochran wrote:

> +static int alttime_offset_post_recv(struct tlv_extra *extra)
> +{
> + struct TLV *tlv = extra->tlv;
> + struct alternate_time_offset_indicator_tlv *atoi =
> + (struct alternate_time_offset_indicator_tlv *) tlv;
> +
> + if (tlv->length < sizeof(struct alternate_time_offset_indicator_tlv) +
> + atoi->displayName.length - sizeof(struct TLV)) {
> + return -EBADMSG;
> + }
> +
> + NTOHS(atoi->type);
> + NTOHS(atoi->length);

These two lines are wrong.  The net/host conversion of type and length
already happened in suffix_post_recv() in msg.c

> + /* Message alignment broken by design. */
> + net2host32_unaligned(>currentOffset);
> + net2host32_unaligned(>jumpSeconds);
> + flip16(>timeOfNextJump.seconds_msb);
> + net2host32_unaligned(>timeOfNextJump.seconds_lsb);
> +
> + return 0;
> +}
> +
> +static void alttime_offset_pre_send(struct tlv_extra *extra)
> +{
> + struct alternate_time_offset_indicator_tlv *atoi;
> +
> + atoi = (struct alternate_time_offset_indicator_tlv *) extra->tlv;
> +
> + HTONS(atoi->type);
> + HTONS(atoi->length);

These two are also wrong.  The host/net conversion happens in
suffix_pre_send().

> + /* Message alignment broken by design. */
> + host2net32_unaligned(>currentOffset);
> + host2net32_unaligned(>jumpSeconds);
> + flip16(>timeOfNextJump.seconds_msb);
> + host2net32_unaligned(>timeOfNextJump.seconds_lsb);
> +}
> +

I took the liberty of fixing these bugs while merging this series.

With the correction of removing those four lines, Wireshark can decode
the ALTERNATE_TIME_OFFSET_INDICATOR TLV, eg:

Alternate time offset indicator TLV
tlvType: Alternate time offset indicator (9)
lengthField: 22
keyField: 0
currentOffset: 3563
jumpSeconds: -1679796001
timeOfNextJump: 641f9910
displayName: Vienna
length: 6
displayName: Vienna


Thanks,
Richard




___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel