Daimonin MMORPG Logo
Daimonin voting system

Recent Posts

[Community chat] internet messed up by VMP-KNIGHT
Today at 10:50:19 am
[Graphics] Artemis Artwork by True_Artemis
Today at 09:06:10 am
[Tech support] cant access main server by VMP-KNIGHT
October 23, 2014, 01:06:40 am
[Community chat] moving the servers by michtoen
October 23, 2014, 12:56:30 am
[Spoilers] GT keys by VMP-KNIGHT
October 21, 2014, 02:27:11 am
[Community chat] Infravision by Shroud
October 20, 2014, 11:23:47 pm
[Community chat] +4 etc on items by VMP-KNIGHT
October 19, 2014, 09:46:16 pm
[Events] hallo maps by Shroud
October 19, 2014, 01:27:20 am

Daimonin Forums

 
Pages: 1 [2] 3   Go Down
  Print  
Author Topic: Overhaul of /ban code  (Read 10115 times)
Torchwood
Warlord of Moroch Army
*
*
*
*
*
*
*



Posts: 1494
Karma: +44/-7


View Profile
« Reply #15 on: March 02, 2012, 09:42:18 pm »

Sorry, also ...

I've updated bug-tracker #26, which requests account banning.  I put a 'close request' on this one, but I don't know how that works?  Who closes?

Not sure about #25; the only thing I can spot there still open is the ability to set the ban time as "15d" or "10h".  At the moment, you can only specify a number, which is translated as a number of seconds.  Do we want the ability to type "15d" to ban for 15 days?
clobber
Map Wizard (ClObliterator)
Raas Army General
*
*
*
*
*
*
*



Posts: 3108
Karma: +56/-7


View Profile
« Reply #16 on: March 02, 2012, 10:50:35 pm »

Sounds nice and useful but it isn't vital.
Torchwood
Warlord of Moroch Army
*
*
*
*
*
*
*



Posts: 1494
Karma: +44/-7


View Profile
« Reply #17 on: March 03, 2012, 08:04:47 am »

:)

Agreed - not vital.  But, remember, my server coding skills are still pretty minimal.  I really don't understand many, many areas of the code yet, and my C skills are fairly basic.  So stuff like this forms a relatively safe area for me to work in.  I'm unlikely to mess up any player files or to break anything, and I'm slowly getting to grips with more aspects of the code.

Of course the downside is I'm not likely to be able to address any of, say Immage's, feedback at the moment.  But I hope that will come in time.  One day I want to be able to add something major to the code ...

Anyway, r6844 adds the ability to ban for 1d, 1h, 1m or 1s.  It is actually a pretty simple piece of code (but useful - I learnt something new).

:)
clobber
Map Wizard (ClObliterator)
Raas Army General
*
*
*
*
*
*
*



Posts: 3108
Karma: +56/-7


View Profile
« Reply #18 on: March 04, 2012, 01:45:25 am »

*cough* clans *cough*
Torchwood
Warlord of Moroch Army
*
*
*
*
*
*
*



Posts: 1494
Karma: +44/-7


View Profile
« Reply #19 on: March 04, 2012, 07:32:27 am »


Hmmm ... maybe?

Make that coughing noise again in about a month to remind me.
clobber
Map Wizard (ClObliterator)
Raas Army General
*
*
*
*
*
*
*



Posts: 3108
Karma: +56/-7


View Profile
« Reply #20 on: March 04, 2012, 02:43:46 pm »

Great :D
Torchwood
Warlord of Moroch Army
*
*
*
*
*
*
*



Posts: 1494
Karma: +44/-7


View Profile
« Reply #21 on: March 04, 2012, 08:26:26 pm »

Wow - that was hard work!!!

I decided to try and write find_players_on_ip(str), for use within /ban.  After several hours scratching my head (and a few 'reverts' - 'lets start again' moments), I've figured out how to create a temporary linked list.

r6847 adds the new code; in the end it was relatively simple.  However, I would really appreciated it if someone more experienced than me could cast their eyes over the code and check I haven't created any memory leaks anywhere?

However, in testing this code I found a bug.  It took me several hours to realise the bug is in the existing doeric_server() code, and actually nothing to do with my new code.

This function contains this code:
Code: [Select]
/* Go through the players.  Let the loop set the next pl value,
 * since we may remove some
 */
