Skip to content
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

implement WHOX #1184

Merged
merged 1 commit into from
Jul 12, 2020
Merged

implement WHOX #1184

merged 1 commit into from
Jul 12, 2020

Conversation

jesopo
Copy link
Contributor

@jesopo jesopo commented Jul 8, 2020

first attempt. i assume there's going to be a few issues of artistic preference and me not being a Go expert so please be brutal.

mostly used InspIRCd's WHOX implementation as a reference for this (thnx @SadieCat ❤️) but also charybdis and http://faerion.sourceforge.net/doc/irc/whox.var

much like charybdis, we do not actually support the filter part of WHOX. I really do not think it's important enough for the complexity it would add to this PR.

I've not yet implemented the o field (channel power level) because I couldn't quite figure out how I'd want to give channel status modes int values. thought I'd leave that up to a discussion,

@jesopo jesopo mentioned this pull request Jul 8, 2020
irc/handlers.go Outdated
@@ -2775,7 +2775,7 @@ func webircHandler(server *Server, client *Client, msg ircmsg.IrcMessage, rb *Re
return true
}

// WHO [<mask> [o]]
// WHO <mask> [<filteR>%<fields>,<type>]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

smol nit, filteR -> filter

@jesopo
Copy link
Contributor Author

jesopo commented Jul 8, 2020

happy to squash this before merge

@slingamn
Copy link
Member

slingamn commented Jul 8, 2020

Still reviewing but wanted to get this down before I forget: we should be publishing a new ISUPPORT token with this, right?

@slingamn
Copy link
Member

slingamn commented Jul 8, 2020

Could you post some sample input and output?

irc/server.go Outdated
flags = "G"
} else {
flags = "H"
var whoFieldMap = map[rune]WhoFields{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's what I'd actually do instead of defining these:

type WhoFields [128]bool

func (wf *WhoFields) Set(field rune) {
	wf[int(field)] = true
}

func (wf *WhoFields) Has(field rune) bool {
	return wf[int(field)]
}

i.e., just use the codepoint value of 't' or 'a' to index directly into a fixed-size array of bools. (We use similar techniques for modes.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This technique would also need bounds checking (or else you could up it to [256]bool and operate on bytes instead of runes) but it's still probably worth it.

@jesopo
Copy link
Contributor Author

jesopo commented Jul 8, 2020

here's some example input and output

> WHOIS jesss
< :oragono.test 311 jesss jesss jess localhost * :the cat
< :oragono.test 338 jesss jesss jess@localhost 0::1 :Actual user@host, Actual IP
< :oragono.test 671 jesss jesss :is using a secure connection
< :oragono.test 317 jesss jesss 9 1594234687 :seconds idle, signon time
< :oragono.test 318 jesss jesss :End of /WHOIS list

> WHO jesss
< :oragono.test 352 jesss * jess localhost oragono.test jesss H :0 the cat
< :oragono.test 315 jesss jesss!*@* :End of WHO list

> WHO jesss %tcuihsnfdlaor,111
< :oragono.test 354 jesss 111 * jess 0::1 localhost oragono.test jesss H 0 24 0 0 :the cat
< :oragono.test 315 jesss jesss!*@* :End of WHO list

> NS REGISTER hunter2
< :NickServ!NickServ@localhost NOTICE jesss :Account created
< :NickServ!NickServ@localhost NOTICE jesss :You're now logged in as jesss

> WHO jesss %tcuihsnfdlaor,111
< :oragono.test 354 jesss 111 * jess 0::1 localhost oragono.test jesss H 0 49 jesss 0 :the cat
< :oragono.test 315 jesss jesss!*@* :End of WHO list

> AWAY :i am away
< :oragono.test 306 jesss :You have been marked as being away

> WHO jesss %tcuihsnfdlaor,111
< :oragono.test 354 jesss 111 * jess 0::1 localhost oragono.test jesss G 0 64 jesss 0 :the cat
< :oragono.test 315 jesss jesss!*@* :End of WHO list

> JOIN #test
< :jesss!jess@localhost JOIN #test
< :oragono.test 353 jesss = #test :@jesss
< :oragono.test 366 jesss #test :End of NAMES list

> WHO #test %tcuihsnfdlaor,111
< :oragono.test 354 jesss 111 #test jess 0::1 localhost oragono.test jesss G@ 0 83 jesss 0 :the cat
< :oragono.test 315 jesss #test :End of WHO list

Copy link
Member

@slingamn slingamn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I'll leave this open for a bit so Dan (and possibly other people) can eyeball the sample output.

irc/server.go Outdated
fAccount := "0"
if target.accountName != "*" {
// WHOX uses "0" to mean "no account"
fAccount = target.accountName
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

details.accountName

irc/server.go Outdated
params = append(params, details.nick)
}
if fields.Has('f') { // "flags" (away + oper state + channel status prefix + bot)
flags := ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe switch this to strings.Builder while you're in the neighborhood?

@jwheare
Copy link

jwheare commented Jul 8, 2020

I was asked to review this but my go fluency is poor. Probably best if this gets shoved onto a testnet somewhere and I'll exercise the implementation.

@slingamn
Copy link
Member

slingamn commented Jul 8, 2020

I put this up on my stage: files.stronghold.network, TLS on port 6697.

@jesopo
Copy link
Contributor Author

jesopo commented Jul 8, 2020

bitbot and ircstates seem to be happy about it, though the former only pulls nuhra and the latter only pulls nuhraf

@jwheare
Copy link

jwheare commented Jul 9, 2020

Behaviour of this looks fine and correct from a cursory check with /WHO #chan %tcuihsnfdlaor,123 calls

@slingamn
Copy link
Member

slingamn commented Jul 9, 2020

@jesopo when you get a chance, can you:

  1. Move rplWhoReply, WhoFields, and the new constants into irc/handlers.go, directly above whoHandler? The current location in irc/server.go is for confusing historical reasons
  2. Squash?

@slingamn slingamn merged commit bdfee9c into ergochat:master Jul 12, 2020
emersion added a commit to emersion/soju that referenced this pull request Nov 2, 2021
This adds support for WHOX, without bothering about flags and mask2
because Solanum and Ergo [1] don't support it either.

The motivation is to allow clients to reliably query account names.

It's not possible to use WHOX tokens to route replies to the right
client, because RPL_ENDOFWHO doesn't contain it.

[1]: ergochat/ergo#1184

Closes: https://todo.sr.ht/~emersion/soju/135
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants