Re: [AD] compressing empty files

["Eric Botcazou" <ebotcazou@xxxxxxxxxx>]

> 1. First this, which is straightforward (and was discovered after fixing the
>    EOF problem):
>
>    AL_INLINE(int, pack_getc, (PACKFILE *f),
>    {
>       f->buf_size--;
>       if (f->buf_size > 0)
>          return *(f->buf_pos++);
>       else
>          return _sort_out_getc(f);
>    })
>
>    We have one byte left in the buffer, none in the disk. So f->buf_size is 1.
>    The f->buf_size becomes 0, and fails the check. We call the _sort_out_getc
>    (which fills the pack buffer).

No, _sort_out_getc() only fills the pack buffer if f->buf_size < 0. But
_sort_out_getc() has an annoying behaviour: it sets the PACKFILE_FLAG_EOF
flag if f->buf_size == 0, which in turn causes pack_feof() to return TRUE.
This behaviour is inconsistent with ISO C feof(), as Vincent and Javier pointed out
more than one year ago (and AFAIK the ISO sub-committee hasn't modified its
standard to conform to Allegro since then ;-)

> So, I suggest either postdecrementing in the check or checking for greater
> or equal to 0.

The latter is the best solution I think. This leads to basically throwing
away _sort_out_getc(). Moreover, pack_fread() should call pack_getc() to
avoid duplicate code.

> 2. Now, most file.c rely on pack_getc() returning EOF. However, that is not
>    true (maybe a bug?). Check the pack_getc() again. The _sort_out_getc() is
>    called (as well as in a few more places):
>
>    int _sort_out_getc(PACKFILE *f)
>    {
>       if (f->buf_size == 0) {
>          if (f->todo <= 0)
>             f->flags |= PACKFILE_FLAG_EOF;
>          return *(f->buf_pos++);
>       }
>       return refill_buffer(f);
>    }
>
>    If the packfile is completely empty (no bytes left in the buffer, no bytes
>    left in the disk), it just sets the PACKFILE_FLAG_EOF flag on, but returns
>    *(f->buf_pos++). What is inside there? As far as I checked, the buffer has
>    not been filled with EOF. Check this at pack_fread():
>
>       i = _sort_out_getc(f);
>       if (i == EOF)

You have been caught by the ill-behaviour of _sort_out_getc() that sets the
PACKFILE_FLAG_EOF flag one byte too early (*f->buf_pos still has valid data
at that time). It does actually return EOF on the next call via refill_buffer()
because of:

static int refill_buffer(PACKFILE *f)
{
   int i, sz, done, offset;

   if ((f->flags & PACKFILE_FLAG_EOF) || (f->todo <= 0)) {
      f->flags |= PACKFILE_FLAG_EOF;
      return EOF;
   }
[...]
}

> 3. Finally, refill_buffer() ends this way:
>
>       f->todo -= f->buf_size;
>       f->buf_pos = f->buf;
>       f->buf_size--;
>       if (f->buf_size <= 0)
>          if (f->todo <= 0)
>             f->flags |= PACKFILE_FLAG_EOF;
>
>       return *(f->buf_pos++);
>
>    So, if both the buffer and the disk are empty, turns the PACKFILE_FLAG_EOF
>    on, but still returns the following position, which might or mightn't be EOF.

Yes, another occurence of the problem with the PACKFILE_FLAG_EOF flag.

> --- cvs/allegro/src/file.c      Wed Mar  6 09:50:12 2002
> +++ mod/allegro/src/file.c      Fri Jun 28 23:13:44 2002
> @@ -2083,7 +2083,7 @@
>     if (f->buf_size == 0) {
>        if (f->todo <= 0)
>   f->flags |= PACKFILE_FLAG_EOF;
> -      return *(f->buf_pos++);
> +         return (EOF);
>     }
>     return refill_buffer(f);
>  }

Unnecessary because _sort_out_getc() will never be called when f->buf_size == 0.

> @@ -2153,8 +2153,10 @@
>     f->buf_pos = f->buf;
>     f->buf_size--;
>     if (f->buf_size <= 0)
> -      if (f->todo <= 0)
> +      if (f->todo <= 0) {
>   f->flags |= PACKFILE_FLAG_EOF;
> +         return (EOF);
> +      }
>
>     return *(f->buf_pos++);

Exactly the same hunk as version #1 of Peter's patch.

> --- cvs/allegro/include/allegro/inline/file.inl Tue Nov  6 13:06:46 2001
> +++ allegro/include/allegro/inline/file.inl Fri Jun 28 22:40:38 2002
> @@ -26,7 +26,7 @@
>  AL_INLINE(int, pack_getc, (PACKFILE *f),
>  {
>     f->buf_size--;
> -   if (f->buf_size > 0)
> +   if (f->buf_size >= 0)
>        return *(f->buf_pos++);
>     else
>        return _sort_out_getc(f);

I agree. I'd just suggest to swap the if-then-else to follow the layout of
pack_putc().

> I tested it under DJGPP, and worked (pack input packed; pack u packed output ->
> output is equal to input, being it 0, 1 or 2 bytes, more test encouraged ;).

Are you sure for 1-byte files ? If I understand correctly the problem with
version #1 of Peter's patch, yours has the same problem: 1-byte files are
turned into empty files.

> There seems to be a small recursion (pack_getc -> _sort_out_getc ->
> refill_buffer -> pack_getc -> _sort_out_getc -> refill_buffer, which breaks the
> recursion returning EOF). I think this behaviour is also done without this
> patch. Maybe someone should look at that.

The recursion is simply there to unwind the packfile hierarchy, which should
be acyclic if the user didn't screw it up.

--
Eric Botcazou
ebotcazou@xxxxxxxxxx