[paho-dev] mqtt.embedded-c/MQTTPacket/ samples and src/MQTTPacket.c

["Sergio R. Caprile" <scaprile@xxxxxxxxx>]

Hi,
I started doing basically the same I did for MQTT-SN, but now for MQTT.
So far the samples are working as expected with a broker.on my machine.

However, when connecting to the eclipse broker, I noticed the sub sample
does not work properly: lots of "publishing reading" followed by
connection closures.
I happen to have a low-speed, way far away, fully loaded connection, so
I checked the TCP read code as usual, here is what I've found:

There are many points in which the receiving process will fail if a
whole message is not present at the time the sample (the user) is trying
to read. This, in turn, may (will) cause succesive calls to fail because
of a sync loss (the first byte in the stream is not the expected start
of message). This, in the sample, causes the publish function to be
caled once for each byte caught in the stream.

int MQTTPacket_decode(int (*getcharfn)(unsigned char*, int), int* value)
{
[...]
                rc = (*getcharfn)(&c, 1);
                if (rc != 1)
                        goto exit;
*** --> this fails if there is not enough data yet

int MQTTPacket_read(unsigned char* buf, int buflen, int
(*getfn)(unsigned char*,
 int))
{
[...]
        /* 1. read the header byte.  This has the packet type in it */
        if ((*getfn)(buf, 1) != 1)
                goto exit;
*** --> This may get trashed by (2) or (3)
        len = 1;
        /* 2. read the remaining length.  This is variable in itself */
        MQTTPacket_decode(getfn, &rem_len);
        len += MQTTPacket_encode(buf + 1, rem_len); /* put the original
remainin
g length back into the buffer */
*** --> This fails if MQTTPacket_decode() fails (just one byte in the
buffer)
        /* 3. read the rest of the buffer using a callback to supply the
rest of
 the data */
        if ((*getfn)(buf + len, rem_len) != rem_len)
                goto exit;
*** --> This fails if the message is not complete, or is longer than the
supplied buffer

IMO, the getfn function must first check to see if there is enough data
as requested, so we can avoid some state machines here. If it returns
"not yet", then MQTTPacket_read() may return "no message". However, this
requires a transport with "peek()" capability, and there might be some
that can't. Otherwise, bytes in the stream must be accumulated somewhere
until there is enough data to check message length and then the message;
otherwise, we'll have data loss over a TCP link.

Do you guys agree?