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

> 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);)

(SecureServ and statserv are currently loaded as well, so we should also see
those various debug messages being printed out)


> > Firstly, when I was debugging why SecureServ/StatServ were 
> > not receiving CTCP version replies, I followed the codepath 
> > into services.c:
> > 
> > Now, as far as I can tell, services_event_ctcpversion was 
> > never getting called anyway, and when I looked at that code, 
> > if it was going to get called, I could see it end up 
> > recursively sending the CTCPVERSIONRPL event over and over again.
> 
> It is called. init_services_bot adds the event list to the 
> NeoStats bot. The
> NeoStats bot processes it through this function to send all 
> module events.
> 

Its not called. I suspect its got something to do with the runlevel system,
because I couldn't get the exculsion databases to close correctly using
closetable. It was something I was going to look into further but havn't got
to it yet.


> I do not see why you think it would get called recursively 
> since it has been
> running like this for a long time and in production on my network
> successfully. It is triggered once for one event. The initial 
> event process
> looks up the target user, finds the NeoStats bot and uses the 
> core dummy
> module to trigger a single event. The subsequent calls to 
> SendAll will not
> find the NeoStats bot since it is not in the module list.
> 

Ok, point taken, but my first statement still stands. This is redundant
anyway. The copy of the version string to Client->version could be done in
the core when it decodes the CTCP reply, rather than call a Event, copy it,
and then call the same event, but this time a SendAll event, which is
basically what my code did in the firstplace?


> > Secondly, I was confused as to why the core was configured 
> for events.
> > Seeing as the CORE doesn't need to use the module API, this 
> > didn't make sense, hence why not change all CTCP replies to 
> > SendAll, and save a few calls to SendModuleEvent in the process. 
> 
> The core is configured to use events to save repeating the 
> code in multiple
> places or having to hack in an additional code path for the 
> core to pick up
> certain information. The core sharing the module API is an exercise in
> removing redundancy and bloat and simplifying the command 
> paths since the
> core and modules share them. 
> 

But in this case, nothing in the NeoStats bot actually does anything with
CTCP events. It copies it to client->version. Why can't the client->version
copy be done in ctcp.c and save a event call?


> > Anyway, without a new API just to deal with CTCP info, I 
> > don't see any other way to have *any* module, other than the 
> > core, receive CTCP replies without a SendAll call.
> 
> The current API is for a module to selectively receive a CTCP reply in
> response to a request and handle the parsing of the reply 
> received. For
> example, some bots change their version reply for each 
> request. While the
> initial response might be useful to multiple modules, the 
> subsequent calls
> should be handled by a module that manages this only not distrbuted
> throughout NeoStats. 
> 
> For a module to receive a reply to a request it makes, it issues the
> request, the core parses the reply and sends it to that 
> module only since
> the replay target identifies the calling module.
> 

See below then.

> > > Since the version request is currently used by StatServ and 
> > > SecureServ, it made sense for the core to make a single 
> > request which 
> > > statserv and secureserv then receive via an event. There seems no 
> > > point having support in SecureServ in case it is switched 
> > off in the 
> > > core. It seems needless repetition and an additional place 
> > we have to 
> > > maintain in the event of a problem with it.
> > > 
> > 
> > I don't like NeoStats (the bot) sending out CTCP requests. 
> > Personal preference here, but I don't want normal users to 
> > ever know about NeoStats at all. If SecureServ or StatServ 
> > need version requests, then one of them should send the 
> > request. (I thought about having the server send the request, 
> > but I don't know about compatibility with other IRCd's 
> > there). I didn't want to remove the functionality of NeoStats 
> > sending the request out, so I expanded it anyway.
> 
> I think it should come from the server as well. Currently 
> there are a lot of
> discrepancies between what is sent from the server and what 
> is sent from a
> bot that really need resolving throughout NeoStats.
> 
> Under the 2.5.x system a user must run SecureServ for StatServ to get
> version information. Having the core handle it removes this 
> limitation while
> allowing any module to access the core CTCP reply information 
> without having
> to issue it. I do not think that a module should require 
> another module to
> be loaded for something such as this. I also do not think a 
> module should be
> able to affect the run time of another module.
> 

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

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?

If in future a module comes alone that's interested only in CTCP replies for
a particular user, then let them write a ImInterested(cmdparams->client)
function and run that when they receive a CTCP reply


> > The other thing, is that even I was confused at the start in 
> > how to get the CORE to even issue a version request. To me, 
> > "/msg secureserv set versionscan on" should have made it 
> > happen... Then I had to go to the core and say hey, turn on 
> > version scanning as well... That just seemed overly 
> > complicated, and in the future, I'm sure would be a support issue. 
> 
> The version scan has been in the core from the early days of 
> 3.0. The "scan"
> in SecureServ is to look for trojans in the text. Assuming 
> that 3.0 works
> the same as 2.5.x is the problem, not the implementation, 
> which is something
> documentation should address. 
> 

Not scan, versionscan. You have to enable versionscan in SecureServ and
versionscan in NeoStats for CTCP scans to work in the original
implementation. 

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 is no more "overly complicated" than to get version stats 
> in 2.5.x you
> must install secureserv and enable version scanning in it 
> even if you do not
> want SecureServ or SecureServ version checking which has been 
> an ongoing
> support issue.
> 

That's because its 2.5, and CTCP requests was a "add-on" when SecureServ
came along. The StatServ support ended up as a feature request (because
Sentinal or whatever else statserv package could do that). 


> 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.

> > > If, and only if, there is a need for multiple modules to 
> > wish to share 
> > > the CTCP "chatter" then we ought to add more core side 
> options, not 
> > > start having modules trigger events that are likely to have 
> > an adverse 
> > > effect on modules which do not expect interference by other 
> > modules. 
> > > Given we have never supported any CTCP "chatter" other than 
> > VERSION in 
> > > the past, it is unlikely we have an immediate need for it 
> > which is the 
> > > reason this was not considered in the initial API other than for 
> > > VERSION.
> > > 
> > 
> > 
> > Well, all VERSION replies are handled by both modules now, 
> > and they don't do any filtering on processing replies (apart 
> > from Exclusions of course.) 
> 
> Yes because the current VERSION reply is a known quantitiy 
> coming from a
> core scan or from a scan initiated by the affected module. By all CTCP
> chatter being broadcast, modules would have to be modified to 
> filter this
> chatter to avoid adverse effects since it is no longer a 
> known quantity.
> 

Then let them. If someone goes and writes a new module now that starts
making its own CTCP version requests while having SecureServ or NeoStats
versionscans enabled, then that module writer is a *idiot*. Pure and simple.
They even have access to the client version in the Client struct (ignoring
the current bug with the services.c event call)


> > 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 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.



> > 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?


> 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?)

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?

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. 

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.
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?