[Nautilus-list] Re: Eel patch

[Darin Adler <darin bentspoon com>]

on 9/18/01 1:28 PM, Alex Larsson at alexl redhat com wrote:

> Index: eel/eel-vfs-extensions.c
> ===================================================================
> RCS file: /cvs/gnome/eel/eel/eel-vfs-extensions.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 eel-vfs-extensions.c
> --- eel/eel-vfs-extensions.c    2001/05/03 16:41:24    1.3
> +++ eel/eel-vfs-extensions.c    2001/09/18 20:16:50
> @@ -63,15 +63,17 @@ static void read_file_read_chunk (EelRea
> #endif
> 
> GnomeVFSResult
> -eel_read_entire_file (const char *uri,
> -               int *file_size,
> -               char **file_contents)
> +eel_read_entire_file_with_limit (const char *uri,
> +                 int *file_size,
> +                 char **file_contents,
> +                 GnomeVFSFileSize max_size)

I'm thinking that either file_size should be changed to be
GnomeVFSFileSize*, or max_size should be an int. The idea here is that it's
not a good idea to read a whole file into a buffer if you are going to read
more than 2^31 bytes, so the sizes don't have to be expressed as 64-bit or
even unsigned 32-bit quantities. But we should at least be consistent, I
think.

Here on HEAD, I think it would be reasonable to only have the version that
includes the limit. It doesn't seem necessary to have a convenience cover
that provides an arbitrary 100 Kb limit. On the gnome 1 branch, we
definitely need the two calls as you have done here.

> +    /* Possibly allocate extra byte for zero termination */
> +    if (total_bytes_read == 0) {
> +        g_free (buffer);
> +        buffer = NULL;
> +    } else {
> +        buffer = g_realloc (buffer, total_bytes_read + 1);
> +        buffer[total_bytes_read] = 0;
> +    }
> +    

Is there a reason we don't want to return a 1-byte buffer with a 0 byte in
it for the 0-bytes-read case? I think an empty string is probably better
than a NULL for that case.

Should we return some kind of error when we hit the max size, rather than
returning a partial file and GNOME_VFS_OK? I think so. Otherwise we have a
"read first N bytes of file" function, not "read entire file with limit to
avoid problems" function.

I'd also like to see some comment documenting this 0 byte at the end of the
buffer. I'm sorry that there isn't already a function comment to add this
to, but you could at least put that at the top of the function where the
function documentation comment belong.

> +    *file_contents = buffer;
> +
> +    /* Zero terminate file */

This comment seems out of place.

> +    /* Pick random filesize limit of 100Kb */

I think the word "arbitrary" would be better here. And it's reasonable to
explain why 100 Kb is a good choice. We don't need to be so modest about our
arbitrary choices.

    -- Darin