I have just realized I overlooked something when converting the BAN_DATA members to std::string's. The new_ban() function (and what looks like all the new_* functions) allocate their memory using the alloc_perm routine, which allocates a raw block of memory. The BAN_DATA constructor is never called (and yes, the compiler gives it a default constructor), and the std::string members never have their constructors called consequently. So, the first time one tries to set a string, you have a happy fun crash on your hands.

This will come up again and again in the std::string conversion, so we should probably make the design decision now about how to fix this. Possible options:

1. Abandon alloc_perm in favor of the new operator when allocating memory for the struct objects. We could still maintain a list of structures to recycle (although I don't really see the value in this) if needed. However, instead of allocating raw blocks of space we would just keep a linked list of free structures. This probably also means creating explicit constructor functions for each structure as we go through the conversion, the better to initialize pointers to NULL or allocate objects as needed.

2. Rather than give each struct std::string members, each struct should have std::string pointers. new_* functions would be responsible for allocating and properly initializing any std::string pointers. The downside to this is that using the string pointers would be a bit more unwieldy, as the pointers would have to be dereferenced for most uses (such as assignment).

I would like to go ahead with course #1, but since this is a major departure, I'd like to put it out there for discussion first. Any comments?

Hmmmm…. so a struct gets a default constructor, even though it isn't a class? Dumb C++.

In this case, I'd probably choose option #3…. convert ban to be a proper class instead of a struct, and use one of the STL containers to maintain a set of the ban objects. Rewrite the query functions to access those, since nobody SHOULD be mucking about with them directly.

We have, save_bans(), load_bans(), check_ban() and ban_site() as our current API.

I would suggest the ban class support methods for save(), erase(), show(), and is_banned(). save() would actually write that ban data to the ban file. erase() would remove that ban data from the ban file. show() is a way to get a summary of what the ban does. is_banned() would be the interface for check_ban().

Then, the container class could feature save(), load(), show(), is_banned(), and ban(), and unban(). save and load do exactly that, save and load the ban set to disk. show lists the bans, so one can route that to the user, is_banned checks to see if a given site is banned just like check_ban does now. ban and unban add or remove bans like ban_site does now.

As for the container to use….. std::map comes to mind first, as most ban lookups will be by hostname/ip-address and the order of the bans is not important.

That's my 30 second blue-sky idea, at 3:33am. I may have a different opinion when I'm more awake. :)

Note that show() is just what comes to mind right now… if there's a more intuitive C++ name for something that presents the data in a human readable form (possibly summarized), do tell!

I've been dinking around with bans this evening and have come to the conclusion that if we are going to update structs/classes, we might as well do it all instead of piece-meal. Sounds scary but otherwise it's layer upon layer of shoehorning in/on top of/around messy code.

There are global functions out there for most of the ban functions. I am probably going to make the ban list a static member of the ban_data class, and will probably turn the global functions into either member functions or static member functions, depending on whether the function operates on one ban or on the entire list.

….and this was supposed to be the easy one :cry:

A struct and a class refer to the same type of internal object in C++. The difference is a struct by default is 'public'.

LOL, retrofitting is never easy. :(

However, I'd avoid looking at it as "what do we need to do to update this structure", and instead look at it as "what do we need to do to update this system".

Nobody cares HOW bans are implemented, they just want the feature to work, and for the UI to be similar to what's there now. At this moment, there are only three interfaces to the ban system.

check_ban( hostname, ban_type )
do_ban()
do_permban()

That's it. Both of the do_ functions call ban_site().
Literally…. do this: grep -i ban *.c | grep -v ban.c

So, instead of trying to retrofit crap, in THIS case I think it's simpler to just shred and rebuild.

Make a "bans" class that has an STL container of individual "ban" entries, along with load/save member functions, a check function that supports the current check_ban() API, and a ban member function to support the ban_site() API. That one will probably call private ban/unban functions to actually add/remove things from the STL container itself, but the public version can take a string argument for compatibility.

All the valid/invalid junk is for recycling, which is a dead horse that needs to go to the glue factory. If you pare down the structure to just what is there to support bans, it has three elements:

ban_flags (which is really a handful of toggles – make them booleans instead), level (the level of whomever put in the ban?), and name (the thing being banned).

Assuming my head clears a bit after I eat something (nasty weather front going through… it's going to hit ZERO degrees tonight), I'll try and make this a little more coherent. :)

I thoroughly hacked apart the ban code. I added a ban_manager class as a Singleton. Interface (commands) are now separated from the ban logic. There is a public api for checking, adding, and removing bans.

I have thoroughly tested it, and perhaps most importantly I have thoroughly documents all the functions (using doxygen style). That was pretty brutal.

It's on the dev branch as revision 81.

This is how it necessarily must be… Even disregarding what Runter said (which is very true, but I think doesn't matter for the conceptual problem here): What would you expect to happen when you allocate a structure with member variables that have their own constructors that need to be called? The default constructor is as dead-simple as it needs to be.

FWIW, I would advocate dumping the recycling business. There's not much point in having it, really.