Re: [AD] file interface fopen

[ Thread Index | Date Index | More lists.liballeg.org/allegro-developers Archives ]


On December 18, 2010, Matthew Leverton wrote:
> I've been working with A5's file interfaces, namely creating an A4
> PACKFILE / DATAFILE loader, and the current setup of creating file
> interfaces bugs me. I recently proposed al_fopen_f on a.cc, but after
> playing around some more, that's not really what I want.
> 
> The problem in a nutshell is that the file interface is responsible
> for creating its own structure and therefore must have access to the
> Allegro internals, which of course is bad. So my proposal is:
> 
> * have Allegro internally be responsible for creating the ALLEGRO_FILE
> struct, and populating the vtable
> * pass that structure to the fi_fopen hook
> * if fi_fopen returns false, destroy the struct, and return NULL
> 
> In addition to that, there's a new private field on the ALLEGRO_FILE
> struct, called void* userdata, accessible via:
> 
> * al_get_file_userdata(file)
> * al_set_file_userdata(file, ptr)
> 
> Also, there's a new way to initialize a file (with the vtable set to
> the current default one):
> 
> * ALLEGRO_FILE al_create_file();
> 
> Now that function doesn't always make sense (and in fact would crash
> subsequent calls with the stdio/physfs systems since they assume an
> extended struct), but the file interface struct could contain a bool
> to enable/disable this behavior.
> 
> So, for example, the memfile addon looks like in al_open_memfile:
> 
> al_store_state(&state, ALLEGRO_STATE_NEW_FILE_INTERFACE);
> al_set_new_file_interface(&memfile_vtable);
> memfile = al_create_file();
> al_set_file_userdata(memfile, userdata);
> al_restore_state(&state);

I think it'd be cleaner to provide a al_create_file_vt() or some such, that 
passes in the vtable. Instead of all that messing with state junk.

> and
> 
> static size_t memfile_fread(ALLEGRO_FILE *fp, void *ptr, size_t size)
> {
>    ALLEGRO_FILE_MEMFILE *mf = al_get_file_userdata(fp);
> ...
> 
> It no longer needs any access to the internals.
> 
> It might be useful to integrate the userdata into the al_create_file,
> such that the hook can validate it and return true/false. Also,
> perhaps the set_file_userdata() could also optionally call a hook that
> notifies the interface that the userdata changed.
> 
> I've attached a patch against 5.1. It's designed to not disrupt
> anything in 5.0... in fact, this implementation could wait until 5.2.
> 
> It simply adds "fi_fopen2" which is given preference over "fi_fopen"
> in the al_fopen function. I didn't feel like updating the physfs and
> stdio drivers from the current hacky struct inheritance to the
> userdata pointer.
> 
> The patch may have some bugs ... I didn't test it.
> 
> (Also, I've never looked at ALLEGRO_FS_INTERFACE, so I don't know if
> there's any sort of similar issue with that.)

I think if this is going to happen, it'd be better to just do now, and convert 
the stdio and physfs code to use the new setup.

-- 
Thomas Fjellstrom
tfjellstrom@xxxxxxxxxx




Mail converted by MHonArc 2.6.19+ http://listengine.tuxfamily.org/