patchlwIP - A Lightweight TCP/IP stack - Patches: patch #9211, mqtt: Check Will Flag rather than...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #9211: mqtt: Check Will Flag rather than will_topic_len for appending will topic and message

Submitter:  Axel Lin <axellin>
Submitted:  Sat 07 Jan 2017 03:57:29 AM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Open/Closed:  Closed
Planned Release:  None

Jump to the original submission

Sun 08 Jan 2017 06:10:36 PM UTC, comment #7: 

Fixed it using your patch, because to be really precise I would have to duplicate the "(client_info->will_topic != NULL && client_info->will_msg != NULL)" check to append the WILL message, and your suggestion is more readable than that

Dirk Ziegelmeier <dziegel>
Group administrator
Sun 08 Jan 2017 10:16:34 AM UTC, comment #6: 

Hi Dirk,
I'm fine you just update the code.

Thanks,
Axel

Axel Lin <axellin>
Group Member
Sun 08 Jan 2017 08:43:17 AM UTC, comment #5: 

In this case, I'd prefer a commit message that describes the bug it fixes, instead of a message that suggests a cleanup.

Also, the problem is that from the MQTT spec, the TOPIC length seems to be required > 0. The current code does not check that. I think an assert should be in place for that. The WILL message can be of length 0, so one can either check the WILL flag as you did or rewrite the same check as above, "client_info->will_msg != NULL" which I'd prefer (for consistency with the code above and it compiles to two instructions less in assembly code ;-) ).

I can do the changes if you like, but since you discovered it, you deserve the credit.

Dirk Ziegelmeier <dziegel>
Group administrator
Sun 08 Jan 2017 02:28:15 AM UTC, comment #4: 

The patch is not to change the coding style.
There is a small difference when will topic is zero-length rather than NULL.
If will topic is zero-length, the Will flag bit is set.
But current code won't send will topic/message in the payload.
i.e. the payload length field does not match the exactly payload sent.

Axel Lin <axellin>
Group Member
Sat 07 Jan 2017 07:34:06 PM UTC, comment #3: 

OK, you are right there.

But then your patch is just a change in coding style. Erik Andersen wrote "will_topic_len > 0" which is not slower or less readable than "flags & MQTT_CONNECT_FLAG_WILL". I don't want to apply patches that just change one person's coding style to the one of another person. A patch should be a real improvement in readability (less #ifdefs, simplify complex code etc.) or fix bugs.

Dirk Ziegelmeier <dziegel>
Group administrator
Sat 07 Jan 2017 01:14:02 PM UTC, comment #2: 

Hi Dirk,
Currently the WILL flag is not set by the caller.
It's set if (client_info->will_topic != NULL && client_info->will_msg != NULL).
So I think it's fine apply this patch as is.

Axel

Axel Lin <axellin>
Group Member
Sat 07 Jan 2017 08:04:06 AM UTC, comment #1: 

Thats correct, but I'd prefer to add an LWIP_ASSERT that the WILL flag is set, because it is a user error to supply a will message without setting the will flag.
Do you think so too, Axel?

Dirk Ziegelmeier <dziegel>
Group administrator
Sat 07 Jan 2017 03:57:29 AM UTC, original submission:  

The spec says:

3.1.3.2 Will Topic
If the Will Flag is set to 1, the Will Topic is the next field in the payload.

3.1.3.3 Will Message
If the Will Flag is set to 1 the Will Message is the next field in the payload.

Axel Lin <axellin>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dziegel (Posted a comment)
  • -email is unavailable- added by axellin (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-01-08 dziegel StatusNone Done
        Open/ClosedOpen Closed
    2017-01-08 dziegel StatusWont Do None
        Open/ClosedClosed Open
    2017-01-07 dziegel StatusNone Wont Do
        Open/ClosedOpen Closed
    2017-01-07 axellin Attached File- Added 0001-mqtt-Check-Will-Flag-rather-than-will_topic_len-for-.patch, #39393

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code