New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Telnet: Write full buffer instead of byte-by-byte #1389

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@richardhsu
Contributor

richardhsu commented Apr 4, 2017

Previous TODO wanting to write in chunks. We should support writing more
at once since some TELNET servers may respond immediately upon first
byte written such as WHOIS servers.


Was attempting to use CURL to connect to WHOIS servers and found that certain servers didn't work well with send_telnet_data sending byte-by-byte. Saw TODO so attempted to do it.

Have the following concerns:

  • TODO states to do chunk by chunk, not sure what a standard practice may be for the sizing so for initial PR ended up making a full escaped out buffer and just let the Curl_write send as much as possible and loop until completion.
  • Not sure how to write tests for this but ran existing tests which all passed and ended up testing a local C program to use CURL against godaddy's WHOIS server which doesn't work until this patch is applied. If need tests, any advice on that would be appreciated!

Thanks

@mention-bot

This comment has been minimized.

mention-bot commented Apr 4, 2017

@richardhsu, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagder, @yangtse and @dfandrich to be potential reviewers.

@bagder

This comment has been minimized.

Member

bagder commented Apr 10, 2017

We don't have any TELNET test server so the only possible way right now, without adding such a test server, is probably to send a HTTP 1.0 request via TELNET to the HTTP test server and make sure that works as expected.

@bagder

bagder approved these changes Apr 10, 2017

I think the change looks fine. The double looping over the contents is unfortunate, but I can't see an easy way to avoid that.

@richardhsu

This comment has been minimized.

Contributor

richardhsu commented Apr 12, 2017

@bagder thanks! Also added a test1399 to do normal TELNET stdin to the HTTP test server :)

@richardhsu

This comment has been minimized.

Contributor

richardhsu commented Apr 14, 2017

@bagder looks like checks and tests pass, let me know if I should add anything more in order to get this merged, thank you!

I've also been running this on my local machine for my use case and it is working as expected.

@bagder

This comment has been minimized.

Member

bagder commented Apr 15, 2017

My gcc complains since there's a comparison of a signed character with 255 (CURL_IAC) that will never equal true:

telnet.c: In function ‘send_telnet_data’:
telnet.c:1232:18: error: comparison is always false due to limited range of data type [-Werror=type-limits]
     if(buffer[i] == CURL_IAC)
                  ^~
telnet.c:1246:20: error: comparison is always false due to limited range of data type [-Werror=type-limits]
       if(buffer[i] == CURL_IAC)
                    ^~
In file included from telnet.c:59:0:
arpa_telnet.h:77:19: error: overflow in implicit constant conversion [-Werror=overflow]
 #define CURL_IAC  255 /* Interpret As Command */
                   ^
telnet.c:1247:23: note: in expansion of macro ‘CURL_IAC’
         outbuf[j++] = CURL_IAC;
                       ^~~~~~~~
@richardhsu

This comment has been minimized.

Contributor

richardhsu commented Apr 17, 2017

@bagder Ah, yes, thanks, updated the outbuf to be unsigned char * and then added casts during comparisons between buffer and the various unsigned chars.

@richardhsu

This comment has been minimized.

Contributor

richardhsu commented Apr 24, 2017

@bagder Hi, was wondering if there's any other issues before getting this merged? Thanks :)

@bagder

This comment has been minimized.

Member

bagder commented Apr 25, 2017

I still see two compiler warnings/errors (./configure --enable-debug adds picky options):

telnet.c: In function ‘send_telnet_data’:
telnet.c:1237:12: error: pointer targets in assignment differ in signedness [-Werror=pointer-sign]
     outbuf = buffer;
            ^
telnet.c:1246:20: error: comparison is always false due to limited range of data type [-Werror=type-limits]
       if(buffer[i] == CURL_IAC)
                    ^~
Telnet: Write full buffer instead of byte-by-byte
Previous TODO wanting to write in chunks. We should support writing
more at once since some TELNET servers may respond immediately upon
first byte written such as WHOIS servers.
@richardhsu

This comment has been minimized.

Contributor

richardhsu commented Apr 26, 2017

@bagder Ah awesome, thanks, the --enable-debug was helpful, missed the two other casts needed but have now corrected them. Thanks!

@bagder

bagder approved these changes May 2, 2017

@bagder bagder closed this in 862b02f May 2, 2017

@bagder

This comment has been minimized.

Member

bagder commented May 2, 2017

thanks!

@richardhsu richardhsu deleted the richardhsu:richardhsu/telnet-write-all branch May 2, 2017

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented May 8, 2017

The new test hangs infinitely on MinGW, also all the MinGW autobuilds have been failing since then. I haven't found time to debug this yet.

@richardhsu

This comment has been minimized.

Contributor

richardhsu commented May 9, 2017

Oh hmm, sorry about that. Looks like 9e093f0#diff-7b143a8d3555776816bc29d9cb83e6a2 actually addresses such issue I think. I believe then the test1399 added isn't needed since test1326 tests TELNET behavior which that test already passes since the suite has been passing.

@lock lock bot locked as resolved and limited conversation to collaborators May 14, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.