Add MQTT protocol handler #3514

zagor · 2019-01-31T15:40:03Z

This patch adds support for MQTT using the mqtt://broker/topic URL scheme.

A plain GET subscribes to the topic and prints all published messages.
Doing a POST publishes the post data to the topic and exits.

Example subscribe: curl mqtt://home/bedroom/temp
Example publish: curl -d 80 mqtt://home/bedroom/dimmer

Limitations:

No username/password support
Only QoS level 0 is implemented for publish
No way to set retain flag for publish
No TLS (mqtts) support
Naive EAGAIN handling won't handle split messages

zagor · 2019-01-31T15:43:15Z

lib/mqtt.c

+    break;
+  }
+  if(result)
+    infof(conn->data, "=== %s result: %d\n", __func__, result);


Oh, this is leftover debug code. Will be removed.

bagder · 2019-01-31T23:15:45Z

Nits on travis:

mqtt.h:33:17: error: comma at end of enumerator list [-Werror=pedantic]
     MQTT_SUBWAIT,
                 ^

and

test 1119...[Verify that symbols-in-versions and headers are in sync]
perl  returned 2, when expecting 0
 exit FAILED
== Contents of files in the log/ dir after test 1119
=== Start of file stdout1119
 CURLPROTO_MQTT
=== End of file stdout1119

and in the tidy build:

mqtt.c:199:6: error: variable 'packet' is used uninitialized whenever 'if'
      condition is true [-Werror,-Wsometimes-uninitialized]
  if(result)
     ^~~~~~
mqtt.c:225:6: note: uninitialized use occurs here
  if(packet)
     ^~~~~~
mqtt.c:199:3: note: remove the 'if' if its condition is always false
  if(result)
  ^~~~~~~~~~
mqtt.c:195:24: note: initialize the variable 'packet' to silence this warning
  unsigned char *packet;
                       ^
                        = NULL
