VFS support

[claudiuslollarius]

Work in progress, adding VFS support to RetroArch

[ghost]

Hi @claudiuslollarius, getting some linker errors here:

http://p.0bl.net/88029

Can you look into that?

[ghost]

Some initial thoughts on the PR itself:

• C89 issues (variables declared in the middle of a block)

• RFILE_MODE_READ_TEXT was removed (video_shader_parse.c, config_file.c, playlist.c, file_stream_transforms.c)

• config_file.c WIIU/setvbuf calls were removed

• config_file.c why is dump_vfs() exactly the same as dump()?

• several functions in file_stream.c use int or long that deal with file size/position... shouldn't we be explicitly using 64-bit everywhere? (e.g. for files >4GB)

• verbosity.c why was logging to stderr removed?

• filestream_get_ext() already exists in the form of path_get_extension(), why duplicate it?

• vfs_implementation.c:209 stream->mapsize is set to the error-code return value of filestream_seek (it's not ftell!)

• libretrodb.c:267,276 (same seek issue as above)

• retro_vfs_file_size_impl seeks to the end of the file and leaves it there, it should return to the original position after getting the file size

• this undoes all my Unicode changes for Windows and other bugfixes since then, perhaps you need to rebase this from the latest master commit?

[claudiuslollarius]

Just for reference, I am @albertofustinoni

I have worked on the PR more an now

• warnings are fixed
• the env callback handles telling cores the VFS is supported.

To answer your points:

• During the discussion obout the VFS API everyone, including @twinaphex, agreed that the filestream interface should be simplified, and removing RFILE_MODE_READ_TEXT is part of that.
• config_file stuff: if the idea is to build that on top of filestream as was the case until now, everyhting that tries to touch physical file handles needs to go.
• Functions in filestream that include int do so to report status. Everything that deals with file sizes or positions is uint64_t
• Verbosity can still dump to stderr
• Hopefully vfs_implementation.c is OK now
• C89 issues: Could you list what you are referring to? I am not an expert on that.

[inactive123]

Lots of cores depend on filestream, if you start changing function prototypes and the like then you'd need to update a lot of cores, especially for statically linked platforms.

And why are changes to the filestream interface necessary for getting this VFS to work? I think it should be perfectly possible to just have a VFS implementation for RetroArch that shouldn't require any drastic ABI-breaking changes to the file_stream interfaces.

[albertofustinoni]

Believe me, I have no particular desire to change more things than necessary.

However, the changes to filestream are needed to make the libretro.h API as stable as possible going forward: first of all turning parameters like file positions and sizes to 64 bit integers, to support files greater than 4GB.

Impact on cores should actually be fairly limited: as part of the PR I had to update the rest of libretro_common to use the modified filestream and non libretro code in cores goes through the file_stream_transforms stdio shims anyways.

[inactive123]

What about C89 though? It doesn't support 64bit types, and we do want to remain compatible with C89. So for this you will have to provide fallback paths for C89/32bit-only builds.

You can use a conditional like this -

https://github.com/libretro/RetroArch/blob/master/libretro-common/file/config_file.c#L648

[inactive123]

I still really doubt and question why we have to do things in this convoluted way and why we can't just have the filestream code in the cores that have an explicit need for it, like we do right now. That still seems to be the best solution to me considering how this implementation ended up. It ended up as a thin layer around file_stream which I already assumed before an RA implementation was written. I appreciate the effort that went into this, but if we have to update AND the cores and make sure the filestream code is always identical to the latest code, AND we have to then provide an implementation in the frontend, what exactly are we doing here, and what is the point? All we did then is add extra maintenance cost for ourselves and layers of indirection without being able to remove file_stream from the cores itself, since we don't want to break the API's ABI or require cores to suddenly move to some hypothetical libretro version 2. Again, if something is not absolutely necessary which should entail a bump, I want to stretch out libretro version 1 as long as humanly possible. Backwards compatibility is a virtue here, and not something that we should be jumping at the hip to sacrifice as soon as possible.

I simply don't see the added value given these preconditions. It might work if one was prepared to break ABI and bump libretro up to version 2, but I am simply not willing to do that. Not when I don't see obvious, measurable advantages that would justify the huge setback that entails an API bump.

More and more I am coming back to - if it ain't broke, why fix it like this? What is it inside the implementation of file_stream as it is right now that absolutely wouldn't work for UWP or say Kodi? Why not simply implement codepaths for UWP then in the libretro-common code and just cover all bases? It all seems like a massive overcomplication and more and more I wonder if this is what libretro should be about. Why not simply add UWP codepaths in file_stream and then do it like that?

So can somebody explain to me again why this is actually even needed at all and what benefit or advantage there is to simply rolling filestream functionality inside the cores that actually need it on a per-core basis? Because i don't see us gaining anything at all from it if the frontend implementation in RA is basically identical to file_stream code as-is. And I don't buy the argument that a different libretro frontend needs a totally unique way to handle files or streams from say RA. If the frontend is written in C/C++, a one-size-fits-all solution in the form of the file stream code in libretro-common should just suffice. I can maybe see it if somebody wants to implement the frontend entirely as C# or some other language, but even still, this will become messy for our libretro ecosystem then. We then need to support AND a codepath that assumes VFS support in a libretro front, AND a codepath that falls back to file_stream in the case a frontend does not support it. Then you need to compile in the file_stream code still into the core as-is, which brings me back to the point - what's the point? It's self defeating then, you increase the amount of manual labor a frontend implementor needs to do in order to get a working frontend, when honestly hooking up a VFS might be beyond his skillset and it might just be handled better by us in the form of file_stream and friends in the first place.

I am not wishing to backpedal here, I just want to be clear here on why this is absolutely necessary if all we are doing is reimplementing file stream behind another layer anyway.

[albertofustinoni]

I don't get the point about C89 and 64 bit integers: libretro.h already had int64_t variables defined.

Right now the code is convoluted to make it so that the VFS implementation for Retroarch is the exact same code as the fallback in libretro_common's filestream for when a core is used with a non VFS aware front end. Doing so will allow us to find and fix bugs as quickly as possible, since the code will inevitably be used.

Once you are happy with the quality of the code AND you have use cases for the VFS in Retroarch, you will change the function pointers passed in the env callback in dynamic.h to something Retroarch specific which will not go into common.

The other reason the code is structured this way is to avoid having to update filestream for every new platform: front ends will be able to take care of that on their own since VFS files are just opaque handles to the cores.

[inactive123]

The other reason the code is structured this way is to avoid having to update filestream for every new platform: front ends will be able to take care of that on their own since VFS files are just opaque handles to the cores.

The thing is though - if you read my previous post, you will see how this is actually false. I will not do that. I want to support frontends that DO NOT support this VFS extension. I will never make VFS a forced requirement for a libretro frontend. So if we are to have a VFS interface, it is to be optional, just like the perf interface is optional, just like the logging interface is optional, etc. Breaking backwards compatibility with existing fronts is something I will not do unless we stumble upon something mission critical that absolutely, positively enforces us to bump API.

So, what that boils down to is for this VFS extension, not only do I need to keep updating the file_stream code on an individual basis per core, but I also then emburden each frontend implementor with the burden of having to hook up his own VFS routines. So what is the net gain in this? None as far as I see. You don't gain anything, it will just be more work for a core writer to start hooking up every core with VFS routines, and ALSO have to provide fallback routines for file_stream code. So instead of one codepath, I now have to support two. And then some adventurous guy starts entertaining the idea of making a core that requires the VFS extension. Exactly the thing we don't want to enable, encourage or enforce. You create a break in the libretro ecosystem this way; before and after this VFS extension was introduced. I don't like the idea to begin with. I think the road to Hell is paved with good intentions, and this might just be one of them. I don't like some kind of cutoff point like SDL 1.2 to SDL 2 where everybody had to stop whatever they were doing and then have to support some entirely new API. I will do everything within my power to prevent such a hard break unless somebody can provide me with 100% solid evidence and proof that this is absolutely needed in order to move forward. So far, I have not seen that.

Again, I don't want to backpedal here, I gave the green light here, but the more we talk about this, and the more we labor over these implementations, I am beginning to have severe second guesses on whether we should pursue this at all and whether or not we should not simply keep extending the file_streams idea instead. It just sounds like a bad idea all around from a backwards compatibility perspective, especially if there are no measurable gains that couldn't be had simply with the file_stream code.

[albertofustinoni]

You will support front ends that do not want to deal with the VFS: that is the whole reason for the fallback logic in filestream.

You do not need to keep updating filestream fallback code: once it's up to par with current functionality you leave it as is - cores paired to front ends that do not support the VFS run on the same platforms as currently.

New front ends do not necessarily need to implement the VFS: if the platform they run on has file access done via stdio they work as they would currently. If on the other hand the platform uses something different, like UWP, they can implement the VFS once, in their codebase, without having to touch every single core.

[inactive123]

You will support front ends that do not want o deal with the VFS: that is the whole reason for the fallback logic in filestream.

You do need to keep updating filestream fallback code: once it's up to par with current functionality you leave it as is - cores paired to front ends that and the VFS run on the same platforms as currently.

Explain to me how this is the case. Libretro-common always updates from time to time. I don't buy the argument that at some point, file_stream is frozen in time and no more updates will be pushed to it.

So how exactly would this work? How exactly would file_stream code inside the core no longer need to be updated? I still don't see how that could be the case when RetroArch for instance has several statically linked platforms that requires you to link the core against the frontend, and we also have dynamic cores where the source files for filestream themselves need to be baked into the core itself, and in RetroArch they'd be baked into the frontend binary as well.

I simply see duplication of effort here. Try to clarify this for me.

[albertofustinoni]

BTW this same mechanism allows you to add support for stuff like CHD on the front end instead of updating common and having to update every single core afterwards.

[inactive123]

Can you make a fork of a libretro core repo and then demonstrate to me these points? In particular, I'd like to see how using this VFS extension, I would no longer have to individually update file_stream code. You make it sound as if the file_stream code would no longer have to be inside the core. I'd like to see how that would work exactly and how that would still work on frontends that did not implement any VFS code at all. Sounds like an impossibility to me, unless I am missing something here. Did you test this yourself on older fronts? For instance, RetroArch 1.0.0.2 or some other ancient version?

If your example proves that, I am willing to set aside all fears and doubts on my end.

[inactive123]

Anyway, let's focus first on the example core that I asked for in the post above. This thing that I am pointing out can wait until later, but I still want to point it out anyway while it is still fresh on my mind -

As for C89, it doesn't support 64bit values period. I think I might have inserted a typedef for int64_t for C89 which basically degrades into a 32bit type, just so that I don't have to put an ifdef conditional around every trivial int64_t/uint64_t variable i use in RA. In most cases, 64-bit precision is not necessary for a lot of these variables. We could use the bits_128 struct but that would be a bit messy code and would require we use macros in order to manipulate it. Kinda messy and not really ideal.

[albertofustinoni]

I'd ask you to look at file_stream.c:

On top it has function pointers for all the VFS callbacks, which are null by default.

If the front end supports the VFS, filestream_vfs_init will be called and those function pointers will be assigned to functions defined in the front end.

At that point, every single call to filestream methods will be serviced by the front end. And since RFILE is not defined anywhere in the core code (unless VFS_FRONTEND is defined, which should not be the case in cores), the front end defines RFILE in whatever way it needs to (in UWP's case you could stick a StorageFile^ in a structure).

How would you want me to demonstrate this to you? I did a rough prof of concept here a while ago:

https://github.com/Aftnet/Libretro_VFS/blob/master/FrontendAssembly/Class1.cpp

It does nothing, but the point is: it compiles and it allows defining an RFILE as something that is completely different from standard stdio stuff.

[ghost]

C89 issues

For example: https://github.com/libretro/RetroArch/pull/5664/files#diff-023a190a8be683b8e372108f582c8de4R121 (audio_mix.c:121), or file_stream.c:67. Variables need to all be declared together at the top of a block (right after an open brace). We have some platforms that only support C89 so this is necessary. And while C89 does not require long long support, I'm not aware of any compiler we currently use (even ones limited to C89) that don't already support it as an extension (even VC6 does).

Also, your filestream_eof() does not have the workaround I added recently that just uses feof(), to get around Windows getting stuck in an infinite loop. Have you experienced that issue or were you able to solve it? Currently I can't test because the code crashes on startup:

Error #1: UNADDRESSABLE ACCESS: reading 0x0000000000000000-0x0000000000000008 8 byte(s)
# 0 filestream_write                                   [libretro-common/streams/file_stream.c:196]
# 1 filestream_vprintf                                 [libretro-common/streams/file_stream.c:348]
# 2 filestream_printf                                  [libretro-common/streams/file_stream.c:356]
# 3 RARCH_LOG_V                                        [C:\msys64\home\bp\alberto/verbosity.c:162]
# 4 RARCH_LOG                                          [C:\msys64\home\bp\alberto/verbosity.c:182]
# 5 rarch_environment_cb                               [C:\msys64\home\bp\alberto/dynamic.c:1092]
# 6 driver_find_index                                  [C:\msys64\home\bp\alberto/driver.c:158]
# 7 path_get_archive_delim                             [libretro-common/file/file_path.c:315]
# 8 config_load_override                               [C:\msys64\home\bp\alberto/configuration.c:2943]
# 9 config_load_shader_preset                          [C:\msys64\home\bp\alberto/configuration.c:3199]
#10 libretro_dummy_retro_set_environment               [cores/dynamic_dummy.c:94]
#11 core_set_environment                               [C:\msys64\home\bp\alberto/core_impl.c:347]
#12 command_event_init_core                            [C:\msys64\home\bp\alberto/command.c:1271]
#13 command_event                                      [C:\msys64\home\bp\alberto/command.c:2159]
#14 video_viewport_get_custom                          [gfx/video_driver.c:2209]
#15 rarch_ctl                                          [C:\msys64\home\bp\alberto/retroarch.c:1405]
#16 find_location_driver                               [location/location_driver.c:106]
#17 driver_ctl                                         [C:\msys64\home\bp\alberto/driver.c:477]
#18 retroarch_main_init                                [C:\msys64\home\bp\alberto/retroarch.c:1282]
#19 snprintf                                           [C:/msys64/mingw64/x86_64-w64-mingw32/include/stdio.h:597]
Note: @0:00:00.869 in thread 6088
Note: instruction: mov    (%rax) -> %rax

[albertofustinoni]

As for 64 bit integers, don't you have a bunch of headers in common for compatibility? Or are you telling me that int64_t in the current libretro.h is not really a 64 bit integer?

[inactive123]

@albertofustinoni - The way I want you to demonstrate this to me is with an existing core that uses filestream right now. Modify it so that it uses the methods you are proposing here. Show me the changes you made to that core in your own forked repo, then I can test it from there and run it through some old RA versions.

The proof in the pudding will be if the core runs in say some old version of RetroArch that does not have VFS implemented at all. That way I have a real working example that it can work, instead of just a hypothetical.

[albertofustinoni]

@bparker06 Is there any way of getting gcc or whatever you use when compiling on Windows (I am following the instructions in the wiki and using Msys2) to compile in C89 mode? Or otherwise can you give me a list of the variables tripping the compiler up?

Also, where do I get the workaround you are talking about?

@twinaphex Well, there is the Genesis Plus GX fork I created for the original PR proposing this VFS implementation.

https://github.com/aftnet/Genesis-Plus-GX/tree/vfs_proposal

Shall we try using that one? I believe the first attempts will result in crashes due to bugs in the fallback code - to fix those though I'd really need help since you are the one who understands all the logic behind the current filestream.

[inactive123]

you can start with genesis plus gx yes.

[albertofustinoni]

And are you going to help me fixing bugs in vfs_implementation?

[inactive123]

OK, so looking at the PR, I see that you have more or less moved the contents of file_stream.c over to vfs_implementation.c and that this is what you also then bake inside cores. file_stream.c falls outside the scope of the core then and in case its actually used, it just wraps around vfs_implementation.

That approach could work indeed. If that is the way you are going about it, how about reporting the most recent file_stream to vfs_implementation.c? bpafker06 could potentially help you in your struggle to get genplusfx to work with this VFS interface. He did some work recently on file_stream.c

[ghost]

@albertofustinoni Just compile with C89_BUILD=1.

The workaround is in 28ec3ac, which I don't understand how it didn't make it when you merged with master.

And while I do see that you merged with master, I still see various other commits of mine that were removed also, and honestly I'm getting very worried that I won't be able to find them all, and that this has happened in the first place in general. For example in vfs_implementation.c:

https://github.com/Aftnet/RetroArch/blob/vfs/libretro-common/vfs/vfs_implementation.c#L193

Compare this to the current file_stream.c:

RetroArch/libretro-common/streams/file_stream.c

Line 247 in bd06668

#if defined(_WIN32) && !defined(_XBOX)

(which has the alternate _wfopen() path for Windows)

Other examples of these changes disappearing are https://github.com/Aftnet/RetroArch/blob/vfs/libretro-common/vfs/vfs_implementation.c#L406 versus

RetroArch/libretro-common/file/file_path.c

Line 923 in bd06668

int path_file_remove(const char *path)

. And for some reason

RetroArch/libretro-common/file/file_path.c

Line 966 in 13e0bd1

bool path_file_rename(const char *old_path, const char *new_path)

was not VFS-ized.

[albertofustinoni]

@bparker06 Just to make sure, the current issue is that vfs_implementation is based on an older version of filestream, is that correct? The other files have been merged properly from the current retroarch master branch.

@twinaphex If you now understand and agree with the logic behind my code, redoing the current vfs implementation to integrate fixes in filestream since I last made it makes sense. To avoid chasing a moving target though, can I ask that:

1. You keep filestream frozen while we work on it
2. Someone helps me working out the kinks to get this done as quickly as possible

?

We are going to have two PRs (three when we create one for the common repository) who need to stay in sync open at the same time. I will need all the help I can get.

[ghost]

vfs_implementation is based on an older version of filestream

That appears to be the issue, yes.

[albertofustinoni]

@bparker06 Where can I get the latest filestream? retroarch repo? common?

[ghost]

Yes from the master RA repo.

[inactive123]

I think both me and @bparker06 would agree to feature freeze file_stream for now so that you are free to work on this.

[ghost]

Yep, fine with me.

[claudiuslollarius]

Hopefully you can test now. I have:

• Integrated later changes from file_stream in vfs_implementation.
• Updated the GenesusPlusGX PR to match libretro_common as used in this PR and to be current with upstream
• Fixed the C89 issues in my files

[Alcaro]

I see little if any use for anything stat()-like. Yes, replace it with open+fstat+close (or, in out case, open+get_size+close). There are few if any usecases for just the size, most callers want to read from the file too, so they already need open(). It's not worth adding API complexity for such a rare usecase.

[inactive123]

@Alcaro Yeah, I lean similarly on this issue. It would require a lot of work adding all this functionality to the APIs.

[albertofustinoni]

@Alcaro @twinaphex

From a brief examination, here are some problematic functions in file_path:

Everything that relies on path_stat:

path_is_directory
path_is_character_special
path_is_valid
path_get_size

path_mkdir

To be honest, I do not want to add to the API myself, but there are only three solutions I can see:

• We use filestream inside of path_stat
• We add calls to the VFS to do what path_stat needs
• We remove the problematic functions from path_stat and change cores to use filestream instead

[albertofustinoni]

Also, just managed to fix GPGX! It was an issue with the pre-processor directives I was using.

Which means that, as of right now, my front end can use GPGX from the libretro repo without code changes - I can confirm the VFS does indeed work as envisioned :)

One thing I was thinking is that with the VFS in place support for things like CHD (I'm assuming they work like zip files, may be wrong) could move to Retroarch itself instead of having to replicate it for every core. Am I wrong?

[inactive123]

Which means that, as of right now, my front end can use GPGX from the libretro repo without code changes - I can confirm the VFS does indeed work as envisioned :)

That's good to hear. Try the other cores as well that I updated with VFS support yesterday.

One thing I was thinking is that with the VFS in place support for things like CHD (I'm assuming they work like zip files, may be wrong) could move to Retroarch itself instead of having to replicate it for every core. Am I wrong?

Well, it would have to be researched and discussed with the other team members (@Alcaro, @bparker06, @rtissera if he still is around? ). Possibly debating the pros and cons such a move would entail, etc.

[andres-asm]

do we have a test:/ of file:/ namespace that allows us to test functionality?

[rtissera]

I'm not sure how this would map to CHD, as CHD is rather designed to replicate low-level structure of an hard disk or optical storage (LD / CD / GD / DVD) rather than as some archive file.

[Alcaro]

One thing I was thinking is that with the VFS in place support for things like CHD (I'm assuming they work like zip files, may be wrong) could move to Retroarch itself instead of having to replicate it for every core. Am I wrong?

If you're right (I don't know either), that would indeed be possible.

But I don't like the idea of mandatory CHD reader in the fronts. The point of VFS is that cores don't know where paths come from or where they go, let's not mess with that.

It'd also make a mess out of fronts that don't or can't use -common, for example the ones that aren't C or C++.

do we have a test:/ of file:/ namespace that allows us to test functionality?

I added vfsonly:///home/alcaro/derp.jpg a while ago

I'm not sure how this would map to CHD, as CHD is rather designed to replicate low-level structure of an hard disk or optical storage (LD / CD / GD / DVD) rather than as some archive file.

File systems are just containers for many other files, just like a zip file.

Though it wouldn't surprise me if some game makes up its own filesystem driver, so it'd blow up over VFS, or even if extracted to a normal folder somewhere.

[andres-asm]

Some parts of the frontend are still not VFS aware
For instance setting the save dir to

savefile_directory = "vfsonly://D:\GameData\EmulatorData\Saves"

Ends up in the path getting reset.
So I can't test if saving on a VFS location works.

[andres-asm]

Also whilst loading content from the vfsonly testing namespace does add the entries to the list, you can't load those items from the playlist.

[anyputer]

Am I the only one here wondering what VFS even is? https://en.wikipedia.org/wiki/Virtual_file_system
I am really not sure, it sounds cool. Do you have to use the command line or can you also use the RetroArch menus?

[i30817]

Just a interested passerby... does this api looks like it's sufficient for a softpatching implementation that is independent of the cores, and will more cores support VFS than currently support softpatching?

Several cores appear to not support it because they 'need' a whole file handle (for seeking i guess) and can't just dump the whole gigabytes of ROM in memory. A softpatching implementation that actually uses disk I/O and a minimal memory overlay instead of dumping the ROM on memory and overwriting the differences seems to be unusual.

I'm also interested because i make use of external FUSE filesystems with retroarch already, so i know how useful they can be. I use a squashfs (master compressed game filesystem) combined with a copy-on-write filesystem in order to keep the computer emulation cores RA supports from touching the computer games 'ROMs' that need writing.

[Alcaro]

Yes, this one can be used to implement both overlay-softpatch and copy-on-write, and it would work in all VFS-enabled cores.

The IPS, UPS and BPS formats aren't designed to be overlaid like that (BPS, with its backreference support, would be especially annoying), but with some clever engineering, it would indeed be possible.

[i30817]

I don't mind converting my softpatches to something more viable. I'm surprised that bps is the worst for this, being the most recent.
To be clear, the thing that hurts the most are the cores that don't support softpatching in small roms (like megadrive) because the core 'needs full_path'. I think this is because the same cores support large isos (sega cd in this case), that seems to be the correlation. With iso patching i'm currently using the approach of keeping a reverse patch. I wouldn't say no to a softpatching that supported large ROMs though.

It might actually be better disallowing softpatching for 'ROMs' which aren't read-only in computer cores and make people depend only on hardpatches and reversal patches here so the copy-on-write keeps working as expected? Stacking these two VFS might be hard to keep consistent because of updates to the patch file not touching the actual rom.

You can to keep some kind of 'touched original file' record on the copy-on-write otherwise and reset the cache automatically/ask to reset the cache when it changed, that would be a good optional feature for cow VFS - the failure case, a file with the same timestamp but different contents - is is unusual and not so bad, just 'keep using the cache', and it's certainly faster than checking a checksum every load.

I suppose you can make this feature possible with stacking by making the softpatch VFS return the modified time that is most recent of the two files (patch and source) too.

[Alcaro]

Yes, softpatching small files is easy. And yes, that sounds likely; libretro doesn't support saying 'needs fullpath for ISOs, but not SMDs', so this is the best we can do. And yes, we should hook up our current softpatchers to the VFS; full-RAM softpatching is a lot better than no softpatching.

None of the three were designed for low-RAM softpatching, so how well they work is mostly luck. Due to their simplicity, IPS and UPS are doable, but BPS, being newer and more complex, is trickier. Not impossible - I'll probably do it at some point if I'm bored enough - but not easy either.

I could explain the technical details, but that'd be rather offtopic here.

[Alcaro]

/* Set the current read/write position for the file. Returns the new position, -1 for error.
* Introduced in VFS API v1 */
typedef int64_t (RETRO_CALLCONV *retro_vfs_seek_t)(struct retro_vfs_file_handle *stream, int64_t offset, int seek_position);

int64_t retro_vfs_file_seek_impl(libretro_vfs_implementation_file *stream, int64_t offset, int seek_position)
{
[...]
return retro_vfs_file_seek_internal(stream, offset, whence);
}

int64_t retro_vfs_file_seek_internal(libretro_vfs_implementation_file *stream, int64_t offset, int whence)
{
[...]
return fseek(stream->fp, (long)offset, whence);
[...]
}

C89 spec, 7.9.9.2 (C99 is identical, 7.19.9.2):

int fseek (FILE *stream, long int offset, int whence);
The fseek function returns nonzero only for a request that cannot be satisfied.

-> fseek returns 0 on success -> our retro_vfs_seek_t always returns 0 on success -> retro_vfs_file_seek violates its specification -> that's a bug.

The obvious fix would be slapping a ftell in retro_vfs_file_seek_impl, but I suspect other backends may end up implementing the same bug, and I'm not interested in playing whack-a-mole forever.

Instead, I propose replacing that return value with a success flag. This will simplify implementations, at minimal impact to anything else; I grepped the entire RetroArch source for fseek and filestream_seek, and the only ones where the return value was neither discarded, compared to 0 or compared to -1 shouldn't have been seek at all (probably written before tell/get_size existed?).

Any objections?

[albertofustinoni]

@Alcaro
Are you proposing we change the VFS spec so that retro_vfs_seek_t returns 0 for success and non-zero for failure just as the standard fseek function? If so, I agree.

[Alcaro]

That is correct. More specifically, failure would be -1, just like the other VFS functions.

I checked the return values of the four current success-flag functions. close is correctly implemented. flush returns fflush(), which is zero/nonzero, not 0/-1. rename returns rename(), which likewise is zero/nonzero. remove returns true/false, aka the exact opposite of what it should! Fixed per a8359a8.

Maybe we should change them all to return zero/nonzero, to avoid future issues of that kind. Or maybe change them all the way to returning bool, and hope some compiler warnings or static analysis tools will yell at people who try to use tell()s return value, and for consistency with the rest of libretro. I have no strong feelings about that one.

[inactive123]

Yes, and that pull request thread is mainly used for discussion now - the VFS code has already been committed, and API hooks have already been added. So theoretically the interface has already been done and implemented into the latest versions of RetroArch. Retrix and RetroArch are two of the libretro frontends that already implement it.

[ghost]

Is an objective move all the active cores to a VFS compatible (more or less)?

Because now PicroDrive (specially for 32x) or blueMSX doesn't work on path with special characters (like I reported here: #6051)

In previous versions PPSSPP doesn't worked too, but now it works fine! And Genesis Plus GX is working too (but it doesn't emulate 32x)

[inactive123]

Picodrive has not been updated for VFS yet I believe. Maybe that is why.

[inactive123]

I believe we can comfortably close this now.

[andres-asm]

while the PR is not valid, some comments are valid, would be good to move them to an issue or something.