RE: [NeoStats-Devel] [Commits] r2699 - in trunk: include src

Justin Hammond wrote:
> > Justin Hammond wrote:
> > > Firstly, the CTCP version reply code is currently broken with a 
> > > SendModuleEvent code, and if it was fixed in its current 
> > > implementation, I fear we have another problem anyway.
> > 
> > How is it broken?
> > 
> 
> With the dlog statement I just put in the code, you can see 
> that the event never gets called for the neostats bot:
> DEBUG1 NeoStats - ------------------------BEGIN
> PARSE-------------------------
> DEBUGRX NeoStats - RX: :Fish NOTICE NeoStats :VERSION Visual IRC 2.0
> (English) - Fast. Powerful. Free. 
> http://www.visualirc.net/features.php
> DEBUG1 NeoStats - origin: Fish
> DEBUG1 NeoStats - cmd   : NOTICE
> DEBUG1 NeoStats - args  : NeoStats :VERSION Visual IRC 2.0 
> (English) - Fast.
> Powerful. Free. http://www.visualirc.net/features.php
> DEBUG3 NeoStats - process_ircd_cmd: running command NOTICE
> DEBUG1 NeoStats - _m_notice: from Fish, to NeoStats : VERSION 
> Visual IRC 2.0
> (English) - Fast. Powerful. Free. 
> http://www.visualirc.net/features.php
> DEBUG1 NeoStats - UserLevel for Fish is 200
> DEBUG5 NeoStats - RX: CTCP VERSION reply from Fish to NeoStats
> DEBUG1 NeoStats - -------------------------END
> PARSE--------------------------
> 
> (it should print a dlog(DEBUG1, "Got Version reply event in 
> services.c from
> %s: %s", cmdparams->source->name, cmdparams->param);)

14/08/2005[15:30:35] DEBUG1 NeoStats - ------------------------BEGIN
PARSE-------------------------
14/08/2005[15:30:35] DEBUGRX NeoStats - RX: :M B NeoStats :
VERSION mIRC
v6.16 Khaled Mardam-Bey

14/08/2005[15:30:35] DEBUG1 NeoStats - origin: M
14/08/2005[15:30:35] DEBUG1 NeoStats - cmd   : B
14/08/2005[15:30:35] DEBUG1 NeoStats - args  : NeoStats :
VERSION mIRC v6.16
Khaled Mardam-Bey

14/08/2005[15:30:35] DEBUG3 NeoStats - process_ircd_cmd: running command
NOTICE
14/08/2005[15:30:35] DEBUG1 NeoStats - _m_notice: from M, to NeoStats :

VERSION mIRC v6.16 Khaled Mardam-Bey

14/08/2005[15:30:35] DEBUG1 NeoStats - UserLevel for M is 0
14/08/2005[15:30:35] DEBUG5 NeoStats - RX: CTCP VERSION reply from M to
NeoStats
14/08/2005[15:30:35] DEBUG1 NeoStats - Running module NeoStats with event 35
14/08/2005[15:30:39] DEBUG1 NeoStats - Got Version reply event in services.c
from M: mIRC v6.16 Khaled Mardam-Bey
14/08/2005[15:30:39] DEBUG1 NeoStats - -------------------------END
PARSE--------------------------

Like that?

As I said, if it is not working in your environment, that is a bug to fix. 

> The argument about 2.5 is moot... This isn't 2.5, but for 
> argument sake,
> lets follow this trail:

It was to demonstrate why the change was made and the fact complexity was
not a new issue and has improved over the previous!

> So assuming you can fix the problem with the event being 
> triggered in the
> core, but I want SecureServ to send the CTCP request rather 
> than NeoStats,
> then I'll have to put up with StatServ remaining broken with 
> regards to
> Client Version replies, and I'll have to tell users that sorry, not
> supported?. For sure I can't call a SendAll event from 
> SecureServ so that
> StatServ can get it, because that would end up being recursive
> 
> I honestly don't see what the big deal is about here. A 
> SendAll event gives
> us a lot more flexibility right now rather than writing a 
> whole new API just
> to deal with CTCP replies?

You appear to be ignoring the fact that the SendAll means a module can
affect all other modules. Think how many attempts were made in 2.5.x to fix
SecureServ getting privmsgs from other modules. The 3.0 core was designed to
stop this random sending of events to the wrong module.

> But this goes back to my previous point. I *DON'T* want 
> NeoStats sending
> CTCP requests, so without my checks in SecureServ, I'm screwed. Same
> argument goes if I was just running StatServ, I don't like it 
> coming from
> NeoStats.
> 
> ("I don't" is personal preference, it might not bother you, 
> but it does
> bother me)

It bothers me in that I would prefer it to come from the server. I don't
wan't any of the NeoStats bots to talk to users in this way. Since I cannot
have this, I don't care which bot is exposed.

There are many areas of Neostats I am unhappy with but am unable to change
due to your perssonal preference.

> > There is a potentially simple solution here. SecureServ could 
> > "enable" the
> > core scan if desired. I have a module flags field which is 
> > not currently
> > widely used and we can simply flag a module as wanting the 
> > results of a core
> > scan and enable the option. This would remove the need for 
> > SecureServ to run
> > the scan. 
> > 
> > It would however not resolve the sender field issue you have.
> > 
> 
> To me, that's sounds like a lot more than just changing to 
> SendAll, and as
> you point out, only fixes half the problem.

It is very little work. The sender issue is also resolvable.

> > > Without a SendAll event, 
> > > SecureServ and StatServ are broken. 
> > 
> > They work perfectly well for me. If they are not working for 
> > you in the
> > current system, that is a bug that needs addressing. The fix 
> > is not to hack
> > an API designed for a specific purpose to do something else.
> > 
> 
> HACK? For fucks sake Mark. Why is it a HACK? WHY the hell are 
> you so against
> a 3 letter change 

It is not a "3 letter change", it is a functional change that affects the
API and event system.

Changing an API unrelated to the issue will not fix it hence it is a hack if
that is the basis for it.

> that adds a lot more flexibility instead of 
> complexity
> with a new/re-written API? So what if a module writter has to 
> filter out
> what he is interested in, most modules have to do that 
> already with regards
> to channel code etc. for instance, LimitServ.

It goes against the whole design of the core. To allow a module to chat to a
user without affecting other modules or users. Everything that a module
triggers than creates a response is filtered for that module. Why make the
CTCP system work completely differently?

> > > Why don't we just bite 
> > > the bullet and and make the replies a SendAll Event, and put 
> > > a CTCP API in the roadmap for 3.1 timeframe? 
> > 
> > A module should not expect to be affected by other modules 
> > and should not
> > expect to affect them adversely. The API as developed is 
> pointless in
> > SendAll form since a module is likely to trigger unexpected 
> > events in other
> > modules. As I said, if a SendAll form is desirable, it should 
> > be coded as
> > the core version scan so it is a fixed and known risk and 
> > effect. Such an
> > implementation is trivial. 
> > 
> 
> Then I could take that argument to things like LimitServ etc, 
> saying why
> should It have a event triggered for a channel its not monitoring?

Different issue entirely. Channel events are sent from the IRCd whether we
ask for them or not. Events that a module triggers for it's own use are
simply sharing the event path in order to avoid the need for lots of module
symbols that the core looks up to issue calls.

> > We have a CTCP API now so do not need to postpone it to 3.1. 
> > 
> > > Right now, I 
> > > accept your argument that some modules in the future might 
> > > only want to see replies from certain users, but I can't 
> > > think of what those modules might be. 
> > 
> > Not a single portion of this API is used by anything in 
> > NeoStats or official
> > modules at present since it is a new set of routines, I use 
> > the API in a
> > custom module myself which does multiple CTCP version 
> > requests under certain
> > conditions but relies on the fact that it can do so without 
> triggering
> > additional stats in StatServ or triggering additional scans 
> > in SecureServ.
> > If the API did a sendall, I could no longer issue version 
> > request via the
> > API since it would have adverse effects on StatServ with 
> > erroneous stats and
> > SecureServ with additional scans. 
> > 
> > Any module which ever issued a CTCP version reply using a 
> > SendAll system,
> > would trigger additional SecureServ scans and erroneous 
> > StatServ stats. I
> > maintain that a SendAll must only be issued by the core so 
> > that it is a
> > known quantity rather than modules having to assume that any 
> > other loaded
> > module can generate responses to it and filter all messages 
> > to avoid adverse
> > effects.
> > 
> 
> 
> Your own code adds the version reply to the client structure, 
> so why can't
> you use that information instead? (although one enhancement I would
> recommend is make the client versions a array of strings 
> because of those
> scripts that send additional CTCP version replies?)

I agree with the multiple extension, but I do not see a way to determine
whether it is multiple line or simply a subsequent send which is why it was
never done. I have a potential solution to this.

> So your basically saying to me, Screw you, I don't want to support the
> flexibility of another Bot sending the version requests, 
> because it would
> mean that it will affect a unreleased module I have written?

No, I am using my module as an example since you could not forsee any issues
arising.

> Fine, go and write a new fucking API that makes me happy and 
> gives me the
> flexibility I also want, and I'll shutup. If you 
> can't/don't/won't then it
> will have to be a SendAll event. 

Well that is encouragement. 

> 
> Your call.
> 
> Justin.
> 
> P.S. This frustrates the hell out of me. Basically your whole 
> email has told
> me why it's the way it is, but hasn't told me anything about 
> how to fix it.

I explained in depth why the system is the way it is and that as far as I
knew there wasno bug and that if there was the bug needs fixing rather than
changing something that is designed for another purpose. It is *THAT*
simple.

If there are bugs I am not aware of (the lack of people using 3.0 due to the
fact releases were broken contributing to lack of bug reports) I am
obviously not going to fix them. Since I run 3.0 and have not experienced
problems, obviously I am at a loss when you make a post saying something is
broken with no supporting evidence or helpful information. 

> Sorry if I sound harsh, but I'm trying to fix bugs here, and 
> your reversing
> the changes saying its not a bug. What the hell am I meant to feel?

How do you think I feel when you make a change that would not address any
potential bug and claim something is broken but provide no information on
it. Your change did nothing to fix the potential bug, it hacked an API to
work around it.

You will notice I left your change in there expecting it would end up being
forced your way anyway but allowing code I have written to continue to work
using the API as designed while explaining in depth why I feel the API
should remain as designed.

Mark.