mqtt.c:287:6: error: variable 'topic' is used uninitialized whenever 'if'
      condition is true [-Werror,-Wsometimes-uninitialized]
  if(!pkt) {
     ^~~~
mqtt.c:310:6: note: uninitialized use occurs here
  if(topic)
     ^~~~~
mqtt.c:287:3: note: remove the 'if' if its condition is always false
  if(!pkt) {
  ^~~~~~~~~~
mqtt.c:281:14: note: initialize the variable 'topic' to silence this warning
  char *topic;
             ^
              = NULL
2 errors generated.

bagder · 2019-01-31T23:19:05Z

Any ideas or thoughts on how we can add tests for this?

wopfel · 2019-02-01T07:37:49Z

@zagor

This is very nice :-)

But why are your subscribe and publish examples beginning with http://?

zagor · 2019-02-01T08:19:02Z

But why are your subscribe and publish examples beginning with http://?

Haha. Because old habits die hard? 😁 Edited.

conduit242 · 2019-02-01T18:41:31Z

When you say no support for username and password, does that mean it's not setting the username and password flags or just can't pass values at the moment? The vast majority of MQTT servers are authenticated. Many systems use JWTs or x.509 certificates passed in the username or password field for auth, it may be valuable to support inclusion of a file for these fields.

zagor · 2019-02-02T09:56:11Z

@conduit242 The limitations are not meant to be permanent, quite the contrary. I listed things that I think might be interesting to add in the future but which this first patch does not contain.

danielgustafsson · 2019-02-03T11:08:46Z

lib/mqtt.c

+
+  /* reallocate a bigger buffer if necessary */
+  if(pktlen > 128) {
+    pkt = realloc(pkt, pktlen + 2); /* one extra for payload termination */


realloc() without a temporary returnvalue will overwrite the original pointer on allocation error, better to use Curl_saferealloc() here instead.

bbucko · 2019-02-11T14:29:55Z

lib/mqtt.c

+static CURLcode mqtt_connect(struct connectdata *conn)
+{
+  CURLcode result = CURLE_OK;
+  const char clientid[] = "curl1234abcd";


Two users will not be able to use curl concurrently if static client id is used. According to spec, client id must be unique so existing client will be disconnected.

This variable is actually only used to find out the length of the client id string (rather than using a magic number). The real client id sent is random and written into the packet a few lines below:
msnprintf(packet + client_id_offset, sizeof(packet), "curl%08x", rand());

But this code isn't terribly elegant, and your misunderstanding shows me I should rewrite it to be more clear. Thanks for reviewing!

bagder · 2019-02-17T13:26:39Z

scan-build found something (appveyor looks like a false positive)

mqtt.c:407:3: warning: Value stored to 'result' is never read
  result = Curl_client_write(conn, CLIENTWRITE_BODY, ptr, payloadlen);
  ^        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

bagder · 2019-05-11T11:09:56Z

@zagor this now has a merge conflict. Do you have any updates on this work? What can we do to help out?

zagor · 2019-05-15T07:43:08Z

I got stuck on trying to figure out if I should implement a new mqtt broker and client in python for the tests or if I should add dependencies on external modules.

bagder · 2019-05-17T10:10:28Z

If I would do it, I would probably write a new MQTT test server in C based on one of the existing TCP servers. (Possibly the sockd server that I introduced most recently.)

Reasons for this choice:

make sure that it can run in many places, including Windows makes testing globally easier
makes it easy to support doing "funny" things by command for testing broken behavior
not depending on (another) scripting language in the tests
doesn't have to actually understand the protocol, just be able to receive and send data as instructed

bagder · 2019-06-10T13:57:58Z

Okay how about this approach to get this moving forward:

rebase to current master
put the mqtt support behind a configure option that makes it disabled by default (so that we can mark and regard it as "experimental" for now)
we can merge that early to allow bleeding edge users to try it out
then we can probably cooperate better on getting docs/tests and whatever fixed
when we're happy, remove configure option and experimental tag

danielgustafsson · 2019-06-11T09:41:51Z

Okay how about this approach to get this moving forward:

rebase to current master

put the mqtt support behind a configure option that makes it disabled by default (so that we can mark and regard it as "experimental" for now)

we can merge that early to allow bleeding edge users to try it out

then we can probably cooperate better on getting docs/tests and whatever fixed

when we're happy, remove configure option and experimental tag

👍

zagor · 2019-06-11T10:01:02Z

Thank you for your patience, sorry about my slowness. I'll get on with this today or tomorrow.

bagder · 2019-06-13T10:15:43Z

The appveyor 1013 test case failure seems to be related to the mqtt thing in CMakeLists.txt. But I don't understand why that only makes one test run fail?

zagor · 2019-06-13T10:21:10Z

I don't understand how curl and curl-config can differ. Aren't they built from the same configure run?

bagder · 2019-06-13T10:25:09Z

Enter: build complexity. The appveyor runs are all on Windows. Most of those builds are done with cmake, not configure. But yes, curl and curl-config are made by the same build process so they should match, which is what test 1013 verifies...

bagder · 2019-06-13T16:15:23Z

I suppose the simplest "fix" is to just not add any MQTT knowledge to the cmake build... I don't know how to do it the proper way, cmake is not my friend.

bagder · 2019-06-13T18:39:57Z

@zagor, since I proposed doing plain C server to drive MQTT tests, I created a "skeleton" for it based on what I did with socksd.c not too long ago to show off what I had in mind.

It is basically a template source code that creates a TCP server, accepts a connection and can read/write to the client. It is designed to read commands from a config file at client connect so that the test script can pass in test specific instructions per test case.

I personally don't know much about the MQTT protocol yet so I didn't venture into doing any actual protocol things.

No pressure, it's just an idea!

0001-mqttd-skeleton.patch

zagor · 2019-06-16T07:55:30Z

@bagder Thanks, that is exactly what I needed!

This patch adds support for MQTT using the mqtt://host/topic URL scheme. A plain GET subscribes to the topic and prints all published messages. Doing a POST publishes the post data to the topic and exits. Example subscribe: curl http://host/home/bedroom/temp Example publish: curl -d 80 http://host/home/bedroom/dimmer Limitations: - No username support - Only QoS level 0 is implemented for publish - No way to set retain flag for publish - No username/password support - No TLS (mqtts) support - Naive EAGAIN handling won't handle split messages

Also, Use Curl_rand_hex() instead of sprintf(rand()).

Enable it with --enable-mqtt. Marked as experimental.

stale · 2019-12-18T06:46:29Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

zagor commented Jan 31, 2019

View reviewed changes

danielgustafsson reviewed Feb 3, 2019

View reviewed changes

zagor force-pushed the mqtt branch from fede96f to 83421c7 Compare February 7, 2019 09:37

bbucko reviewed Feb 11, 2019

View reviewed changes

bagder added the needs-info label May 11, 2019

zagor force-pushed the mqtt branch 3 times, most recently from 1f29073 to cad9f2d Compare June 13, 2019 09:00

bagder removed the needs-info label Jun 13, 2019

zagor force-pushed the mqtt branch from cad9f2d to 8c2125d Compare June 20, 2019 20:23

zagor added 9 commits June 21, 2019 07:48

Fix travis remarks. Add length checks.

90010b5

Fix "initializer element is not a constant"

77a2035

Move strlen initializers to runtime

1c56b8d

Add MQTT to protocol and symbol lists.

f06e3c4

Add MQTT cmake options

b506b9b

Clarify client id code.

7a137ab

Also, Use Curl_rand_hex() instead of sprintf(rand()).

Fix buggy client id code...

4572272

Disable MQTT by default

0173b36

Enable it with --enable-mqtt. Marked as experimental.

Disable MQTT by default in cmake builds

c44b03a

zagor force-pushed the mqtt branch from 8c2125d to c44b03a Compare June 21, 2019 05:51

jay added the needs-info label Dec 7, 2019

stale bot added the stale label Dec 18, 2019

stale bot closed this Jan 1, 2020

lock bot locked as resolved and limited conversation to collaborators Apr 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MQTT protocol handler #3514

Add MQTT protocol handler #3514

zagor commented Jan 31, 2019 •

edited

Loading

zagor Jan 31, 2019

bagder commented Jan 31, 2019

bagder commented Jan 31, 2019

wopfel commented Feb 1, 2019

zagor commented Feb 1, 2019

conduit242 commented Feb 1, 2019

zagor commented Feb 2, 2019

danielgustafsson Feb 3, 2019

bbucko Feb 11, 2019

zagor Feb 11, 2019

bagder commented Feb 17, 2019

bagder commented May 11, 2019

zagor commented May 15, 2019

bagder commented May 17, 2019

bagder commented Jun 10, 2019

danielgustafsson commented Jun 11, 2019

zagor commented Jun 11, 2019

bagder commented Jun 13, 2019

zagor commented Jun 13, 2019

bagder commented Jun 13, 2019

bagder commented Jun 13, 2019

bagder commented Jun 13, 2019

zagor commented Jun 16, 2019

stale bot commented Dec 18, 2019

Add MQTT protocol handler #3514

Add MQTT protocol handler #3514

Conversation

zagor commented Jan 31, 2019 • edited Loading

zagor Jan 31, 2019

Choose a reason for hiding this comment

bagder commented Jan 31, 2019

bagder commented Jan 31, 2019

wopfel commented Feb 1, 2019

zagor commented Feb 1, 2019

conduit242 commented Feb 1, 2019

zagor commented Feb 2, 2019

danielgustafsson Feb 3, 2019

Choose a reason for hiding this comment

bbucko Feb 11, 2019

Choose a reason for hiding this comment

zagor Feb 11, 2019

Choose a reason for hiding this comment

bagder commented Feb 17, 2019

bagder commented May 11, 2019

zagor commented May 15, 2019

bagder commented May 17, 2019

bagder commented Jun 10, 2019

danielgustafsson commented Jun 11, 2019

zagor commented Jun 11, 2019

bagder commented Jun 13, 2019

zagor commented Jun 13, 2019

bagder commented Jun 13, 2019

bagder commented Jun 13, 2019

bagder commented Jun 13, 2019

zagor commented Jun 16, 2019

stale bot commented Dec 18, 2019

zagor commented Jan 31, 2019 •

edited

Loading