Add MQTT v5 properties support #2273
Conversation
|
Thanks! From a quick glance it looks like a big patch, so I'll need some time to review it. I'll get back to you as soon as I have some feedback 👍 |
| @@ -46,4 +47,7 @@ general: { | |||
| #tls_client_key = "/path/to/key.pem" | |||
| #tls_ciphers | |||
| #tls_version | |||
|
|
|||
| # These options work with MQTT 5 only. | |||
| #add_user_properties = () # List of user property ["key", "value"] pairs to add. | |||
There was a problem hiding this comment.
It's nice to see the list/array features of libconfig finally used somewhere 🙂
events/janus_mqttevh.c
Outdated
| @@ -142,6 +145,7 @@ typedef struct janus_mqttevh_context { | |||
|
|
|||
| /* Connection data - authentication and url */ | |||
| struct { | |||
| int mqtt_version; | |||
There was a problem hiding this comment.
Indentation is broken: please use tabs and not spaces (not sure if the same happens in other parts of the code, in case please fix it there too).
events/janus_mqttevh.c
Outdated
| @@ -161,6 +165,9 @@ typedef struct janus_mqttevh_context { | |||
| char *disconnect_status; | |||
| int qos; | |||
| int retain; | |||
| #ifdef MQTTVERSION_5 | |||
| GArray *add_user_properties; | |||
events/janus_mqttevh.c
Outdated
|
|
||
| #ifdef MQTTVERSION_5 | ||
| static int janus_mqttevh_client_publish_message5(janus_mqttevh_context *ctx, const char *topic, int retain, char *payload, MQTTProperties *properties) { | ||
| int rc; |
events/janus_mqttevh.c
Outdated
| msg.payloadlen = strlen(payload); | ||
| msg.qos = ctx->publish.qos; | ||
| msg.retained = retain; | ||
| msg.properties = MQTTProperties_copy(properties); |
transports/janus_mqtt.c
Outdated
| @@ -956,32 +1254,32 @@ int janus_mqtt_client_subscribe(janus_mqtt_context *ctx, gboolean admin) { | |||
| MQTTAsync_responseOptions options = MQTTAsync_responseOptions_initializer; | |||
| options.context = ctx; | |||
| if(admin) { | |||
| #ifdef MQTTVERSION_5 | |||
| #ifdef MQTTVERSION_5 | |||
There was a problem hiding this comment.
See above on macros indentation.
transports/janus_mqtt.c
Outdated
| if(ctx->connect.mqtt_version == MQTTVERSION_5) { | ||
| options.onSuccess5 = janus_mqtt_client_admin_subscribe_success5; | ||
| options.onFailure5 = janus_mqtt_client_admin_subscribe_failure5; | ||
| } else { | ||
| options.onSuccess = janus_mqtt_client_admin_subscribe_success; | ||
| options.onFailure = janus_mqtt_client_admin_subscribe_failure; | ||
| } | ||
| #else | ||
| #else |
There was a problem hiding this comment.
See above on macros indentation.
transports/janus_mqtt.c
Outdated
| options.onSuccess = janus_mqtt_client_admin_subscribe_success; | ||
| options.onFailure = janus_mqtt_client_admin_subscribe_failure; | ||
| #endif | ||
| #endif |
There was a problem hiding this comment.
See above on macros indentation (not going to write it for them all, as I see there are others: please fix them all).
transports/janus_mqtt.c
Outdated
| /* MQTT 5 specific types */ | ||
| typedef struct janus_mqtt_transaction_state { | ||
| MQTTProperties *properties; | ||
| time_t created_at; |
There was a problem hiding this comment.
Why the time_t? It may be easier (and probably more easily compatible on other systems) to use a gint64 here and rely on janus_get_monotonic_time() instead.
| @@ -28,6 +28,10 @@ general: { | |||
| #certfile = /path/to/cert.pem | |||
| #keyfile = /path/to/key.pem | |||
|
|
|||
| # These options work with MQTT 5 only. | |||
| #vacuum_interval = 60 # Interval for removing old transcation states in seconds. | |||
There was a problem hiding this comment.
Typo: transcation --> transaction
feymartynov
force-pushed
the
feature/mqttevh-v5
branch
from
5cc15f1
to
dc02133
Compare
There was a problem hiding this comment.
Looks good now, thanks for the fixes!
I'm quite busy so I won't be able to test them, though: considering I never use the MQTT plugins myself, I'm not even sure my checks there would make much sense. I'm assuming you're using MQTT extensively in your setups, so is it safe to say you've tested both of them and they still for fine for you? If so, I'm ok with merging them.
|
Yes, sure. We have this branch running in production since December and it's OK so far. |
|
Excellent, then it's safe to merge for me 👍 Thanks again for taking the time to work on this huge update and sharing it! |
This is related to #1872.
Sorry for taking a long time to send the PR. We had to postpone testing of this feature but now it's good to go.
Also this PR includes MQTT v5 support for the MQTT event handler in the similar way as for the MQTT transport.