change port to 23 instaid of 5050

[fennectech]

The reason for this is so that users can connect without specifying the port. They must simply type the IP of their device as 23 is default port for telnet.

[friedkeenan]

When I tried this I got an "Error: command not found" message right when I joined, using the telnet command on Ubuntu. Probably related to #1

[fennectech]

i dont know how this could be related to changing the listening port.

Does it happen without this patch?

[fennectech@Jasper ~]$ telnet 10.0.0.24 Trying 10.0.0.24... Connected to 10.0.0.24. Escape character is '^]'.

Welcome to nxsh! Type 'help' for a list of available commands.

Enter password: Incorrect password entered Enter password: root@switch:/ # exit Connection closed by foreign host.

This is caused by the client sending stuff.

[friedkeenan]

Yes, I have telnet. That's how I was able to use the telnet command.

Maybe it was because I added the changes from the latest commit, but when I used port 5050 with those changes I did not get the command not found message.

[fennectech]

I do not see that if i manualy specify the port on telnet. SO telnet is definitly behaving differently and this isnt an issue related to me changing the port.

[fennectech]

@Cesura how did you fix #1 in the beginning?

[friedkeenan]

This seems to be the relevant commit

[fennectech]

The linux version of telnet sends ÿýÿûÿûÿû ÿû!ÿû"ÿû'ÿýÿû# if not port is not passed.

[friedkeenan]

A lot of that stuff nxsh doesn't really need. It seems most of those commands start with 0xFF, maybe you could check to see if command starts with that, and if so, don't put it through the normal command logic? Later if we want to implement any of those commands we can put an else corresponding to the if the check goes in and shove the logic in the else block

[friedkeenan]

I think it would be nice to not exactly hardcode the port, but to turn it into a define. Could you put #define NXSH_PORT 23 around here (with appropriate newlines) and then change the two places where you have 23 into NXSH_PORT

[Cesura]

While 5050 was a fairly arbitrary port and I admit could have been better chosen, I strongly disagree with the decision to change it to 23. Despite what the project description says (and I really should change this), nxsh is not a telnet server and does not even begin to implement the telnet standards as defined by RFC 495 (the fact that the shell is receiving data from telnet clients and responding erratically is fairly indicative of that). The decision to suggest telnet as a means of connecting was made because it's convenient and familiar to most users; a raw socket is still technically the preferred method of connection.

As for the spaces vs. tabs thing, while I'm not particularly concerned, I tend to prefer the latter. It's nice to allow people downloading and viewing the source code to set the tab size to their preferences, rather than force a style by using spaces.

Just my $0.02. Also, my Switch is operational again so I expect to dive back into this soon.

[friedkeenan]

I personally think the #define NXSH_PORT should stay, but if you want it to be 5050 that's fine with me. As for the specific logic that this PR added for the telnet protocol (checking if the first byte of recv_buf is 0xFF), I think it would be good to have that somewhere just in case we ever want to add logic for that.

For spaces vs. tabs, it ended up happening because I noticed there were tabs mixed in with spaces in the code @fennectech added, and I asked him to change them to spaces (we were talking on discord). I then realized there were tabs mixed with spaces throughout the code, and I figured it would be good to standardize them. I personally prefer spaces, as they give consistency in how people see the code, any editor worth its salt should be able to put four spaces when you want to indent, making them as easy as tabs, and the tabs are unusually wide when you look at them in GitHub, which gets very mucky when you have tabs mixed with spaces, like we do in this repo. I get that tabs offer more customizability, but I think most people would agree 4 spaces are a good width for indents, but of course if you think tabs are better I'm fine with switching to that.

[fennectech]

The idea was so that the user wouldent have to specify the port when connecting. Are our goals to make a telnet server or are we going after something else

[Cesura]

I agree with the define, just not 23 as the default port, at least until we decide to fully implement the telnet standard in a way that most clients are expecting. That's a conversation I'm certainly willing to have, but it involves a decent amount of work to achieve.

I also like 4 as a tab width, but as a setting in my editor. There are browser extensions that allow you to set how tabs are rendered, if the Github formatting is bothersome. Using spaces dictates to the end user how they should view the source, and I personally can't get behind that. Let's not get caught up in trivial things though, as there are many more exciting features to implement ahead. :)

[fennectech]

I can programmatically change every instance of 4 spaces to a tab

On Sat, Mar 30, 2019 at 6:31 PM Cesura notifications@github.com wrote:

I agree with the define, just not 23 as the default port, at least until we decide to fully implement the telnet standard in a way that most clients are expecting. That's a conversation I'm certainly willing to have, but it involves a decent amount of work to achieve.

I also like 4 as a tab width, but as a setting in my editor. There are browser extensions that allow you to set how tabs are rendered, if the Github formatting is bothersome. Using spaces dictates to the end user how they should view the source, and I personally can't get behind that. Let's not get caught up in trivial things though, as there are many more exciting features to implement ahead. :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Cesura/nxsh/pull/14#issuecomment-478298400, or mute the thread https://github.com/notifications/unsubscribe-auth/ALZmAbayMqOcffHkkMOgLUO1jQhQpJigks5vb_PqgaJpZM4cM6fq .

-- FennecTECH

[friedkeenan]

Ok, so I guess we can merge the revert-14-master branch and then @fennectech can make another PR, adding the stuff for the NXSH_PORT define and the switching all tabs to spaces

[fennectech]

Okay. So. Do we want tabs or spaces.

[fennectech]

I can also push a commit reversing the changes we made setting the port to 5050

[fennectech]

It’d be less work to undo the changes on master as it is ive already got the code in my own fork im trying to figure out the PR button.