telnet: Quote IAC on sending

[ukleinek]

To send a single IAC in the payload, you have to send two IACs, the first quoting the second. Otherwise the sequence will be interpreted as a command by the other side.

[jluebbe]

Will dprintf handle EINTR and incomplete writes correctly?

It took me some time to understand what this code should do. Perhaps it would be better to first quote the IAC in a local buffer (which can't fail) and then use a generic retrying send helper?

[ukleinek]

Incomplete writes are a principle problem before and after my patch also in the other backends. See also issue #21.

The problem with quoting the IAC first is that the buffer is provided by mux.c, so the telnet code would need to allocate memory (which complicates the code further).

I added a few comments to hopefully simplify understanding the code.

[jluebbe]

Why must only the first IAC be quoted?

[ukleinek]

Why must only the first IAC be quoted?

Hmm, should I add "Iterate for all IACs in the input" to the comment? In the code all IACs are handled.

[jluebbe]

Ah, the comment confused me. Being explicit about iteration sounds good.

I still think this would be simpler by just allocating count*2, handling the quoting and then one write. :)

[ukleinek]

I still think this would be simpler by just allocating count*2, handling the quoting and then one write. :)

I found another difficulty with this approach: I want to keep the return code semantic as is. This is necessary to later properly handle partial writes. If telnet_write is called with (say) 40 bytes to write, there are 3 IACs in the input, so in the end write(ios->fd, localbuf, 43); is called. If that write returns 20, it's not trivial to determine what to return. In the end this has the downside to allocate extra memory and isn't easier than the approach I chose now.

[jluebbe]

I don't see any way this function would return with a partial write in a non-error-case.

In your example above, the function would not return, keep retrying. The final return value seems to be either negative or count. As count is already known to the caller, there is not much benefit in returning it.

[ukleinek]

The idea is to not block in .write() to keep microcom responsive to user input (and maybe other events) and so I want to do completion of writes in the core instead of in the callback directly.
Once this works, .write() can also become simpler and look e.g. like this:

if (count == 0)
    return 0;
if (buf[0] == IAC) {
    return write_full(ios->fd, "%c%c", IAC, IAC); // TODO: better handle partial write
} else {
    const char *iac = memchr(buf, IAC, count);
    if (!iac)
        iac = buf + count;
    return write(ios->fd, buf, iac - buf);
}

[jluebbe]

Ah. As long as you may need to write more than one byte in the lower level write, you may need to block. Perhaps split the quoting from the write function? The quoting function would append to a buffer and the core could handle draining the buffer asynchronously (i.e. via poll()/select()), perhaps using libuv/libevent?

[ukleinek]

Yeah, the eventual plan is something like that. However I hesitate to add a dependency like libuv or libevent, so I tend to do it directly with poll() or select() (or maybe epoll()).

[ukleinek]

I accidently pushed some debug code, to see the intended changes, look at https://github.com/pengutronix/microcom/compare/3080a9d..d721d2d9bd49e7fcd399e7fd3b3bb548e077ce3d

[a3f]

Why not just write(ios->fd, "\xff\xff", 2); /* 2x IAC */? And in future, this could be changes to write_full or similar that retries on EINTR.

[ukleinek]

Why not just write(ios->fd, "\xff\xff", 2); /* 2x IAC */? And in future, this could be changes to write_full or similar that retries on EINTR.

I wanted to benefit from the definition of IAC and not open-code it as \xff.

[a3f]

I wanted to benefit from the definition of IAC and not open-code it as \xff.

Not completely serious, but there's also (uint8_t []) { IAC, IAC }..