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?

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

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.

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

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

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

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.

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.

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

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

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

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.

Mark.