RFC2217 support integration

[npat-efault]

Thanks to @dleonard0 who contributed the implementation, picocom is in the process of gaining support for RFC2217-protocol remote ports.

Here is the original PR #93 submitted by @dleonard0 Integration work currently happens in branch rfc2217, so keep monitoring this if interested.

I open this issue to have a convenient place for the relevant discussion...

[npat-efault]

@dleonard0

A more serious problem: The totally-asynchronous nature of RFC2217 commands can cause some problems, especially during program initialization and startup.

When a new port is added, first term_add() is called, which then calls term_new(), which calls (through the ops indirection) tn2217_init(). This gets the RFC2217 negotiation going and returns. Once tn2217_init() returns, term_new() also returns, and term_add() calls (again through indirection) tcsgetattrs() to aquire the initial port settings (here).

The problem is that, at this point, negotiation has not completed, and the port settings are not, yet, known. So the term library copies in its origtermios-structure some arbitrary settings (not the initial port settings as it was supposed to). Ideally, we should wait for the negotiation to conclude and the original port options to become known before proceeding. This does not practically cause a problem, but a similar phenomenon a bit later-on, does.

Once the RFC2217 port is added, it gets configured, by calling term_set(), followed by term_apply(), which, in turn, calls tn2217_tcsetattr(). When tn2217_tcsetattr() (and subsequently term_apply()) returns, the port configuration has not completed, and picocom enters its main loop.

This is a very real problem: The (rational) user expectation is that the port has been configured (to the command-line settings values) before picocom enters its main loop. This problem can easily manifest if the user does something like, for example:

echo -e 'AAATZ\r\n' | picocom -x 2000 -T localhost,7777

The string is sent to the port, before the port has been configured, which is clearly wrong.

Possible solution: The solution would be to wait for the respective RFC2217-commands to complete (i.e. their outstanding responses actually received by the tn2217 code). This must happen, possibly, every time a configuration change is requested (i.e. tn2217_tcsetattr() is called), but certainly after the first call, before picocom enters the main loop. That is, the "wait for outstanding requests to complete" code could either be a part of tn2217_tcsetattr(), or be a separate operation that is only invoked at specific cases...

Postscript 1: A question occurs: What to do with user-data that are received from the port while waiting for the outstanding operations to complete? Hopefully, I believe, that the easy answer (discard them) is adequate. I'm almost certain it is ok to discard them at initialization and initial configuration phases---but I think it would also be ok to discard them at subsequent configuration change-requests (since a configuration change-request is a disruptive operation, anyhow).

Postscript 2: If discarding user-data, as described in postscript 1, i not acceptable, then the only solution (and it is a much harder one) is to buffer them, and have term_read() deliver the buffered user-data when subsequently called. But let's not get into details on this (yet), since I don't believe it will be needed.

[npat-efault]

Clean-up and refactoring / restructuring for the integration with picocom is mostly done... More testing is needed and (most-likely) more small tweaks will come-up during testing.

Most important, for now, is the error / problem described in the comment above...

[npat-efault]

Commit 0f2194d6a895df2c186ca7a3a24a1ecb8851e931 is a solution to the issue described in https://github.com/npat-efault/picocom/issues/95#issuecomment-367064989

[dleonard0]

Hi, @npat-efault Yes that is a problem in non-interactive mode.

