View Issue Details

IDProjectCategoryView StatusLast Update
0002814FSSCPPilot datapublic2013-03-30 21:26
ReporterMjnMixael Assigned Toniffiwan  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
PlatformPCOSWindowsOS VersionWin7
Product Version3.6.19 
Target Version3.7.0 
Summary0002814: FSO does not delete .csg files when deleting pilots
DescriptionPretty straightforward, this one.

FSO is not deleting .csg files related to pilots when those pilots are deleted in the pilot select screen.
Steps To Reproduce1) Create a new pilot. Load a campaign or 2 or 3 to ensure multiple .csg files exists. May need to do this across a couple mods.

2) At the Pilot Select screen, delete that pilot.

3) Exit FSO and check your data/players folder. You'll see that pilot's .plr file is gone, but many PILOT.CAMPAIGN.csg files remain.
TagsNo tags attached.

Activities

niffiwan

2013-03-18 20:31

developer   ~0014782

I wonder if it deletes the currently active campaign .csg, or if it doesn't delete any. Regardless, should be a fairly simple fix.

niffiwan

2013-03-19 11:13

developer   ~0014784

I think that the problem is that the function that FSO uses to find all the .csg files ignores any files that are longer than 31 characters (i.e. >= MAX_FILENAME_LEN). i.e. using "tbd" as a pilot name resulted in all .csg files being removed. Using "quitealongname1234567890" as the pilot name resulted in none of the .csg files being deleted.

Not sure of the best way to fix this issue, as the function that finds the .csg files is "cf_get_file_list" and that's used for heaps of other stuff as well. Maybe give the flag an extra param to control its behaviour. Or write a new find-the-csg-files function. Or change the names assigned to .csg files :) I'll have to give it some more thought.

(note: looks like this problem predates the new pilot code)

FUBAR-BDHR

2013-03-20 08:12

developer   ~0014785

Last edited: 2013-03-20 08:24

I took a quick look and it seems that there are a few different file lengths used. One area uses _MAX_FNAME which is 256 characters, another uses MAX_PATH_LEN at 128. Then there is one that uses MAX_FILENAME_LEN + 2 for 34 characters (probably the one you were looking at).

Looks like there needs to be some uniformity to what the size can be.

Also just noticed there is a second cf_get_file_list function that appears to take a 128 length (brain is still not up to par). Could the call just be to the wrong function?

niffiwan

2013-03-20 09:20

developer   ~0014786

There are two cf_get_file_list functions, one puts the filenames into a SCP_vector, the other uses a C-style array. Unfortunately, both do the same ignore-more-than-31-char-files thing...

            if ( strlen(find.name) >= MAX_FILENAME_LEN )
                continue;

Re: the 128 length, were you referring to this?

char filespec[MAX_PATH_LEN];

FUBAR-BDHR

2013-03-20 17:01

developer   ~0014787

Highly possible. The thing I can't help wondering about this is how the game can read the list for use if it can't read it for deleting. Seems it should be able to delete in the same way it converts, creates, loads unless it's deleting the pilot file before it tries to delete the .csg files and its stored in there.

niffiwan

2013-03-20 20:45

developer   ~0014788

tl;dr I might have a patch ready tonight

Eh - those functions just make a choice to ignore those files. i.e. they both use OS calls to read the files from disk, those calls return all the files matching the specification (in this case, PILOT.*.csg), and then FSO tosses out the ones that are too long.

I suspect that the pilot conversion process will have the same issue because it uses the same function call (cf_get_file_list).

I think I'm going to fix this by removing the filename length check from the SCP_vector version of cf_get_file_list. This is only used in a few places, all to do with the new pilot code. Once I've removed the check, I'll change "mission_campaign_delete_all_savefiles" to use the SCP_vector version.

Regarding the removal of the filename length check, I can't see this being a problem. With an array you could overrun the array if you're not limiting the length of the filename, but not with a vector. And the array version of cf_get_file_list gets around this anyway by dynamically allocating the memory required to hold the string. So it'd perhaps come back to a memory use issue, and from what I can find the max filename length of NTFS/ext4/HPFS+ is 255 chars, so no big deal there.

niffiwan

2013-03-21 08:28

developer   ~0014792