for (pl = first_player; pl != NULL;)
{
    if (pl->socket.status == Ns_Dead)
    {
        player *npl = pl->next;

        remove_ns_dead_player(pl);
        pl = npl;
    }

Now, when we /ban an IP address, my code searches for all characters on that IP address and /kicks them (similarly when banning an account).

The problem comes in that if a person has logged into the server twice using 2 different characters, it is possible these 2 characters are next to each other in the server's list of players.  In this case, when both characters are kicked, the reference to pl->next won't be valid in the code above!  This causes a sigfault in the server.

In the past, you would /kick one player, and then /kick the second player in a separate command, and doeric_server has a chance to tidy stuff up in between the 2 command calls.  Now we have both /kicks inside the same command call, and doeric_server then gets it wrong!

For now, the /kick bit of my new code for IPs and Accounts is commented out.

Any ideas how we fix this?
smacky
The Singular Soul of Lom Lobon Tribe
*
*
*
*
*
*
*



Posts: 8648
Karma: +275/-134


View Profile
« Reply #22 on: March 04, 2012, 11:18:56 pm »

Add Ns_Kicked then instead of actually kicking the player/ip just change the socket.status to Ns_Kicked. Then the above if block tests for Ns_Dead || Ns_Kicked, IOW kick/ban/whatever just marks the perp, do_eric_server() does the actual kicking. (IDK if you even need to add Ns_Kicked, perhaps just setting NsDead works, not sure which is best)
_people_
Warlord of Moroch Army
*
*
*
*
*
*
*



Posts: 1571
Karma: +53/-4


View Profile
« Reply #23 on: March 05, 2012, 03:03:15 am »

Another issue I've just found is that in many recent revisions, I cannot create a new character. I get a message saying something about error code 2, call a GM.
smacky
The Singular Soul of Lom Lobon Tribe
*
*
*
*
*
*
*



Posts: 8648
Karma: +275/-134


View Profile
« Reply #24 on: March 05, 2012, 04:10:52 am »

Is this on a public server (which one?) or local?

Error 2 is ADDME_MSG_CORRUPT which is an unhelpfully generic error but seems most often to indicate the file could not be saved for some reason. Lack of disk space?

EDIT: Yes, I get this on dev 100% of the time. Main is fine though.
_people_
Warlord of Moroch Army
*
*
*
*
*
*
*



Posts: 1571
Karma: +53/-4


View Profile
« Reply #25 on: March 05, 2012, 04:15:39 am »

I have plenty of disk space. I grabbed another revision a ways back and it worked fine. I checked the account file and it contained nothing but my password.

It was on local only, but it might be on Dev too if it was rebooted since whenever it broke. EDIT: Yep, broken on Dev too.
smacky
The Singular Soul of Lom Lobon Tribe
*
*
*
*
*
*
*



Posts: 8648
Karma: +275/-134


View Profile
« Reply #26 on: March 05, 2012, 04:50:08 am »

Here's my guess: http://daimonin.svn.sourceforge.net/viewvc/daimonin?view=revision&sortdir=down&revision=6837

These new functions in account.c are called from login.c:player_save(), causing iit to fail iif the player is not in an account. Which for just created chars is the case. So 100% failure. In commands.c:cs_cmd_newchar() is:

Code: [Select]
            if(!player_save(pl->ob)) /* if we can't save we don't add this char to the account */
                ret = ADDME_MSG_CORRUPT;
Torchwood
Warlord of Moroch Army
*
*
*
*
*
*
*



Posts: 1494
Karma: +44/-7


View Profile
« Reply #27 on: March 05, 2012, 07:59:03 am »

Ooops - sorry guys.

Yes - you are right smacky.  I replicated the fault on my local server.  I temporarily disabled the account_save inside player_save, and problem goes away.  I've just committed this and rebooted DEV, so when that comes back up we should be fine.

I'll investigate the proper fix this evening; I guess should be simple enough.
Torchwood
Warlord of Moroch Army
*
*
*
*
*
*
*



Posts: 1494
Karma: +44/-7


View Profile
« Reply #28 on: March 05, 2012, 05:35:39 pm »


Properly fixed now with r6856, although I haven't rebooted DEV yet.
Torchwood
Warlord of Moroch Army
*
*
*
*
*
*
*



Posts: 1494
Karma: +44/-7


View Profile
« Reply #29 on: March 05, 2012, 07:56:54 pm »

Add Ns_Kicked then instead of actually kicking the player/ip just change the socket.status to Ns_Kicked. Then the above if block tests for Ns_Dead || Ns_Kicked, IOW kick/ban/whatever just marks the perp, do_eric_server() does the actual kicking. (IDK if you even need to add Ns_Kicked, perhaps just setting NsDead works, not sure which is best)

Just been looking at this, and I know why we get the crash.  In pseudocode, we have this:

Code: [Select]
command_ban()
{
kick_player(pl1)
{
remove_ob(pl1)  // marks pl1 as eligible for removal
}
kick_player(pl2)
{
remove_ob(pl2)  // marks pl2 as eligible for removal
}
}

doeric_server()
{
remove_ns_dead_player(pl1)
remove_ns_dead_player(pl2)
}


remove_ns_dead_player(pl)
{
...
leave_map(pl)
{
remove_ob(pl)
}
...
free_player(pl)
{
object_gc()  // removes ALL objects marked as eligible for removal
}
}

So, the ban command ends up calling remove_ob() on each player, which means the call to object_gc() in the first call to remove_ns_dead_player() ends up wiping out both players at the same time.  The second call to remove_ns_dead_player() then has an invalid pointer, and we crash.

Solution is simple - get rid of the call to remove_ob() inside kick_player().  We can safely do this because remove_ns_dead_player() calls remove_ob() anyway via leave_map() prior to free_player().

Hope that all makes sense?  I've tested it and all seems to work, but I'm not going to commit just yet ... would like some opinions first?
Pages: 1 [2] 3   Go Up
 
 

Powered by SMF 2.0 RC1-1 | SMF © 2006–2008, Simple Machines LLC
Page created in 0.312 seconds with 24 queries.
Checkout