Re: [hatari-devel] Replacing direct file system access with an abstraction layer?

[Nicolas Pomarède <npomarede@xxxxxxxxxxxx>]

Le 31/08/2023 à 23:05, Brad Smith a écrit :
>  > #define File_ftell ftell
>
> I see that as a potential "quick" solution to trying to make an 
> adaptation like mine, but I don't think it's beneficial in the long term.
>
> You mentioned one case, but there are several places in the Hatari code 
> that are trying to redirect low level functions like this with #define, 
> and it was actually a bit complicated to sort out which 
> functions/structures were really secretly something else. There are 
> places in the code that are using file functions/structures that have 
> the same names but are actually incompatible renames of different 
> things, independent workarounds.
>
> At least for my adaptation, I much prefer to see what I am replacing, 
> rather than #define and hope it's doing the right thing globally. It 
> allowed me to review exactly what I had to replace and why. I think 
> trying to use yet another layer of #define renames on top would only 
> make future maintenance even more difficult and error prone.
>
> I feel like the long term goal should be to remove such #define renames 
> where possible, allowing them only as an interim solution, or where it's 
> otherwise impractical.
>
> So, what I am proposing is not a #define rename. I'm proposing to 
> replace all direct filesystem use with an explicit Hatari-provided 
> interface. I would rename fopen in-place and not hide its redirection. 
> We wouldn't have some code using fseek and some code using fseeko. All 
> the posix filesystem implementation can go into a single code file 
> providing the interface implementation, and the majority of code would 
> only need to know about "file.h". We would need far fewer 
> platform-specific library includes and workarounds in many files.
>
> For the most part it would simply a function like fopen with something 
> with a very similar name. Make it all go through one interface so that 
> it's easier to audit and maintain as a whole.
>
> Performance-wise, the wrapper function for the abstraction would become 
> a very small overhead, but I think a trivial one? Even if it wasn't, 
> actual file system calls are sparse and not performance critical.
>
> That's my question, whether anyone else would feel that's worth doing? 
> I've already had to gather most of the information needed, and my 
> adaptation basically accomplishes it by different means, but I want to 
> know if the main project would want to have this change before investing 
> time into it.

Hi

I also agree that using define can have unwanted side effect, as you 
might end up with a define that replaces another define that replaces 
the real system call and it makes everything more difficult to 
follow/debug and can lead to some bugs if everything is not replaced in 
the correct order.

That being said, maybe the best to discuss this would be to look at 
changes you did in Hatari+retroarch and see how this could be merged or not.

Doing a diff between hatari 2.4.1 and the hatari sources in your 
retroarch, I see several cases :
 - file access using the classic stdio.h (fopen, fread, fflush, ...)
 - file access using the posix way (open, read, write, ...)


I see those file access functions are mainly in gemdos.c, hdc.c, 
inffile.c, ... Not that much files need to be changed.

for the 1st case, retroarch uses a "corefile *" structure, while hatari 
uses the direct "FILE *" structure :

+#ifndef __LIBRETRO__
        FILE *FileHandle;
+#else
+       corefile* FileHandle;
+#endif
[...]
+#ifndef __LIBRETRO__
        fflush(FileHandles[Handle].FileHandle);
+#else
+       core_file_flush(FileHandles[Handle].FileHandle);
+#endif

so we could go with :

#ifndef __LIBRETRO__
typedef FILE* H_FILE;

int h_fflush ( H_FILE *flux )
{
  return fflush ( flux );
}
#else
typedef corefile* H_FILE;

int h_fflush ( H_FILE *flux )
{
  return core_file_flush ( flux );
}
#endif

-> we need to change the parameter type and add some typedef (note that 
we already do some conditional type on FILE ourselves in 
memorysnapshot.c to have MSSFILE, so it's not something that we never did)


for the 2nd case we have for example :

+#ifndef __LIBRETRO__
        if (stat(filename, &filestat) != 0)
+#else
+       if (core_file_stat_system(filename, &filestat) != 0)
+#endif

-> here we just need to have a wrapper function that that take the same 
argument

int h_stat ( const char *path , struct stat *pstat )
{
+#ifndef __LIBRETRO__
  return stat ( path , pstat );
#else
  return core_file_stat_system ( path , pstat );
#endif
}


Same with this part if we want to have a common h_ftello() function :

+#ifndef __LIBRETRO__
                offset = ftello(handle->FileHandle);
                stat(handle->szActualName, &fstat);
+#else
+               offset = core_file_tell(handle->FileHandle);
+               core_file_stat_system(handle->szActualName, &fstat);
+#endif


Another example with opendir :

+#ifndef __LIBRETRO__
        DIR *dir;
+#else
+       coredir* dir;
+#endif

+#ifndef __LIBRETRO__
        dir = opendir(path);
+#else
+       dir = core_file_opendir_system(path);
+#endif


Another with fseeko :

+#ifndef __LIBRETRO__
            fseeko(dev->image_file, (off_t)dev->nLastBlockAddr * 
dev->blockSize, SEEK_SET) == 0)
+#else
+           core_file_seek(dev->image_file, (off_t)dev->nLastBlockAddr * 
dev->blockSize, SEEK_SET) == 0)


Knowing the pain it's sometimes to merge patches from WinUAE into Hatari 
because we have places with specific code that prevents the patches to 
apply directly, I'm always grateful to Toni when he agrees to make 
changes to WinUAE to ease merging changes.

S I would not be against adding an abstraction level for these 
functions, as long as it keep an explicit name and leave the code 
readable as it was before.


Nicolas