| Message ID | CAHQM780agD+6-cCQmWuHzguHYVQiKiS-xAw+R7u=a_PLvKP3Cg@mail.gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On 2016-06-17 13:19, xinglp wrote: > --post-file parameter only support text file not binary file. > "Content-Length" is more compatible to the http server world. Your patch cannot apply due to whitespace damage. Also, the library supports delivering POST data in multiple calls to send_data - your patch is breaking that completely. If you want to support using content_length instead of chunked encoding, please do so in an optional way that does not break the existing code. Could you please also provide some information why you're replacing chunked encoding with content-length? What servers do not accept this? And why are you saying it "only support text file not binary file"? - Felix
2016-06-17 23:31 GMT+08:00 Felix Fietkau <nbd@nbd.name>: > On 2016-06-17 13:19, xinglp wrote: >> --post-file parameter only support text file not binary file. >> "Content-Length" is more compatible to the http server world. > Your patch cannot apply due to whitespace damage. Also, the library > supports delivering POST data in multiple calls to send_data - your > patch is breaking that completely. Now, there is an attachment. I saw uclient_http_send_data() was only called once in uclient-2016-06-16, what did I missed ? > If you want to support using content_length instead of chunked encoding, > please do so in an optional way that does not break the existing code. That's a easy work, but where the multiple calls to send_data is needed on earth, wget does not use that at all. > Could you please also provide some information why you're replacing > chunked encoding with content-length? What servers do not accept this? Some APPs's server side are "homemade" http server, which may not support client to server chunked encoding transport. And most of the browsers use content-length to post data, I only saw chunked encoding was used in server to client transport. > And why are you saying it "only support text file not binary file"? Check this: uclient-fetch.c line 337 if (post_data) { uclient_http_set_header(cl, "Content-Type", "application/x-www-form-urlencoded"); uclient_write(cl, post_data, strlen(post_data)); } > > - Felix >
On 2016-06-18 05:09, xinglp wrote: > 2016-06-17 23:31 GMT+08:00 Felix Fietkau <nbd@nbd.name>: >> On 2016-06-17 13:19, xinglp wrote: >>> --post-file parameter only support text file not binary file. >>> "Content-Length" is more compatible to the http server world. >> Your patch cannot apply due to whitespace damage. Also, the library >> supports delivering POST data in multiple calls to send_data - your >> patch is breaking that completely. > Now, there is an attachment. I would recommend using git send-email to avoid such issues in the future. > I saw uclient_http_send_data() was only called once in > uclient-2016-06-16, what did I missed ? What you missed is the fact uclient-fetch is not the only user of the library code. The uclient library is used in a few other projects as well, some of which are not in the LEDE tree yet. Please make sure that any change you come up with to support --post-file does not break the library API. >> If you want to support using content_length instead of chunked encoding, >> please do so in an optional way that does not break the existing code. > That's a easy work, but where the multiple calls to send_data is > needed on earth, wget does not use that at all. If you upload a large file, it makes sense to not need to have it in memory completely. >> Could you please also provide some information why you're replacing >> chunked encoding with content-length? What servers do not accept this? > Some APPs's server side are "homemade" http server, which may not > support client to server chunked encoding transport. > And most of the browsers use content-length to post data, I only saw > chunked encoding was used in server to client transport. >> And why are you saying it "only support text file not binary file"? > Check this: > uclient-fetch.c line 337 > if (post_data) { > uclient_http_set_header(cl, "Content-Type", > "application/x-www-form-urlencoded"); > uclient_write(cl, post_data, strlen(post_data)); > } Ah, okay. I was confused for a moment because you wrote "--post-file" in that sentence in the original description. - Felix
2016-06-18 13:01 GMT+08:00 Felix Fietkau <nbd@nbd.name>: > If you upload a large file, it makes sense to not need to have it in > memory completely. Please review this one.
2016-06-18 18:38 GMT+08:00 xinglp <xinglp@gmail.com>: > 2016-06-18 13:01 GMT+08:00 Felix Fietkau <nbd@nbd.name>: >> If you upload a large file, it makes sense to not need to have it in >> memory completely. > Please review this one. Last patch still won't work for file larger than memory, but it supports binary file.
2016-06-18 18:40 GMT+08:00 xinglp <xinglp@gmail.com>: > 2016-06-18 18:38 GMT+08:00 xinglp <xinglp@gmail.com>: >> 2016-06-18 13:01 GMT+08:00 Felix Fietkau <nbd@nbd.name>: >>> If you upload a large file, it makes sense to not need to have it in >>> memory completely. >> Please review this one. > Last patch still won't work for file larger than memory, but it > supports binary file. last patch has a mistake, try this one.
2016-06-18 13:01 GMT+08:00 Felix Fietkau <nbd@nbd.name>: >> I saw uclient_http_send_data() was only called once in >> uclient-2016-06-16, what did I missed ? > What you missed is the fact uclient-fetch is not the only user of the > library code. The uclient library is used in a few other projects as > well, some of which are not in the LEDE tree yet. > > Please make sure that any change you come up with to support --post-file > does not break the library API. > >>> If you want to support using content_length instead of chunked encoding, >>> please do so in an optional way that does not break the existing code. >> That's a easy work, but where the multiple calls to send_data is >> needed on earth, wget does not use that at all. > If you upload a large file, it makes sense to not need to have it in > memory completely. I think this patch is better than others I posted. It will behave same as wget while not break the library API. I add an new API: int uclient_http_set_contentlength(struct uclient *cl, int len); We can call this first when we know the exact total length before send data, it wont break uclient_http_send_data().
diff --git a/uclient-fetch.c b/uclient-fetch.c index 065742e..13f2fe2 100644 --- a/uclient-fetch.c +++ b/uclient-fetch.c @@ -43,6 +43,7 @@ static const char *user_agent = "uclient-fetch"; static const char *post_data; +static char *post_file_content; static struct ustream_ssl_ctx *ssl_ctx; static const struct ustream_ssl_ops *ssl_ops; static int quiet = false; @@ -449,6 +450,7 @@ static int usage(const char *progname) " --password=<password> HTTP authentication password\n" " --user-agent|-U <str> Set HTTP user agent\n" " --post-data=STRING use the POST method; send STRING as the data\n" + " --post-file=FILE use the POST method; send contents of FILE\n" " --spider|-s Spider mode - only check file existence\n" " --timeout=N|-T N Set connect/request timeout to N seconds\n" " --proxy=on|off|-Y on|off Enable/disable env var configured proxy\n" @@ -491,6 +493,30 @@ static int no_ssl(const char *progname) return 1; } +static ssize_t load_file(const char *path, char **buf) +{ + int fd; + if ((fd = open(path, O_RDONLY)) < 0) { + if (!quiet) + fprintf(stderr, "open %s: %s\n", path, strerror(errno)); + return -1; + } + struct stat st; + if (fstat(fd, &st)) { + if (!quiet) + fprintf(stderr, "fstat %s: %s\n", path, strerror(errno)); + return -1; + } + *buf = malloc(st.st_size); + if(st.st_size != read(fd, *buf, st.st_size)) { + if (!quiet) + fprintf(stderr, "read size different from fstat %s\n", path); + return -1; + } + close(fd); + return st.st_size; +} + enum { L_NO_CHECK_CERTIFICATE, L_CA_CERTIFICATE, @@ -498,6 +524,7 @@ enum { L_PASSWORD, L_USER_AGENT, L_POST_DATA, + L_POST_FILE, L_SPIDER, L_TIMEOUT, L_CONTINUE, @@ -512,6 +539,7 @@ static const struct option longopts[] = { [L_PASSWORD] = { "password", required_argument }, [L_USER_AGENT] = { "user-agent", required_argument }, [L_POST_DATA] = { "post-data", required_argument }, + [L_POST_FILE] = { "post-file", required_argument }, [L_SPIDER] = { "spider", no_argument }, [L_TIMEOUT] = { "timeout", required_argument }, [L_CONTINUE] = { "continue", no_argument }, @@ -568,6 +596,11 @@ int main(int argc, char **argv) case L_POST_DATA: post_data = optarg; break; + case L_POST_FILE: + if(load_file(optarg, &post_file_content) < 0) + exit(1); + post_data = post_file_content; + break; case L_SPIDER: no_output = true; break; @@ -697,5 +730,8 @@ int main(int argc, char **argv) if (ssl_ctx) ssl_ops->context_free(ssl_ctx); + if (post_file_content) + free(post_file_content); + return error_ret; } diff --git a/uclient-http.c b/uclient-http.c index f0451cc..07de2d4 100644 --- a/uclient-http.c +++ b/uclient-http.c @@ -286,18 +286,6 @@ static void uclient_http_process_headers(struct uclient_http *uh) uh->auth_type = uclient_http_update_auth_type(uh); } -static bool uclient_request_supports_body(enum request_type req_type) -{ - switch (req_type) { - case REQ_POST: - case REQ_PUT: - case REQ_DELETE: - return true; - default: - return false; - } -} - static void uclient_http_add_auth_basic(struct uclient_http *uh)
--post-file parameter only support text file not binary file. "Content-Length" is more compatible to the http server world. Signed-off-by: xinglp <xinglp@gmail.com> --- { @@ -576,9 +564,6 @@ uclient_http_send_headers(struct uclient_http *uh) blobmsg_for_each_attr(cur, uh->headers.head, rem) ustream_printf(uh->us, "%s: %s\r\n", blobmsg_name(cur), (char *) blobmsg_data(cur)); - if (uclient_request_supports_body(uh->req_type)) - ustream_printf(uh->us, "Transfer-Encoding: chunked\r\n"); - uclient_http_add_auth_header(uh); ustream_printf(uh->us, "\r\n"); @@ -984,12 +969,16 @@ uclient_http_send_data(struct uclient *cl, const char *buf, unsigned int len) if (uh->state >= HTTP_STATE_REQUEST_DONE) return -1; + if (len > 0) { + char buf[32]; + sprintf(buf, "%d", len); + uclient_http_set_header(cl, "Content-Length", buf); + } + uclient_http_send_headers(uh); if (len > 0) { - ustream_printf(uh->us, "%X\r\n", len); ustream_write(uh->us, buf, len, false); - ustream_printf(uh->us, "\r\n"); } return len; @@ -1004,8 +993,6 @@ uclient_http_request_done(struct uclient *cl) return -1; uclient_http_send_headers(uh); - if (uclient_request_supports_body(uh->req_type)) - ustream_printf(uh->us, "0\r\n\r\n"); uh->state = HTTP_STATE_REQUEST_DONE; return 0;