Re: [AD] Clarifying and revising filesystem API

[Thomas Fjellstrom <tfjellstrom@xxxxxxxxxx>]

On Wed April 15 2009, Elias Pschernig wrote:
> Peter Wang wrote:
> > * ALLEGRO_MAKE_TEMP_REMOVE_ON_OPEN
>
> That confuses me a bit... wouldn't it make more sense to say CLEAR_ON_OPEN?

Its actually removing the file. "clear" doesn't really do the operation 
justice.

> > *(Q) As discussed, I think these should be removed*
>
> I still think so. Let's leave this to the physfs addon (and it will be up
> to the addon author whether to hide physfs and provide wrappers for the
> search path stuff, or have users call physfs functions and just provide
> some glue).

Again, I disagree. I added it because ACTUAL PEOPLE wanted it. The fact that 
it also supports a physfs operation directly, all the better.

> > *(Q) Why not just use al_close_file_handle or al_destroy_file_handle?*
> > *(Q) At least in the stdio version, this is no different to
> > al_open_file_handle followed by a check that the path was a directory, so
> > maybe this is not required?*
>
> There may be a reason - but right now, I also don't see it. If an
> ALLEGRO_FILE_ENTRY is a directory, opening/closing it should always be the
> same as al_open/close_dir. So those are indeed not needed.
>
> > *(Q) Peter Hull points out there is no rewind.*
>
> We could make al_fseek work with it. And independent of that an al_rewind()
> which just does al_fseek(0) does not sound bad either.
>
> > ## API: al_remove_file
> >
> > *(Q) Do we need this?*
>
> Why not. If my game removes a save-file, I probably have the location
> available as a path... so this makes it easier than having to construct a
> file handle first.

You might as well keep the handle around for it then no?

> > ## API: al_update_file_handle_info
> >
> > *(Q) Does this need to be called before al_get_file_handle_mode, etc.?*
>
> I'd say no. Those functions should check a flag to see if stat() info is
> available and if not call it. The only use for al_update_file_handle_info
> would be if you suspect something might have changed since the last time
> you called any of those functions (and therefore stat was called and the
> info cached).

Indeed. And I don't think any of the functions should automatically update it. 
stat is a rather heavy function. Should only update if and when the user says 
they want to.

> > ## API: al_file_exists
> > *(Q) I think this is unnecessary*
>
> We already have al_path_exists, so yeah. One question is, would those two
> always have done the same?
>
> > ## API: al_file_is_regular
> > *(Q) I think this is unnecessary*
> > ## API: al_file_is_directory
> > *(Q) I think this is unnecessary*
> > ## API: al_get_file_mode
> > *(Q) I think this is unnecessary*
> > ## API: al_get_file_atime
> > *(Q) I think this is unnecessary*
> > ## API: al_get_file_ctime
> > *(Q) I think this is unnecessary*
> > ## API: al_get_file_mtime
> > *(Q) I think this is unnecessary*
> > ## API: al_get_file_size
> > *(Q) I think this is unnecessary*
>
> Not sure, but they all seem uncommon enough that you could just as well
> create a file handle and query the info using that.

Thats what the implementations do anyhow ;)

> > ## API: al_fclose
> >
> > *(Q) Why not just make this a synonym for al_destroy_file_handle?*
>
> Or remove it completely and rename al_fopen to al_open_file.

They do different things... al_fopen and al_fclose a slightly different 
wrappers to open_handle and close_handle. The former two could possibly be 
removed, but the later two are complex enough that no, you don't want to just 
remove them.

> > ## API: al_feof
> >
> > Returns true if we have an end of file condition.
>
> I'd like a note here to explicitly define the behavior when fungetc was
> used.
>
> > ## API: al_fgets
>
> I think there's another thread about it and it's still unresolved.
>
> > *(Q) Consider naming this with 'byte' in the name, as char != byte since
> > forever.*
>
> Indeed. I would be fine with al_read_byte, libc names tend to be rather
> cryptic.

So is its argument ordering.

> > ## API: al_fread_32le
> >
> > Reads a 32-bit word in little-endian format (LSB first).
> >
> > Returns:
> > The read 32-bit word or EOF on error.
>
> I think this should be:
>
> """
> Returns the read 32-bit word. If an error occurs the return value is
> undefined. """
>
> The reason is, we apparently can't return anything here which isn't also a
> valid return value. So we should not tempt the user to check for EOF or 0
> or whatever before assuming an error (and consequently forget to do proper
> error checking).
>
> > ## API: al_fwrite_32le
> >
> > Writes a 32-bit word in little-endian format (LSB first).
> >
> > Returns:
> > The written 32-bit word or EOF on error.
>
> I would prefer this to either return void or the number of bytes written
> (which would be either 4, or 0 on error).
>
> > ## API: al_fseek
> > ## API: al_ftell
>
> We could make those work with directories (in the stdio driver, use
> "seekdir" and "telldir" from libc). Or if not, we probably should state
> that it's an error to call them on non-file entries.
>
> > ## API: al_change_directory
> >
> > Changes the current working directory to 'path'.
> >
> > Returns true on success, false on error.
> >
> > *(Q) need to figure out what to do with this if there are multiple
> > filesystems in effect; which filesystem's version of the "current
> > directory" is changed?*
> > *(Q) need to consider if this should _actually_ change the process'
> > working directory when using stdio driver, or just change it logically*
> >
> > *(Q) threads*
>
> Yeah. This is very closely related to the search path issue of course. *If*
> we keep a logical current directory anyway, then it actually would be
> tempting to allow an addon to do something like:
>
> al_zip_set_current_archive("data.zip")
>
> And then all paths would be interpreted to inside the zip file... I feel we
> discuss the same thing on multiple places now though...
>
> > ## API: al_get_path_separator
> >
> > *(Q) need to figure out what to do with this if there are multiple
> > filesystems in effect, each with potentially its own syntax*
>
> How relevant is the path separator btw.? Would things break if we always
> use /? I found it a bit cumbersome to have to use:
>
> al_path_to_string(path, al_get_path_separator())
>
> Maybe we chould have a function like this:
>
> ALLEGRO_FILE_HANDLE *al_create_file_handle_from_path(ALLEGRO_PATH *path)

Its basically because I didn't bother to fix up that api. At different times 
Allegro, and the user will want to make fshook or OS style paths, all with the 
same function.

An addon may want to change the os path separator because it deals with it 
itself. So thats why the drive/path _sep functions, and the path_to_x 
functions exist. So an addon can override everything. And fshooks should be 
using those functions itself. Due to lack of interest I never got around to 
finishing all that up. Needed input, barely got any.

-- 
Thomas Fjellstrom
tfjellstrom@xxxxxxxxxx