Re: [pdf-devel] Missing stream backend error conditions.

[Michael Gold]

On Mon, May 18, 2009 at 16:44:15 -0300, gerel wrote:
> 
> Hi,
> 
> After modifying the Fsys API I noticed that stream backends considered an EOF
> and an error as the same thing (i.e. zero-length write/read).  I attach a 
> patch
> to distinguish one from the other.
...
>   pdf_stm_be_cfile_read (pdf_stm_be_t be,
...
> +  *read_bytes = fread (buffer, 
> +                       1, bytes,
> +                       be->data.cfile.file);
>  
> -  return readed_bytes;
> +  if (feof(be->data.cfile.file))
> +    {
> +      ret = PDF_EEOF;
> +    }
> +  else if (ferror(be->data.cfile.file))
> +    {
> +      ret = PDF_ERROR;
> +    }
> +  return ret;

I think this code can return PDF_EEOF on a successful read (i.e. when
you read some bytes at the end of a file).  PDF_OK should be returned if
any bytes were read, even if you're now at the end of the file.

...
> === modified file 'src/base/pdf-stm-filter.c'
...
> +      ret = pdf_stm_be_read (filter->backend,
> +                             filter->in->data,
> +                             filter->in->size,
> +                             &read_bytes);
> +
> +      if (ret != PDF_ERROR)
> +        {        
> +          filter->in->wp = read_bytes;
> +          if (read_bytes < filter->in->size)
> +            {
> +              ret = PDF_EEOF;
> +            }

The condition (read_bytes < filter->in->size) has no documented
significance, i.e. it's not documented to mean EOF (or error).
In addition, it has the same bug I mentioned above.

This check should be removed, and the backend should return PDF_EEOF
when necessary.

...
> +pdf_stm_read (pdf_stm_t stm,
...
> +  if ((*read_bytes == bytes) &&
> +      (ret == PDF_EEOF))
> +    {
> +      /* Avoid a false PDF_EEOF in the current operation */
> +      ret = PDF_OK;
> +    }

This check (*read_bytes == bytes) doesn't make sense to me.  If you get
any data at all, PDF_OK should be returned.

Also, the text "If this value is less than bytes then an end of file
condition occurred" should be removed from this function's
documentation (and EAGAIN added as an error code).  A short read can
occur before EAGAIN, but this doesn't mean anything.

For this function and all similar ones, PDF_EEOF should only be returned
when 0 bytes were read, and the end of file was reached (we don't need
to set *read_bytes in this case, unless that's documented to happen for
all error types).

...
> +pdf_stm_write (pdf_stm_t stm,
...
> +  if ((*written_bytes == bytes) &&
> +      (ret == PDF_EEOF))
> +    {
> +      /* Avoid a false PDF_EEOF in the current operation */
> +      ret = PDF_OK;
> +    }

Does PDF_EEOF make sense when writing a file?  In any case, I think this
check is invalid as above.

...
> +pdf_stm_flush (pdf_stm_t stm,
...
> +      if (written_bytes != cache_size)
> +        {
> +          /* Could not write all the contents of the cache buffer into
> +             the backend */
> +          stm->cache->rp += written_bytes;
> +          ret = PDF_EEOF;
> +          break;

This function documentation says PDF_EEOF means "A disk full condition
occurred", which
 - is properly represented by PDF_ENOSPC
 - is not what's detected above

The condition (written_bytes != cache_size) has no documented meaning
(for pdf_stm_be_write).  If pdf_stm_be_write returns PDF_OK,
pdf_stm_flush should keep trying to write more data until an error is
returned (or all data is flushed).  Note that a short write can
legitimately occur before EAGAIN, and shouldn't be treated as an error.

...
> +pdf_stm_read_peek_char (pdf_stm_t stm,
...
> +  /* Is the cache empty? */
> +  ret = PDF_OK;
> +  if (pdf_buffer_eob_p (stm->cache))
> +    {
> +      ret = pdf_stm_filter_apply (stm->filter, PDF_FALSE);
> +    }
> +
> +  if (pdf_buffer_eob_p (stm->cache))
> +    {
> +      ret = PDF_EEOF;
> +    }
> +  else
> +    {
> +      /* Read a character from the cache */
> +      *c = 
> +        (pdf_u32_t) stm->cache->data[stm->cache->rp];
> +
> +      if (!peek_p)
> +        {
> +          stm->cache->rp++;
> +        }
> +
> +      if (ret == PDF_EEOF)
> +        {
> +          /* Avoid a false PDF_EEOF */
> +          ret = PDF_OK;
> +        }
> +    }
...

If pdf_stm_filter_apply returned PDF_EEOF, pdf_buffer_eob_p should be
true, which makes that final check redundant (assuming the other changes
I've suggested are made).

-- Michael

signature.asc
Description: Digital signature