Can you please try this patch? It should remove all .csg files (no matter how long) for a pilot.

MjnMixael

2013-03-22 19:23

manager   ~0014800

Tested and it didn't work. Still deleted the .plr, but not any .csg files.

niffiwan

2013-03-22 22:14

developer   ~0014801

Hmmmm. Did you delete the pilot from the initial selection screen, or from the barracks?

MjnMixael

2013-03-22 23:13

manager   ~0014802

Did both just to be sure.

niffiwan

2013-03-23 10:54

developer   ~0014814

Well, I'm confused. I double checked on my PC and the patch does remove all .csg files. I've double checked the code for all places where the string ".csg" exists and there's nowhere else that could delete the campaign files. So, I guess that the #if defined _WIN32 section in cf_get_file_list must behave differently to the Linux/OSX section and therefore I might need someone else to help resolve this.

FYI, I've been mostly testing with ST:R (since it has effectively 5 campaigns available)

One more request though, with a plain trunk build, can you check if the .csg files are or are not deleted when a pilot has a name only 1 character long?

MjnMixael

2013-03-23 14:15

manager   ~0014815

It didn't even remove 'd.str.csg'.

niffiwan

2013-03-24 01:47

developer   ~0014817

Last edited: 2013-03-24 03:20

I found a difference between the two versions of cf_get_file_list. The "temp" patch I attached *may* fix the issue since it makes the SCP_string version more like the other version, and I'm assuming that this isn't broken in the other version... (but I can't test/check it, so I might be completely wrong)

edit: nope - tested by chief1983 and that didn't work...

niffiwan

2013-03-30 09:21

developer  

mantis2814-mk2-svn.patch (2,390 bytes)   
Index: code/mission/missioncampaign.cpp
===================================================================
--- code/mission/missioncampaign.cpp	(revision 9612)
+++ code/mission/missioncampaign.cpp	(working copy)
@@ -814,9 +814,10 @@
 void mission_campaign_delete_all_savefiles( char *pilot_name )
 {
 	int dir_type, num_files, i;
-	char *names[MAX_CAMPAIGNS], file_spec[MAX_FILENAME_LEN + 2], *ext;
+	char file_spec[MAX_FILENAME_LEN + 2], *ext;
 	char filename[1024];
 	int (*filter_save)(char *filename);
+	SCP_vector<SCP_string> names;
 
 	ext = NOX(".csg");
 	dir_type = CF_TYPE_PLAYERS;
@@ -827,14 +828,13 @@
 	// be.  I have to save any file filters
 	filter_save = Get_file_list_filter;
 	Get_file_list_filter = NULL;
-	num_files = cf_get_file_list(MAX_CAMPAIGNS, names, dir_type, file_spec);
+	num_files = cf_get_file_list(names, dir_type, const_cast<char *>(file_spec));
 	Get_file_list_filter = filter_save;
 
 	for (i=0; i<num_files; i++) {
-		strcpy_s(filename, names[i]);
+		strcpy_s(filename, names[i].c_str());
 		strcat_s(filename, ext);
 		cf_delete(filename, dir_type);
-		vm_free(names[i]);
 	}
 }
 
Index: code/cfile/cfilesystem.cpp
===================================================================
--- code/cfile/cfilesystem.cpp	(revision 9612)
+++ code/cfile/cfilesystem.cpp	(working copy)
@@ -1447,12 +1447,9 @@
 	find_handle = _findfirst( filespec, &find );
 	if (find_handle != -1) {
 		do {
-            if (strcmp(strstr(filter, "."), strstr(find.name,".")) != 0)
+            if (strcmp(strrchr(filter, '.'), strrchr(find.name,'.')) != 0)
                 continue;
 
-			if ( strlen(find.name) >= MAX_FILENAME_LEN )
-				continue;
-
 			if (!(find.attrib & _A_SUBDIR)) {
 				if ( !Get_file_list_filter || (*Get_file_list_filter)(find.name) ) {
 					if ( check_duplicates && cf_file_already_in_list(list, find.name) ) {
@@ -1488,9 +1485,6 @@
 	dirp = opendir (filespec);
 	if ( dirp ) {
 		while ((dir = readdir (dirp)) != NULL) {
-			if ( strlen(dir->d_name) >= MAX_FILENAME_LEN ) {
-				continue;
-			}
 
 			if (fnmatch(filter, dir->d_name, 0) != 0)
 				continue;
@@ -1650,7 +1644,7 @@
 			if (num_files >= max)
 				break;
 
-            if (strcmp(strstr(filter, "."), strstr(find.name,".")) != 0)
+            if (strcmp(strrchr(filter, '.'), strrchr(find.name,'.')) != 0)
                 continue;
 
 			if ( strlen(find.name) >= MAX_FILENAME_LEN )
mantis2814-mk2-svn.patch (2,390 bytes)   

niffiwan

2013-03-30 09:22

developer   ~0014861

Last edited: 2013-03-30 09:28

I think I've found the issue in windows. It was assuming that there was only a single dot in all filenames. I've made some minor changes to check for the last dot (not the 1st) in the file checks. Can you please give the latest patch a try & let me know if it works?

MjnMixael

2013-03-30 15:01

manager   ~0014862

That patch totally fixes it. Nice work.

niffiwan

2013-03-30 21:26

developer   ~0014863

Fix committed to trunk@9613.

Related Changesets

fs2open: trunk r9613

2013-03-30 18:13

niffiwan


Ported: N/A

Details Diff
Fix for mantis 2814: don't check filename lengths when deleting .csg files
Also change win32 to check for last . in filenames, not the 1st
Affected Issues
0002814
mod - /trunk/fs2_open/code/cfile/cfilesystem.cpp Diff File
mod - /trunk/fs2_open/code/mission/missioncampaign.cpp Diff File

Issue History

Date Modified Username Field Change
2013-03-18 14:18 MjnMixael New Issue
2013-03-18 20:31 niffiwan Note Added: 0014782
2013-03-18 20:31 niffiwan Assigned To => niffiwan
2013-03-18 20:31 niffiwan Status new => assigned
2013-03-19 11:13 niffiwan Note Added: 0014784
2013-03-20 08:12 FUBAR-BDHR Note Added: 0014785
2013-03-20 08:24 FUBAR-BDHR Note Edited: 0014785
2013-03-20 09:20 niffiwan Note Added: 0014786
2013-03-20 17:01 FUBAR-BDHR Note Added: 0014787
2013-03-20 20:45 niffiwan Note Added: 0014788
2013-03-21 08:16 niffiwan File Added: mantis2814-svn.patch
2013-03-21 08:28 niffiwan Note Added: 0014792
2013-03-21 08:28 niffiwan Status assigned => feedback
2013-03-22 19:23 MjnMixael Note Added: 0014800
2013-03-22 19:23 MjnMixael Status feedback => assigned
2013-03-22 22:14 niffiwan Note Added: 0014801
2013-03-22 23:13 MjnMixael Note Added: 0014802
2013-03-23 10:54 niffiwan Note Added: 0014814
2013-03-23 14:15 MjnMixael Note Added: 0014815
2013-03-24 01:41 niffiwan File Added: mantis2814-temp-svn.patch
2013-03-24 01:47 niffiwan Note Added: 0014817
2013-03-24 01:49 niffiwan File Deleted: mantis2814-temp-svn.patch
2013-03-24 01:49 niffiwan File Added: mantis2814-temp-svn.patch
2013-03-24 03:20 niffiwan Note Edited: 0014817
2013-03-24 03:20 niffiwan File Deleted: mantis2814-temp-svn.patch
2013-03-30 09:21 niffiwan File Added: mantis2814-mk2-svn.patch
2013-03-30 09:22 niffiwan Note Added: 0014861
2013-03-30 09:22 niffiwan Status assigned => feedback
2013-03-30 09:22 niffiwan File Deleted: mantis2814-svn.patch
2013-03-30 09:28 niffiwan Note Edited: 0014861
2013-03-30 15:01 MjnMixael Note Added: 0014862
2013-03-30 15:01 MjnMixael Status feedback => assigned
2013-03-30 21:26 niffiwan Changeset attached => fs2open trunk r9613
2013-03-30 21:26 niffiwan Note Added: 0014863
2013-03-30 21:26 niffiwan Status assigned => resolved
2013-03-30 21:26 niffiwan Resolution open => fixed