I think i see how 0f2194d operates, and I suspect I can achieve a fix with less complexity. I would make tn2217_open() block until it had established that the remote service handled (or didn't handle) all the requested option.

You wondered what to do with user data received while waiting for option negotiations to settle? I would send them to stdout. I recommend not discarding them because sometimes just the act of accessing a remote port can cause the service to dump its own rx buffers, which may contain valuable content.

[dleonard0]

I'm not sure how to reference it here through github, but please see a perhaps simpler fix at https://github.com/dleonard0/picocom/tree/tn2217-init-fixes

• block during tn2217_init() until COM-PORT is established
• we only need the server to support COM-PORT (it's an asymmetric option)
• the can_comport flag is no longer needed
• copy received data to stdout
• use alarm() to give up waiting for non-responsive server

[npat-efault]

@dleonard0 Hmm, maybe not open tn2217_open(), since there you don't have the term_s structure to keep any state... you only have the filedes (??). In term_add() / tn2217_init() seems more likely. I could do that too, using the pretty-much the same mechanism (instead of blocking in read / write I could block in tn2217_add()). And maybe it would be cleaner...

Also: You will see a couple of FIXME comments on top of the "wait for config" code in tn2217_read and tn2217_write, that indicate that we should not block and wait if s->set_termios is not set. The rationale is that if a user runs picocom with --noinit, he should neither wait for configuration to complete (since effectively, no configuration is done), nor "lose" any initial data send by the server. If we move the blocking code to term_add() / tn2217_init(), then perhaps we should add an extra arg, like "noinit" or something...

On the other hand maybe this is too complicated, and we should always (regardless of --noinit) wait for negotiations to finish...

[dleonard0]

Less complicated is good. Perhaps it would be easier to document caveats about Telnet mode than to close all the differences with local terms?

[npat-efault]

Regarding https://github.com/dleonard0/picocom/tree/tn2217-init-fixes

I believe that before we start reading from the port, the port must have been configured to the requested settings. That is: configuration commands send, and replied. We do not want to read (show to the user) data comming from a remote device that is configured to work at a different baudrate than the port it's connected to (i.e garbage data).

More detailed explanation: If I run:

picocom -b 19200 -T host,7777

Then I expect that at the requested port, a device is connected that is configured to operate @ 19200. Obviously, I don't want to see ANY data from this device, received before the port is configured to the requested settings (as this data will by definition be garbage). This means I have to wait for the config command to be replied, and discard any data received in between.

On the other hand, if I run:

picocom --noinit -T localhost,7777

Then yes, I assume the port is already properly configured, and I want to see data coming from it, as soon as I open it / connect to it...

[dleonard0]

before we start reading from the port

Sadly, I believe that once the server accepts the TCP connection it may begin reading from its port, even before RFC2217 support is negotiated, making this an impossible problem to solve.

[dleonard0]

The servers I have seen tend to retain the UART settings of their port. The ones I am most familiar with are console servers which have their port speeds already set up. So, specifying --noinit would be normal operation for these.

[npat-efault]

Sadly, I believe that once the server accepts the TCP connection it may begin reading from its port, even before RFC2217 support is negotiated, making this an impossible problem to solve.

The server may start reading immediately, but we should discard any data read and sent (to us) before the port is configured. That is, any data sent (to us) before the replies to the config commands. This not very different to what happens when you open a local port. Any data received by the driver before the port is configured are normally discarded / flushed...

[npat-efault]

Two questions:

1/ What would it mean for the server to accept COMPORT, while we haven't? That it's ok for us to send COMPORT commands, but not ok for him to send responses? Or that it would not be ok for him to send notifications? (Since I believe the server never initiates COMPORT commands, anyway). The distinction seems a bit moot

2/ In what way would the behavior of the server (or ours, for that matter) be different before BINARY is negotiated?

[dleonard0]

Any data received by the driver before the port is configured are normally discarded

I see: This is the "init" (non-noinit) case where the picocom user is in full control of the port, and discarding garbage makes sense. For this to work, I think we'd have to keep track of the first time we receive confirmation of UART changes, and discard rx'd data up to that point? I'm not sure how that should go. Perhaps a call to tn2217_read() could be interpreted to indicate that setup has completed?

1/ What would it mean for the server to accept COMPORT, while we haven't?

That was a mistake, fixed in 3cf7f7b. We only implemet the client half of COM-PORT, so we need only send a DO, and not a WILL.

Update: also, we could start sending the COM-PORT sub-commands immediately and maybe the remote end will process or discard them. But we really shouldn't until the remote has told us that it WILL COM_PORT.

2/ In what way would the behavior of the server (or ours, for that matter) be different before BINARY is negotiated?

Without BINARY, the peer could conceivably choke on characters that are not NVT-ASCII. I've always thought of BINARY to mean "8-bit clean". The details are in https://tools.ietf.org/html/rfc856 and NVT-ASCII is defined in RFC 854 and is basically 7-bit ASCII with a few holes in it:

The code set is seven-bit USASCII in an eight-bit field, except as modified herein.

(Yet another edit:) In a UTF-8 world, we pretty much need 8-bit :)

[npat-efault]

If you think of it, our differences are:

1/ You (say we should) wait for COMPORT to be negotiated, always. I (say we should) wait for the initial COMPORT commands to be replied, unless run with --noinit. In this case, don't wait for anything (maybe only for BINARY to be negotiated?)

2/ While waiting, you timeout using alarm signals. I timeout using select() (and I prefer it very much this way).

3/ I add a tn2217_wait_for_cmd() function which is essentially a repeated tn2217_read() with a length of 1, until a full command is received (plus it accepts a timeout). Once a full command is received and processed, I check if the conditions I'm waiting for are met. You, instead, call tn2217_read() with a length of 1, and check if the conditions you are waiting for are met after every character read. I agree that tn2217_wait_cmd() replicates some if the tn2217_read() code, but it's the best I could come up with...

4/ I agree with you that the "waiting for negotiations" should better happen in tn2217_init() and not in tn2217_read() and tn2217_write() (it's more confusing this way).

5/ You say we should dump everything read from the server (even before negotiations are complete, and regardless of --noinit) to stdout (ignoring their proper path through picocom, and stuff like character-mapping, logging, etc) as a solution of expedience. I say we discard everything, unless picocom is run with --noinit in which case we don't have to wait for negotiations to complete...

I think it's a fair assessment, don't you?

[npat-efault]

I see: This is the "init" (non-noinit) case where the picocom user is in full control of the port, and discarding garbage makes sense. For this to work, I think we'd have to keep track of the first time we receive confirmation of UART changes, and discard rx'd data up to that point? I'm not sure how that should go. Perhaps a call to tn2217_read() could be interpreted to indicate that setup has completed?

The way I see it, any data rx'ed (by us) before the replies to the COMPORT config commands, are data (read by the server) before its UART was reconfigured. Any data received (by us) after the replies, are data (read by the server) after its UART was reconfigured.

It is conceivable that a server could behave differently, but it would have to be a very strange implementation (and in any case we can do nothing better in this case).

[npat-efault]

That was a mistake, fixed in 3cf7f7b. We only implemet the client half of COM-PORT, so we need only send a DO, and not a WILL.

I think you got it backwards... Copying from RFC2217:

The negotiation of the com port control option protocol uses the
   standard Telnet negotiation protocol mechanism:

     IAC WILL COM-PORT-OPTION
       The sender of this command is willing to send com port
       control option commands.
     IAC WONT COM-PORT-OPTION
       The sender of this command refuses to send com port
       control option commands.
     IAC DO COM-PORT-OPTION
       The sender of this command is willing to accept com port
       control option commands.
     IAC DONT COM-PORT-OPTION
       The sender of this command refuses to accept com port control
       options commands.

    Typically a client will use WILL and WONT, while an access server
    will use DO and DONT.

So we need to send a WILL and not a DO

[dleonard0]

I think you got it backwards

!%@#%, you're right!

I think it's a fair assessment, don't you?

Yes that's pretty fair. Perhaps this reveals the extent that I have had expediency beaten into me ;)

[dleonard0]

Actually, if the meaning of the option is that the peer will send IAC SB COM-PORT ... IAC SE messages, and we expect to both send and receive control messages then I guess that means it has to go back to both DO and WILL?

[npat-efault]

Actually, if the meaning of the option is that the peer will send IAC SB COM-PORT ... IAC SE messages, and we expect to both send and receive control messages then I guess that means it has to go back to both DO and WILL?

Debatable... It depends on how you interpret the "willing to accept" part of the following:

IAC DO COM-PORT-OPTION
       The sender of this command is willing to accept com port
       control option commands.

Is sending the reply / acknowledgement part of me accepting a command? Or is it considered another command altogether?

I tend to lean towards the former interpretation based on the wording of this:

  Once DO and WILL have been negotiated, the client may send any of the
   following commands. The client can send these commands at any time
   and multiple times throughout the Telnet session. Each command
   transmitted from the client to the access server must be acknowledged
   once the command has been processed by the access server.  This
   confirmation informs the client of the value set at the access server
   after the processing of the command. This acknowledgment is not used
   to acknowledge the receipt of the command, which is handled at the
   TCP protocol layer.  Its purpose is to inform the client of the value
   in use, which may be different than the value requested in the
   client's command.  For example, the client may request a baud rate
   higher than the access service can provide.  If an acknowledgment is
   not received by the client within a reasonable time (such as twice
   the delay acknowledgment timer), the client may wish to resend the
   command or terminate the session.

This of-course leaves open the case of unsolicited access server notifications (e.g MODEMSTATE or LINESTATE notifications), but these are never mentioned as "commands" and they need not be acknowledge by the client.

So I tend to believe that the intention was for the client to announce with WILL, and the access server to ack with DO, and not the other way around...

The opposite (the access server anouncting with WILL and the client acking with DO) would (to me) indicate the intention of the access server to initiate himself COMPORT commands (for whatever reason I cannot think of)...

[dleonard0]

BTW I have also started to wonder if this feature is "too big" for picocom. Perhaps a separated pty-rfc2217 approach would be better... I think I would prefer that picocom stay "pico".

[npat-efault]

BTW I have also started to wonder if this feature is "too big" for picocom. Perhaps a separated pty-rfc2217 approach would be better... I think I would prefer that picocom stay "pico".

I have also thought of this, but no...

1/ The memory footprint is not too large

2/ It can be conditionally compiled out

3/ The implications to the rest of picocom are not too big (and most of them, like the abstraction of term are rather good)

Actually picocom has stopped being truly, really, honest to god, "pico" quite some time ago (not that it's getting huge... I don't mean that)...

[npat-efault]

4/ I agree with you that the "waiting for negotiations" should better happen in tn2217_init() and not in tn2217_read() and tn2217_write() (it's more confusing this way).

OOPS! Important!

Now I remembered why I didn't put the "waiting for negotiations" code in tn2217_init() in the first place:

Because, at this point, the port configuration settings have not been passed to tn2217 anyway (since term_set() and term_apply() have not been called). At this point we actually don't know (yet) if we will requests the port's configuration or not.

Aside: As a matter of fact, your code in tn2217_init() would not work. At the point it deems the negotiations complete, no port-configuration COMPORT commands have been sent to the server, so the subsequent tn2217_write() will actually write data to an un-configured port, hence our initial problem.

So my "wait for port ready" code in tn2217_read() and tn2217_write() makes perfect sense, even conceptually, like this:

The first time you are asked to read (user data) from the port, or write (user data) to the port, see if the port's configuration has been requested. If so, make sure that all negotiations have finished and that the port has actually been configured to the requested settings, before going ahead with the read, or the write. If, instead, no port configuration has been requested, then go ahead with the read or the write immediately, as the port's existing configuration can be assumed operable.

The check for "if the port's configuration has been requested", is not currently done, but it's the subject of the FIXME commend in tn2217_read() and tn2217_write()... (and it's trivial).

So, try to peruse through the code at the tip of my rfc2217 branch, and you'll see that it's neither too complicated, not too ad-hoc and irrational.

The things I'm not very happy with are:

1/ I would prefer it if there was a function like term_wait_ready() called before we enter the main loop, as this would be the perfect place to put the "wait for negotiations to completer, or not" code... but there not being one, putting it in term_read() and term_write() seems like the next logical choice.

2/ The fact that tn2217_wait_cmd() replicates some code from tn2217_read() but it's the best I could come up with.

2/ I would be open to different ways of verifying that the configuration commands have been replied, but the current one (of counting requests against replies) seem ok to me.

3/ The thing about the COMPORT negotiation being symmetric or not, we discussed before.

EDIT: FIXED TYPOS

[npat-efault]

Regarding COMPORT negotiation:

I ran a little test with sredird.

If picocom sends, by itself, neither WILL COMPORT, nor DO COMPORT, then sredird will send DO COMPORT (but it will not send WILL COMPORT).

(If picocom does sent DO COMPORT, then sredird does not reject us, and replies with WILL COMPORT... But it will not send WILL COMPORT by itself).

This seems to support the case that clients should send WILL COMPORT and expect the access server to reply with DO COMPORT, but they should (or at least need) not send DO COMPORT...

[npat-efault]

Some additional tests with ser2net, which seems much more polished than sredird.

I've rigged picocom's debugging output to show all messages exchanged during negotiation (I will commit this, when I polish it up a bit). I 've also removed irrelevant debugging output...

First a run when picocom initiates no negotiation (no WILL no DO) at all:

[received: WILL SGA]
[sent: DO SGA]
[received: WILL ECHO]
[sent: DONT ECHO]
[received: DONT ECHO]
[received: DO BINARY]
[sent: WILL BINARY]
[received: WILL SGA]
[received: WONT ECHO]
[received: DO BINARY]

Ok, the implementation seems not to be exactly RFC854 compliant, since it sends several superfluous messages, but it does not attempt to send any WILL COMPORT. Also curious is the fact that it does never send a WILL BINARY, though it does sent two DO BINARY

Next, I send him only WILL COMPORT

[sent: WILL COMPORT]
[received: WILL SGA]
[sent: DO SGA]
[received: WILL ECHO]
[sent: DONT ECHO]
[received: DONT ECHO]
[received: DO BINARY]
[sent: WILL BINARY]
[received: COMPORT MODEMSTATE: <cts,cd,dsr>]
[received: DO COMPORT]
[received: WILL SGA]
[received: WONT ECHO]
[received: DO BINARY]

Again no intention from ser2net to send WILL COMPORT. It does though reply with DO COMPORT and it does sent MODEMSTATE notifications---even before it replies with DO COMPORT (but I guess this isn't strictly against the standard...)

Third run, I sent it WILL COMPORT and DO COMPORT:

[sent: WILL COMPORT]
[sent: DO COMPORT]
[received: WILL SGA]
[sent: DO SGA]
[received: WILL ECHO]
[sent: DONT ECHO]
[received: DONT ECHO]
[received: DO BINARY]
[sent: WILL BINARY]
[received: DO COMPORT]
[received: WONT COMPORT]
[received: WILL SGA]
[received: WONT ECHO]
[received: DO BINARY]

Now things are clear: Although it operates as an RFC2217 access server, it replies to DO COMPORT with WONT COMPORT (which I believe is correct).

[npat-efault]

All these lead me to believe that the correct (most robust) thing to do is

• send WILL BINARY and DO_BINARY

• send WILL SGA and DO SGA

• send WILL COMPORT but do not send DO COMPORT

• reply to WILL ECHO with DO ECHO and to DO ECHO with WILL ECHO, but don't initiate negotiations ourselves about this option. EDIT: This is wrong, we should reply to DO ECHO with WONT ECHO. See comment bellow.

• reply to WILL COMPORT with DONT COMPORT (debatable)

• ignore the negotiation results for all options other than COMPORT, which should be YES on our side. Once COMPORT becomes YES on our side, set can_comport, and do comport_start.

The transaction with ser2net, with the above described settings is as follows:

[sent: WILL BINARY]
[sent: DO BINARY]
[sent: WILL SGA]
[sent: DO SGA]
[sent: WILL COMPORT]
[received: WILL SGA]
[received: WILL ECHO]
[sent: DO ECHO]
[received: DONT ECHO]
[received: DO BINARY]
[received: DO BINARY]
[received: WILL BINARY]
[received: DONT SGA]
[received: WILL SGA]
[received: COMPORT MODEMSTATE: <cts,cd,dsr>]
[received: DO COMPORT]
[received: WILL ECHO]

And the option values, end up like this (name: us him):

[opt BINARY: YES YES]
[opt SGA: NO YES]
[opt ECHO: NO YES]
[opt COMPORT: YES NO]

[npat-efault]

A test:

Two ports, connected with null-modem cable: /dev/ttyUSB0 and /dev/ttyUSB1. One port (/dev/ttyUSB0) is controlled by ser2net running on port 6666 on localhost. The default baudrate for this port is 9600 (configurable in ser2net)

First I run:

./picocom --quiet -b 19200 -x 20000 /dev/ttyUSB1 > foo.bin

And then, from another terminal, within 20sec, I run:

cat picocom | ./picocom --no-escape -b 19200 -T localhost,6666

I wait for both to finish, and do:

cmp picocom foo.bin

They match.

Then I configure ser2net to have a default baudrate of 19200 on port /dev/ttyUSB0, and I run the test again, this time the second command being:

cat picocom | ./picocom --noinit --no-escape -T localhost,6666

Again, at the end, the files match.

NOTE 1: When picocom is run with --noinit it does not wait for negotiations to finish before it starts writing user data to the remote port. When run without --noinit it does.

NOTE 2: Observe that the sending picocom has no -x command line argument given to it. It exits immediately when zero is read from stdin. This indicates that the graceful "drain on close" socket shutdown code introduced with bcc753167aad011cecd02a6625acd899070b5ad2 works well (provided that the server is correctly behaving in this regard).

[npat-efault]

Regarding negotiations, again. A correction about the handling of the ECHO option:

• We should reply to a WILL ECHO from the server with DO ECHO. It is ok for the server (or the remote service) to echo-back characters / data to us (e.g. a modem echoing what we type). It is also ok for it not to echo-back, so we do not initiate negotiations ourselves by requesting DO ECHO.
• We should reply to a DO ECHO from the server with WONT ECHO. A terminal (an access server client) never echoes-back characters / data to the server. An echo option with value YES YES indicates an endless echoing loop and should not be allowed.

[npat-efault]

Branch rfc2217 is, for the most part, ready for merging to master. Will let it wait some time for additional testing and merge it at some point the next couple of weeks...