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

> 
> > From: [email protected] [mailto:[email protected]] 
> > Author: Fish
> > Modified: trunk/src/ctcp.c
> > -	SendModuleEvent( EVENT_CTCPVERSIONRPL, cmdparams, 
> > cmdparams->bot->moduleptr );
> > +	/* because all modules might be interested */
> > +	SendAllModuleEvent( EVENT_CTCPVERSIONRPL, cmdparams);
> >  	return NS_SUCCESS;
> 
> The CTCP interface was designed to allow modules to send 
> correctly formatted
> CTCP requests to clients and receive replies to their requests without
> having to code it, not to trigger reply events for all modules within
> NeoStats. 
> 
> As such, I do not think this change appropriate. With this 
> change, a module
> is forced to check whether the CTCP reply was meant for it or 
> will have to
> duplicate the CTCP code to ensure it only gets responses it wants so
> basically makes the whole point of this API redundant. We 
> could have 10
> modules make a request and all 10 modules then have to 
> process 10 replies.
> If a reply is likely to be useful to multiple modules, then 
> it ought to be
> handled as the version request, i.e. the core makes one request for
> whichever modules wish to watch it. Version reply support for 
> all modules is
> handled by enabling version scan in the core and should not occur if a
> module makes the request. 
> 

Hi Mark,

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. 

Firstly, when I was debugging why SecureServ/StatServ were not receiving
CTCP version replies, I followed the codepath into services.c:

ModuleEvent neostats_events[] = {
         {EVENT_CTCPVERSIONRPL,     services_event_ctcpversion,
EVENT_FLAG_IGNORE_SYNCH},
         {EVENT_NULL,               NULL}
};

/** @brief services_event_ctcpversion
 *
 *  NeoStats CTCP VERSION reply event handler
 *
 * @return none   
 */
static int services_event_ctcpversion( CmdParams *cmdparams )
{
         strlcpy( cmdparams->source->version, cmdparams->param, MAXHOST );
         SendAllModuleEvent( EVENT_CTCPVERSIONRPL, cmdparams );
         return NS_SUCCESS;
}

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.

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. 

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.


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

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. 


> 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.)
Without a SendAll event, SecureServ and StatServ are broken. 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? 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. 

Thanks

Justin