Add MQTT v5 support #1709

feymartynov · 2019-07-24T13:51:41Z

No description provided.

lminiero · 2019-07-24T13:56:59Z

Thanks! I won't be able to review until I get back home (currently abroad for a conference for the whole week), but from a quick glance you definitely broke the indentation of the configuration file. Please fix that.

lminiero · 2019-08-01T09:14:52Z

@feymartynov ping on the config file fix 😉

On the rest of the patch, I don't think it's ok the way it is now. You updated the README to say that only v1.1 of the library is required, and that v1.3 is only needed if you want MQTTv5. Anyway, the code uses the *5 version of methods and structures (e.g., MQTTAsync_successData5) no matter what, which means that if someone has the v1.1 installed, compilation will fail because of the unrecognized structures.

This means the patch should be updated to:

either mandate support for v1.3 of the library (but I don't know how widespread that is, and it would break retrocompatibility to those with an older version of the library)
or add a new condition that you can #ifdef on to see if you can use the new stuff (but that will make the code "ugly", since it would be in a lot of places).

I personally don't want to break retrocompatibility (we've tried to ensure that for other transports too across the years), so I'd go for the second option, unless there's a strong and compelling point on 1.3 being pretty much universally deployed and available. Pinging @oej as well since AFAIK he's still using the MQTT code.

feymartynov · 2019-08-01T11:34:11Z

You're right. I would also like to save back compatibility so I'm going to add missing #ifdefs and try to build it with Paho 1.1.

feymartynov · 2019-08-01T23:25:39Z

@lminiero updated the PR.
Tested it with both Paho 1.1.0 (MQTT 3.1.1) and Paho 1.3.0 (MQTT 3.1.1 & 5).

lminiero

Thanks! Added some notes, mostly on indentation/code style, and a small nit on a memory leak. Apart from that, looks good to me! As soon as you update the code, I'll make some tests and in case merge.

lminiero · 2019-08-02T09:20:15Z

conf/janus.transport.mqtt.jcfg.sample

