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

Tcl getuser command submits botaddr ports in wrong order #459

Closed
vanosg opened this Issue Sep 4, 2017 · 7 comments

Comments

Projects
None yet
2 participants
@vanosg
Collaborator

vanosg commented Sep 4, 2017

Documentation implies (doesn't actually state) the order for changing a botaddr with setuser is address, telnet port, bot port:

    BOTADDR  returns a list containing the bot's address, telnet port, and
             relay port (bot-only)

But it appears it reverses user and bot ports when you submit it

> .tcl setuser ztestbot botaddr 1.1.1.1 2222 3333
>  tcl: builtin dcc call: *dcc:tcl vanosg 9 setuser ztestbot botaddr 1.1.1.1 2222 3333
>  tcl: evaluate (.tcl): setuser ztestbot botaddr 1.1.1.1 2222 3333
> Tcl: 
> .whois ztestbot
> HANDLE                           PASS NOTES FLAGS           LAST
> ztestbot                         yes      0 b               2017-08-23 (unlinked)
>   ADDRESS: 1.1.1.1
>      users: 3333, bots: 2222
>   BOT FLAGS: gs

Recommend adjusting documentation for 1.8.3, and discussing the merits of "fixing" this in 2.0, as it would be a non-backward compatible change to the API. (Reminder to self, if we do that, please submit a new issue for that and tag it 2.0 before we close this one).

@vanosg vanosg added this to the v1.8.3 milestone Sep 4, 2017

@Cizzle

This comment has been minimized.

Show comment
Hide comment
@Cizzle

Cizzle Sep 4, 2017

Member

I think clarifying the docs is enough. IMO there's nothing to fix codewise.

Member

Cizzle commented Sep 4, 2017

I think clarifying the docs is enough. IMO there's nothing to fix codewise.

@vanosg

This comment has been minimized.

Show comment
Hide comment
@vanosg

vanosg Sep 4, 2017

Collaborator

For bonus consideration points,

setuser bot botaddr 1.2.3.4 5555

sets only the bot port (not the user port). I'm ok with modifying docs, but just want to point out how non-standard this behavior is when compared to the +bot syntax, where specifying a single port sets bot bot and user.

I'm on a standardization kick this month, it seems 🔑 🌱 🤳

Collaborator

vanosg commented Sep 4, 2017

For bonus consideration points,

setuser bot botaddr 1.2.3.4 5555

sets only the bot port (not the user port). I'm ok with modifying docs, but just want to point out how non-standard this behavior is when compared to the +bot syntax, where specifying a single port sets bot bot and user.

I'm on a standardization kick this month, it seems 🔑 🌱 🤳

@vanosg

This comment has been minimized.

Show comment
Hide comment
@vanosg

vanosg Sep 7, 2017

Collaborator

Re-reading the help, you're right, the code is fine. I'll update the docs and close this out shortly.

Collaborator

vanosg commented Sep 7, 2017

Re-reading the help, you're right, the code is fine. I'll update the docs and close this out shortly.

@vanosg vanosg self-assigned this Sep 7, 2017

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Sep 21, 2017

In the past, setting only the 1st port would set both bots & users ports. In a change somewhere in 1.6 branch, it only set the bot port (users remained 3333 or some other default). Which is the intended behavior?

ghost commented Sep 21, 2017

In the past, setting only the 1st port would set both bots & users ports. In a change somewhere in 1.6 branch, it only set the bot port (users remained 3333 or some other default). Which is the intended behavior?

@vanosg

This comment has been minimized.

Show comment
Hide comment
@vanosg

vanosg Sep 24, 2017

Collaborator

Working on this now - bot foo is added like this:

foo                              no       0 b               never (nowhere)
  ADDRESS: 1.1.1.1
     users: 5555, bots: 4444

Doing a chaddr with only one port argument changes both bot and user ports:

.chaddr foo 1.1.1.1 3333
foo                              no       0 b               never (nowhere)
  ADDRESS: 1.1.1.1
     users: 3333, bots: 3333

Using the Tcl command with only one port argument changes only the bot listening port:

.tcl setuser foo botaddr 1.1.1.1 4444
foo                              no       0 b               never (nowhere)
  ADDRESS: 1.1.1.1
     users: 3333, bots: 4444

To standardize this between chaddr and the associated Tcl setuser command, I think it makes sense to implement this the way that chaddr does, where one argument changes both values, and two arguments allows you to set them individually. Comments/Improvements?

Collaborator

vanosg commented Sep 24, 2017

Working on this now - bot foo is added like this:

foo                              no       0 b               never (nowhere)
  ADDRESS: 1.1.1.1
     users: 5555, bots: 4444

Doing a chaddr with only one port argument changes both bot and user ports:

.chaddr foo 1.1.1.1 3333
foo                              no       0 b               never (nowhere)
  ADDRESS: 1.1.1.1
     users: 3333, bots: 3333

Using the Tcl command with only one port argument changes only the bot listening port:

.tcl setuser foo botaddr 1.1.1.1 4444
foo                              no       0 b               never (nowhere)
  ADDRESS: 1.1.1.1
     users: 3333, bots: 4444

To standardize this between chaddr and the associated Tcl setuser command, I think it makes sense to implement this the way that chaddr does, where one argument changes both values, and two arguments allows you to set them individually. Comments/Improvements?

@Cizzle

This comment has been minimized.

Show comment
Hide comment
@Cizzle

Cizzle Sep 24, 2017

Member

I agree.

Member

Cizzle commented Sep 24, 2017

I agree.

@vanosg

This comment has been minimized.

Show comment
Hide comment
@vanosg

vanosg Sep 25, 2017

Collaborator

Ok. Part 2- can anyone come up with a reason to say this changing this is not backwards compatible (and thus belongs in 2.0)? While technically it changes functionality that could break existing scripts, every use case I can think of doesn't seem plausible that a script writer would use it this way for, but that's not my thing. On the other hand, I'm not comfortable calling this a 'bug', as it appears to be intentional coding on both commands, just not synched.

I really want to do this now, but I can't get away from believing this is a 2.0 thing. Can someone convince me otherwise?

@maimizuno likely has some thoughts on this

Collaborator

vanosg commented Sep 25, 2017

Ok. Part 2- can anyone come up with a reason to say this changing this is not backwards compatible (and thus belongs in 2.0)? While technically it changes functionality that could break existing scripts, every use case I can think of doesn't seem plausible that a script writer would use it this way for, but that's not my thing. On the other hand, I'm not comfortable calling this a 'bug', as it appears to be intentional coding on both commands, just not synched.

I really want to do this now, but I can't get away from believing this is a 2.0 thing. Can someone convince me otherwise?

@maimizuno likely has some thoughts on this

@vanosg vanosg referenced this issue Oct 9, 2017

Merged

Bug/botaddrdocs #469

@vanosg vanosg closed this in #469 Oct 21, 2017

vanosg added a commit that referenced this issue Oct 21, 2017

Fix tcl setuser botaddr field docs and functionality. Fixes #459
* Standardize setuser botaddr syntax with chaddr
.chaddr syntax changes both user and bot port if only one port is specified. .tcl setuser botaddr
syntax changed only the bot port if one port was entered. This patch standardize to the chaddr behavior
* Reset port to non-SSL when adding a non-SSL port with .tcl setuser
* Clarify BOTADDR docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment