Re: Big files upload

On Mon, May 12, 2008 at 12:14:52AM +0300, Kalle Olavi Niemitalo wrote:
> Witold Filipczyk <[email protected]> writes:
> 
> > The big_files-1008-r branch is derived from master. On that branch
> > there is the code handling big file uploads.
> 
> Thank you.  It is easier for me to review a branch than a set of
> patches.

It is a new tradition "branch per feature" :)

> In addition to what I posted as comment #4 in bug 1008, I see
> these potential problems:
> 
> - The code is duplicated between src/protocol/file/cgi.c and
>   src/protocol/http/http.c.  This may be the best way but it
>   looks a bit annoying.

I have no idea how to make it better.

> - It is not clear how http_connection_info.post_fd is closed if
>   the user aborts the connection.  Is http_end_request() called
>   in that case?  It seems not.

I moved the post_fd to the struct connection. Now, the post_fd
is closed in many places (too many probably).

> 
> - send_big_files() should check whether the open(big_file + 1,
>   O_RDONLY) call succeeds.

I added a check.

> - If something changes the size of the file while ELinks is
>   sending it, then it seems possible that the amount of data
>   posted does not match the generated Content-Length header.
>   ELinks should detect this and report an error instead of
>   annoying the server with an invalid request.  (Alternatively,
>   send the POST request in chunked encoding, so that the size
>   need not be known in advance.)

I skip this. It's too difficult to implement right now.

> - You moved struct http_connection_info from
>   src/protocol/http/http.c to src/protocol/http/http.h but left
>   the related macros and comments in http.c.  Please keep them
>   together, or at least add a comment in the definition of the
>   structure, saying that the values of those macros are used.

I added some comments.

> - In struct uri, there's a comment /* Number of files bigger than
>   1M */ but the actual limit in encode_multipart() seems to be 64k.

I changed the implementation. Upload of files is done by the new
code for all files.

> - In encode_multipart(), please check for error from fstat().
I used access intead.

> - Please move struct big_files_offset into src/viewer/text/form.c
>   and add a comment saying that the begin and end numbers
>   indicate the position of the file name in the POST data
>   generated by encode_multipart(), thus int will be large enough
>   regardless of how big the files are.

I added some comments. Feel free to add more.

 
> - Please fully document the format of uri.post, including:
>   - whether it contains only the body or can also contain headers;
>   - that most of the data is encoded in hexadecimal;
>   - that the data can include names of files to be posted, and
>     each such name begins and ends with BIG_FILE_CHAR;
>   - that BIG_FILE_CHAR has this meaning only in the post component,
>     i.e. when it follows POST_CHAR.

I replaced BIG_FILE_CHAR with FILE_CHAR, everywhere.
I didn't add comments here. My Emglish is too bad to write more
than a few sentences.
-- 
Witek