@@ -7,6 +7,7 @@ general: {
 url = "tcp:https://localhost:1883" # The connection URL of the MQTT broker: if you want
 # to use SSL, make sure you type ssl:https:// instead of tcp:https://,
 # and that you configure the SSL settings below
+ #mqtt_version = "3.1.1" # Protocol version. Available values: 3.1, 3.1.1 (default), 5.


Indentation still broken here... please use tabs as per the guidelines.

lminiero · 2019-08-02T09:20:39Z

transports/janus_mqtt.c

@@ -157,27 +162,95 @@ int janus_mqtt_client_message_arrived(void *context, char *topicName, int topicL
 void janus_mqtt_client_delivery_complete(void *context, MQTTAsync_token token);
 int janus_mqtt_client_connect(janus_mqtt_context *ctx);
 void janus_mqtt_client_connect_success(void *context, MQTTAsync_successData *response);
+#ifdef MQTTVERSION_5
+ void janus_mqtt_client_connect_success5(void *context, MQTTAsync_successData5 *response);


No need to indent here, it's a function declaration as the others; same for the others below.
Thinking about it, it might make sense to group all the v5 declarations together in a single #ifdef? Would make everything more readable IMO.

lminiero · 2019-08-02T09:24:04Z

transports/janus_mqtt.c

@@ -306,6 +379,25 @@ int janus_mqtt_init(janus_transport_callbacks *callback, const char *config_path
 }

 /* Connect configuration */
+ janus_config_item *mqtt_version = janus_config_get(config, config_general, janus_config_type_item, "mqtt_version");
+ const char *mqtt_version_str = (mqtt_version && mqtt_version->value) ? g_strdup(mqtt_version->value) : JANUS_MQTT_VERSION_DEFAULT;


No need to g_strdup here, you're not keeping mqtt_version_str: it's only declared and used here. The way it is now you're introducing a memory leak.

lminiero · 2019-08-02T09:26:08Z

transports/janus_mqtt.c

+ #ifdef MQTTVERSION_5
+ if (ctx->connect.mqtt_version == MQTTVERSION_5) {
+ create_options.MQTTVersion = MQTTVERSION_5;
+ }


Indentation is broken. The way the code is structured in the rest of the repo, the #ifdef, #endif, etc. are all shifted to the left (no indentation), while what they contain should have the same indentation as if they were not wrapped in there. I won't write it under all of the changes, so please update this wherever this occurs.

feymartynov · 2019-08-02T12:48:43Z

Updated the PR.

lminiero · 2019-08-02T12:53:08Z

Thanks a lot for the very quick fixes! I'll give it a test later to try and merge before I leave 👍

lminiero

Did a quick test and found these last few nits: for some reason I had both an older and a recent version of Paho-C installed, and it was picking the old one, so trying v5 failed initially, which showed the error message with no endline.

Getting rid of that, I verified that it seems to be working as expected, thanks! As soon as you can fix those last few remaining nits I'll merge 👍

lminiero · 2019-08-02T14:26:05Z

conf/janus.transport.mqtt.jcfg.sample

@@ -7,6 +7,7 @@ general: {
 url = "tcp:https://localhost:1883" # The connection URL of the MQTT broker: if you want
 # to use SSL, make sure you type ssl:https:// instead of tcp:https://,
 # and that you configure the SSL settings below
+ #mqtt_version = "3.1.1" # Protocol version. Available values: 3.1, 3.1.1 (default), 5.


It looks like it's still spaces between the value and the comment. Can you fix that too?

lminiero · 2019-08-02T14:26:23Z

transports/janus_mqtt.c

+ #ifdef MQTTVERSION_5
+ ctx->connect.mqtt_version = MQTTVERSION_5;
+ #else
+ JANUS_LOG(LOG_FATAL, "Using MQTT v5 requires compilation with Paho >= 1.3.0");


There's a \n missing here.

lminiero · 2019-08-02T14:27:09Z

transports/janus_mqtt.c

+ } else if(strcmp(mqtt_version_str, JANUS_MQTT_VERSION_3_1_1) == 0) {
+ ctx->connect.mqtt_version = MQTTVERSION_3_1_1;
+ } else if(strcmp(mqtt_version_str, JANUS_MQTT_VERSION_5) == 0) {
+ #ifdef MQTTVERSION_5


This block of #ifdef #else #endif still has a broken indentation.

Why? #ifdef #else #endif are aligned one tab left and the code itself is aligned like it would be without these directives.

feymartynov · 2019-08-02T18:04:46Z

Updated the PR.

lminiero · 2019-08-02T18:11:33Z

Thanks, merging 👍

feymartynov force-pushed the mqttv5 branch from 90e0027 to fb9300f Compare August 1, 2019 23:20

lminiero reviewed Aug 2, 2019

View reviewed changes

feymartynov force-pushed the mqttv5 branch 2 times, most recently from af86eb5 to a00a462 Compare August 2, 2019 12:46

lminiero reviewed Aug 2, 2019

View reviewed changes

Add MQTT v5 support

58dd92c

feymartynov force-pushed the mqttv5 branch from a00a462 to 58dd92c Compare August 2, 2019 18:04

lminiero merged commit 506d970 into meetecho:master Aug 2, 2019

lminiero added a commit that referenced this pull request Aug 2, 2019

Fixed directives indentation to match code style (see #1709)

f60b0ae

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MQTT v5 support #1709

Add MQTT v5 support #1709

feymartynov commented Jul 24, 2019

lminiero commented Jul 24, 2019 •

edited

Loading

lminiero commented Aug 1, 2019

feymartynov commented Aug 1, 2019

feymartynov commented Aug 1, 2019

lminiero left a comment

lminiero Aug 2, 2019

lminiero Aug 2, 2019

lminiero Aug 2, 2019

lminiero Aug 2, 2019

feymartynov commented Aug 2, 2019

lminiero commented Aug 2, 2019

lminiero left a comment

lminiero Aug 2, 2019

lminiero Aug 2, 2019

lminiero Aug 2, 2019

feymartynov Aug 2, 2019

feymartynov commented Aug 2, 2019

lminiero commented Aug 2, 2019

Add MQTT v5 support #1709

Add MQTT v5 support #1709

Conversation

feymartynov commented Jul 24, 2019

lminiero commented Jul 24, 2019 • edited Loading

lminiero commented Aug 1, 2019

feymartynov commented Aug 1, 2019

feymartynov commented Aug 1, 2019

lminiero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feymartynov commented Aug 2, 2019

lminiero commented Aug 2, 2019

lminiero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feymartynov commented Aug 2, 2019

lminiero commented Aug 2, 2019

lminiero commented Jul 24, 2019 •

edited

Loading