use MD5 for save filename

[Rosalie241]

This makes mupen64plus-core use the ROM's MD5 for the save filename, this makes more sense because let's assume you have a patched ROM, the GoodName will be Game (Unknown ROM), now let's assume you have another patched ROM that uses the same base game, then the saves of patched ROM 1 will show up in patched ROM 2, which is undesired and will most likely break stuff.

[richard42]

This is good; I just have one request. Please change the filename generation string formatting parameters from their current template of "%s%s" to "%s-%.6s" to make it a bit more human readable by separating the hash with a dash and only using the first 6 characters of the MD5.

[Rosalie241]

to make it a bit more human readable by separating the hash with a dash and only using the first 6 characters of the MD5.

wouldn't that technically be risky and be able to cause issues? (i.e that 2 roms will have the same first 6 characters of a md5 hash)

[richard42]

It is possible but extremely unlikely. The chance that 2 ROMs are different but have MD5 hash values with the first 3 bytes being the same are about 1 in 16 million. I think it's worth the very very small risk of not solving the problem that you're trying to solve in some cases, in order to avoid subjecting all users to the horror of appending an extra 32 characters of crap on all of their savegame filenames :) You could maybe bump it up to 8 hex digits and chances go down to 1 in 4 billion, but I suspect that even with 6 digits no user would ever experience a collision. I doubt that there are 16 million different patched ROM images out there.

[Rosalie241]

the added - to the front of the filename looks a bit ugly in my opinion, the filename will be (for example): -2048A640.eep, would removing the - be okay aswell?

[richard42]

Sorry I didn't quite look closely enough at your changes; I thought you were appending the MD5 to the goodname, while I see now that you were replacing the name with the MD5. I think we should keep the rom name in the filenames, again for the sake of readability. The dash was intended to be a separator between the name and the hash; you could also use parentheses or brackets, like this: "%s(%s)", ROM_SETTINGS.goodname, ROM_SETTINGS.MD5. We could also switch to using the rom header name if you think the goodname is too long, though the rom header name is often truncated IIRC and the goodname might be better.

[Rosalie241]

Sorry I didn't quite look closely enough at your changes; I thought you were appending the MD5 to the goodname, while I see now that you were replacing the name with the MD5. I think we should keep the rom name in the filenames, again for the sake of readability. The dash was intended to be a separator between the name and the hash; you could also use parentheses or brackets, like this: "%s(%s)",

That makes a lot more sense, thank you!

ROM_SETTINGS.goodname, ROM_SETTINGS.MD5. We could also switch to using the rom header name if you think the goodname is too long, though the rom header name is often truncated IIRC and the goodname might be better.

Yeah because the maximum path on windows is quite short (260 chars iirc), it might be better to use the internal name, a dash & the first 6 (or 8) chars of the md5 hash.

[Rosalie241]

Hmm, actually, what if the internal name is empty? i.e for iQue games

[richard42]

When things start to get complicated it's good to think about adding a new function. You could implement a function which returns a string with the calculated state filename like this: use the internal ROM header name if present, otherwise use the first 32 characters of the goodname if present, otherwise something like "unknown", and then append a dash and the first 6 or 8 characters of the md5 hash.

[Rosalie241]

When things start to get complicated it's good to think about adding a new function. You could implement a function which returns a string with the calculated state filename like this: use the internal ROM header name if present, otherwise use the first 32 characters of the goodname if present, otherwise something like "unknown", and then append a dash and the first 6 or 8 characters of the md5 hash.

good point, thank you for your help and I hope I did it right :two_hearts:

[richard42]

Looks good to me!

[loganmc10]

@Rosalie241 this change breaks the netplay code.

Netplay syncs saves based on the filename. Because of this change, it's impossible to reliably determine what the filename is.

Basically, if the save exists, Player 1 will tell the netplay server "here is 'Super Mario.mpk'". Player 2 will assume that the file should be called 'Super Mario-MD5.mpk" and ask for that from the netplay server, which the netplay server won't be able to serve.

[Rosalie241]

@Rosalie241 this change breaks the netplay code.

Netplay syncs saves based on the filename. Because of this change, it's impossible to reliably determine what the filename is.

Basically, if the save exists, Player 1 will tell the netplay server "here is 'Super Mario.mpk'". Player 2 will assume that the file should be called 'Super Mario-MD5.mpk" and ask for that from the netplay server, which the netplay server won't be able to serve.

Can't you fix that client-side? i.e use the same function to determine what save filename to use and then save it there.

[loganmc10]

Can't you fix that client-side? i.e use the same function to determine what save filename to use and then save it there.

mupen64plus-core is the client (https://github.com/mupen64plus/mupen64plus-core/blob/673aa48d1ddf843e4889274d1e666f0dca1ab468/src/backends/file_storage.c#L33)

When player 1 reads a file, let's say "Super Mario.mpk", player 1 sends the file contents to the netplay server, along with the filename (Super Mario.mpk). When players 2-4 read the file, rather than read it from the disk, they ask the server for the file. However, they are now asking for "Super Mario-MD5.mpk" (unless "Super Mario.mpk" exists on their local disk, which shouldn't matter during netplay), so the netplay server doesn't have that file.

[Rosalie241]

When player 1 reads a file, let's say "Super Mario.mpk", player 1 sends the file contents to the netplay server, along with the filename (Super Mario.mpk). When players 2-4 read the file, rather than read it from the disk, they ask the server for the file. However, they are now asking for "Super Mario-MD5.mpk" (unless "Super Mario.mpk" exists on their local disk, which shouldn't matter during netplay), so the netplay server doesn't have that file.

then can't you fix it server-side? I assume you have the ROM's md5 (or you could maybe send it from the client to the server) to generate a list of 'supported' filenames for the save file.

[loganmc10]

I just reverted this commit in simple64, I'm just letting you know that it breaks the netplay code included with mupen64plus-core, it's not something that should be handled server-side

[richard42]

So if I understand correctly, the problem is not due to the fact that the filename has changed, but that the filename is adaptive depending upon whether or not a particular file exists on the disk, correct? If there was no fallback to the old filename, and it always used the new name with the MD5, it would not break the netplay?

[loganmc10]

Yes correct,

The flow is like this for the host/P1:

• P1 reads a save file (mpk, fla, etc)
• P1 sends this file to the netplay server, along with the filename. This filename may be "game_name.mpk" or "game_name-md5.mpk" depending on the new logic

The flow is like this for P2-4:

• P2-4 wants to read a save file
• It says to the server "I need game_name.mpk" or maybe "I need game_name-md5.mpk"
• The server returns the bytes for that file.

If there is a mismatch between what P1 told the server the filename is, and what P2-4 ask for, it doesn't work.

The code for this is here: https://github.com/mupen64plus/mupen64plus-core/blob/673aa48d1ddf843e4889274d1e666f0dca1ab468/src/main/netplay.c#L437-L499

At the time it was fine, since the filenames were consistent/predictable. It would probably work to just modify netplay_read_storage() and have it just use the extension as the "short_filename", since each game should only have 1 save type per extension. This way, P1 would just say the file name is "mpk", and the other player would just request "mpk"

[m4xw]

+1 for short names (i'd personally prefer a enum even)