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 _______________________________________________ elinks-dev mailing list [email protected] http://linuxfromscratch.org/mailman/listinfo/elinks-dev
