09 Dec, 2008, Banner wrote in the 1st comment:
Votes: 0
Har har. I'm using my own version of account code and it saves/loads from memory. I'm trying to write a function that will remove accounts with no players attached to them, but for some reason, when I add the line that removes the account, it'll only remove the first account then stop. If I comment it out, it'll report it all correctly.

Cleanup Function:
void cleanup_accounts( void )
{
ACCOUNT_DATA *account;
char buf[MSL];
int count = 0, accnum = 0;

sprintf( buf, "%s initiated", __FUNCTION__ );
log_string(buf);
for( account = first_account, accnum = 0; account; account = account->next, accnum++ )
if( !account->first_acc_char )
{
sprintf( buf, "%s", account->name );
log_string(buf);
sprintf( buf, "Removing Account %s", account->name );
log_string(buf);
count++;
dispose_account(account); <———— PROBLEM LINE
}
sprintf( buf, "%s: %d of %d accounts removed. %d remaining.", __FUNCTION__, count, accnum, accnum-count );
log_string(buf);
return;
}


dispose_account and dispose_acc_char:
void dispose_acc_char( ACCOUNT_DATA * acc, ACC_CHAR_DATA * ch )
{
if( !ch )
return;

if( ch->name )
STRFREE( ch->name );
if( ch->clan )
STRFREE( ch->clan );
if( ch->password )
STRFREE( ch->password );
if( ch->quit_loc )
STRFREE( ch->quit_loc );

UNLINK( ch, acc->first_acc_char, acc->last_acc_char, next, prev );
DISPOSE( ch );
}

void dispose_account( ACCOUNT_DATA * acc )
{
ACC_CHAR_DATA *ch, *next_ch;

if( !acc )
return;

if( acc->name )
STRFREE( acc->name );

for( ch = acc->first_acc_char; ch; ch = next_ch )
{
next_ch = ch->next;

dispose_acc_char( acc, ch );
}

UNLINK( acc, first_account, last_account, next, prev );
DISPOSE( acc );
write_accounts();
}


The command appears to work, but when the line I've marked above is commented, this is the output. It doesn't actually delete the accounts, but reports it correctly:

Commented Output:

[Health]: 100/100 [Move]: 30000/30000 [$$]:[82910372]|>Log: cleanup_accounts initiated
Log: Bob
Log: Removing Account Bob
Log: Bob
Log: Removing Account Bob
Log: Bob
Log: Removing Account Bob
Log: Bob
Log: Removing Account Bob
Log: Bob
Log: Removing Account Bob
Log: Bob
Log: Removing Account Bob
Log: Bob
Log: Removing Account Bob
Log: Bob
Log: Removing Account Bob
Log: Bob
Log: Removing Account Bob
Log: Bob
Log: Removing Account Bob
Log: Bob
Log: Removing Account Bob
Log: Bob
Log: Removing Account Bob
Log: Bob
Log: Removing Account Bob
Log: Bob
Log: Removing Account Bob
Log: Bob
Log: Removing Account Bob
Log: Bob
Log: Removing Account Bob
Log: Bob
Log: Removing Account Bob
Log: Bob
Log: Removing Account Bob
Log: Bob
Log: Removing Account Bob
Log: Bob
Log: Removing Account Bob
Log: Bob
Log: Removing Account Bob
Log: Bob
Log: Removing Account Bob
Log: Bob
Log: Removing Account Bob
Log: cleanup_accounts: 23 of 40 accounts removed. 17 remaining.


Uncommented output:

[Health]: 100/100 [Move]: 30000/30000 [$$]:[82910372]|>Log: cleanup_accounts initiated
Log: Bob
Log: Removing Account Bob
Log: cleanup_accounts: 1 of 6 accounts removed. 5 remaining.


Basically, when I uncomment the line that actually deletes the unused account, it'll delete the first bad account and end. If I comment the line, it'll report all problem accounts but it won't actually delete them (obviously, since I uncommented it). I suspect the problem is obviously in dispose_account somewhere, but I'm unsure as to what it is. Can anyone point me in the right direction? Thanks in advance..


Edit: To remove break from code.. wasn't supposed to be there.
09 Dec, 2008, Sharmair wrote in the 2nd comment:
Votes: 0
I can see a number of things wrong with this code. But the two things most likely causing
your problem would be the break kicking you out of the loop when the if was true, and
the use of account in the loop to get the next account after it is DISPOSEd.
09 Dec, 2008, Banner wrote in the 3rd comment:
Votes: 0
Sharmair said:
I can see a number of things wrong with this code.

More specifically?

Sharmair said:
the use of account in the loop to get the next account after it is DISPOSEd.

Which part? Can you quote it?


But yeah, that break isn't supposed to be there. I added that there to test something. The above still hapens without the break in the code.
09 Dec, 2008, David Haley wrote in the 4th comment:
Votes: 0
Sharmair's talking about this…
for( account = first_account, accnum = 0; account; account = account->next, accnum++ )
…dispose_account(account);

Bad times to dereference a pointer that you freed. If you ran this through valgrind, it would complain bitterly about this.
09 Dec, 2008, Sharmair wrote in the 5th comment:
Votes: 0
When you call dispose_account(account); (line 19) you UNLINK account from the list and DISPOSE
(free) it. Then when you get to the end of the for loop, you do account = account->next using
the now freed and unlinked account pointer. The normal fix for this is to add a next holder like
your dispose_account function uses for the characters (next_ch). You should not try to use any data
from a freed chunk of memory (like trying to get the next pointer).
09 Dec, 2008, Banner wrote in the 6th comment:
Votes: 0
To reiterate, this is now the correct way to do it? I tested it and it worked, but I never knew what you just told me (and it actually makes a lot of sense since I now know, thanks for that).

void cleanup_accounts( void )
{
ACCOUNT_DATA *account;
ACCOUNT_DATA *next_acc;
char buf[MSL];
int count = 0, accnum = 0;

sprintf( buf, "%s initiated", __FUNCTION__ );
log_string(buf);
for( account = first_account, accnum = 0; account; account = next_acc, accnum++ )
{
next_acc = account->next;
if( !account->first_acc_char )
{
sprintf( buf, "Removing Account %s", account->name );
log_string(buf);
count++;
dispose_account(account);
}
}
sprintf( buf, "%s: %d of %d accounts removed. %d remaining.", __FUNCTION__, count, accnum, accnum-count );
log_string(buf);
return;
}
09 Dec, 2008, David Haley wrote in the 7th comment:
Votes: 0
Yes, that's what you have to do when iterating over linked lists structured this way if you might delete some of the elements.
0.0/7