al_save_config_file_f() returns true when it shouldn't

no-reply@allegro.cc (Bruce Pascoe) — Tue, 16 Jun 2015 10:24:27 +0000

I have the following code to save out config files in my engine:

#SelectExpand
  1next_buf_size = 4096;
  2while (!is_aok) {
  3  free(buffer); buffer = malloc(next_buf_size);
  4  memfile = al_open_memfile(buffer, next_buf_size, "wb");
  5  next_buf_size *= 2;
  6  is_aok = al_save_config_file_f(memfile, file->conf);
  7  file_size = al_ftell(memfile);
  8  al_fclose(memfile);
  9}
 10if (!sfs_fspew(g_fs, file->path, "save", buffer, file_size))
 11  goto on_error;
 12free(buffer);

This is needed because games run in my engine may have their assets either on disk or compressed into a wad-like package and my sfs_*() routines abstract that so that everything works in both cases. So I figured, okay, I could use a memfile to stage everything first, then call sfs_fspew() to write it out to whereever. Except for one small problem: al_save_config_file_f() returns true even if the buffer is too small to hold the entire file (try setting next_buf_size to 1 before the loop, it still "succeeds"), so my code is broken.

Any way I could get around this, or if it could perhaps be fixed in Allegro?

no-reply@allegro.cc (SiegeLord) — Tue, 16 Jun 2015 11:18:16 +0000

Depending on how this error manifests itself, this can go both ways. Allegro doesn't check the return value of the file writing functions (shame on it) but it does check al_ferror ~~which you could set~~ (EDIT: nvm, you're using the memfile addon). I'd say Allegro's at the fault here, however.

EDIT: Another thing you might be able to do in a pinch is call al_feof on the file. It'll return true if an attempt was made to write past its size.

no-reply@allegro.cc (Bruce Pascoe) — Tue, 16 Jun 2015 11:40:32 +0000

Thanks for the tip about al_feof, I didn't think to do that. I generally don't use feof myself because fread/fwrite returning a size less than what was requested generally means the same thing. But for this case it's a useful workaround.

Regardless, al_save_config_file_f should really be returning false in this situation.

no-reply@allegro.cc (beoran) — Tue, 16 Jun 2015 20:56:16 +0000

Yes, it's a good idea. So why not contribute a patch?

no-reply@allegro.cc (Bruce Pascoe) — Wed, 17 Jun 2015 00:26:20 +0000

I would, but I'm not sure where to submit it to. The GitHub repo is considered a mirror, so I don't know if it would be acceptable to send a pull request there.

no-reply@allegro.cc (Thomas Fjellstrom) — Wed, 17 Jun 2015 00:31:24 +0000

I think its ok to do a github pull request, but you can just send a patch to the Development forum here, or the AD mailing list.

no-reply@allegro.cc (SiegeLord) — Wed, 17 Jun 2015 07:13:34 +0000

I even prefer a pull request, as it'll run the TravisCI/AppVeyor checks.

no-reply@allegro.cc (Bruce Pascoe) — Wed, 17 Jun 2015 21:22:09 +0000

I'll post the patch here because Git wants to commit all the CMake cruft and I don't feel like dealing with it at the moment.

See attached.

no-reply@allegro.cc (SiegeLord) — Thu, 18 Jun 2015 09:41:31 +0000

Thanks, applied. It took me awhile to do so, as that file was encoded in UTF-16 .

no-reply@allegro.cc (Bruce Pascoe) — Thu, 18 Jun 2015 10:28:39 +0000

Huh, wonder how that happened. All I did was:

git diff src/config.c > config.patch

Although that was done from PowerShell (the default shell for GitHub for Windows), maybe PS outputs UTF-16 when redirecting output? Pretty dumb of it if that's the case.

Sorry about that.