Skip to content

Conversation

@AniruddhaKanhere
Copy link
Member

Description

Test Steps

Checklist:

  • I have tested my changes. No regression in existing tests.
  • I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@AniruddhaKanhere AniruddhaKanhere force-pushed the AddPublish branch 2 times, most recently from e942ad7 to 49fbfc7 Compare December 13, 2025 00:30
LogDebug( ( "Packet identifier %hu.",
( unsigned short ) *pPacketIdentifier ) );

/* Packet identifier cannot be 0. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should check this right after L727?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that L727 would be the better place. However, that breaks the pattern that we increment the index after we get a value out from the buffer. This will not cause any issues since as long as the return value of the function is not MQTTSuccess, any and all values returned by the function in the parameters are invalid and should not be consumed.

pLocalIndex = &pLocalIndex[ variableLengthEncodedSize( propertyLength ) ];

/*Validate the remaining length.*/
if( remainingLength != ( propertyLength + variableLengthEncodedSize( propertyLength ) + 3U ) )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this check. What does remainingLength represent here? And why would it equal the length of the properties (encoded + decoded)? Why isn't this equal to variableLengthEncodedSize( propertyLength ) + 3U?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is an artifact of how the code was written earlier.

I reworked it. Let me know if that makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants