<div dir="ltr">Thanks for your comment, Felix.<div><br></div><div>I am sorry my response is so late.</div><div>As your comments, I modified some code.</div><div><br></div><div><div> wget: clean up</div><div><br></div><div> - shorten error messages</div><div> - replace mk_rq with sprintf</div><div> - remove struct and defines</div><div> - change unsigned int to unsigned</div></div><div><br></div><div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Feb 19, 2016 at 8:44 PM, Felix Janda <span dir="ltr"><<a href="mailto:felix.janda@posteo.de" target="_blank">felix.janda@posteo.de</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div>Lipi C. H. Lee wrote:<br>
> implement simple 'wget' and port name can be specified in URL if default<br>
> port 80 is not used.<br>
> It may be added to toys/pending directory.<br>
<br>
</div></div>Thanks for your submission!<br>
<br>
Some comments below.<br>
<br>
In general, try simplifying the error messages:<br>
<br>
<a href="http://lists.landley.net/pipermail/toybox-landley.net/2013-April/000850.html" rel="noreferrer" target="_blank">http://lists.landley.net/pipermail/toybox-landley.net/2013-April/000850.html</a><br>
<br></blockquote><div><br></div><div>I changed them shortly.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Also, generally functions are only added when they can be called at<br>
least twice. See<br>
<br>
<a href="http://lists.landley.net/pipermail/toybox-landley.net/2013-April/000919.html" rel="noreferrer" target="_blank">http://lists.landley.net/pipermail/toybox-landley.net/2013-April/000919.html</a><br>
<br></blockquote><div><br></div><div>I will check it later.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
(URLs taken from <a href="http://landley.net/toybox/cleanup.html" rel="noreferrer" target="_blank">http://landley.net/toybox/cleanup.html</a>)<br>
<br>
> /* wget.c - Simple downloader to get the resource file in HTTP server<br>
> *<br>
> * Copyright 2016 Lipi C.H. Lee <<a href="mailto:lipisoft@gmail.com" target="_blank">lipisoft@gmail.com</a>><br>
> *<br>
><br>
> USE_WGET(NEWTOY(wget, "f:", TOYFLAG_USR|TOYFLAG_BIN))<br>
><br>
> config WGET<br>
> bool "wget"<br>
> default n<br>
> help<br>
> usage: wget -f filename URL<br>
> -f filename: specify the filename to be saved<br>
> URL: HTTP uniform resource location and only HTTP, not HTTPS<br>
><br>
> examples:<br>
> wget -f index.html <a href="http://www.example.com" rel="noreferrer" target="_blank">http://www.example.com</a><br>
> wget -f sample.jpg <a href="http://www.example.com:8080/sample.jpg" rel="noreferrer" target="_blank">http://www.example.com:8080/sample.jpg</a><br>
> */<br>
><br>
> #define FOR_wget<br>
> #include "toys.h"<br>
><br>
> GLOBALS(<br>
> char *filename;<br>
> )<br>
><br>
> #define HN_LEN 128 // HOSTNAME MAX LENGTH<br>
> #define PATH_LEN 256 // PATH MAX LENGTH<br>
><br>
> struct httpinfo {<br>
> char hostname[HN_LEN];<br>
> char port[6]; // MAX port value: 65535<br>
> char path[PATH_LEN];<br>
> };<br>
<br>
In the code base, global defines are usually avoided. If the functions<br>
were inlined, this struct would actually be unecessary and HN_LEN<br>
could be sizeof hostname.<br>
<br></blockquote><div><br></div><div>I removed defines and struct. </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
><br>
> // extract hostname from url<br>
> static unsigned int get_hn(char *url, char *hn) {<br>
> unsigned int i;<br>
><br>
> for (i = 0; url[i] != '\0' && url[i] != ':' && url[i] != '/'; i++) {<br>
> if(i >= HN_LEN)<br>
> error_exit("The hostname's length is lower than %d.", HN_LEN);<br>
<br>
You mean "larger"<br></blockquote><div>I shortened it. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
> hn[i] = url[i];<br>
> }<br>
> hn[i] = '\0';<br>
><br>
> return i;<br>
> }<br>
><br>
> // extract port number<br>
> static void get_port(char *url, char *port, unsigned int *url_i) {<br>
> unsigned int i;<br>
><br>
> for (i = 0; url[i] != '\0' && url[i] != '/'; i++, (*url_i)++) {<br>
> if('0' <= url[i] && url[i] <= '9') port[i] = url[i];<br>
> else error_exit("Port is invalid");<br>
> }<br>
> if(i <= 6) port[i] = '\0';<br>
> else error_exit("Port number is too long");<br>
> }<br>
><br>
> // get http infos in URL<br>
> static void get_info(struct httpinfo *hi, char *url) {<br>
> unsigned int i = 7, len;<br>
><br>
> if (strncmp(url, "http://", i)) error_exit("Only HTTP can be supported.");<br>
> len = get_hn(url+i, hi->hostname);<br>
> i += len;<br>
><br>
> // get port if exists<br>
> if (url[i] == ':') {<br>
> i++;<br>
> get_port(url+i, hi->port, &i);<br>
> } else strcpy(hi->port, "80");<br>
><br>
> // get uri in URL<br>
> if (url[i] == '\0') strcpy(hi->path, "/");<br>
> else if (url[i] == '/') {<br>
> if (strlen(url+i) < PATH_LEN) strcpy(hi->path, url+i);<br>
> else error_exit("The URL path's length is less than %d.", PATH_LEN);<br>
> } else error_exit("The URL is NOT valid.");<br>
> }<br>
><br>
> // connect to any IPv4 or IPv6 server<br>
> static int conn_svr(const char *hostname, const char *port) {<br>
> struct addrinfo hints, *result, *rp;<br>
> int sock;<br>
><br>
> memset(&hints, 0, sizeof(struct addrinfo));<br>
> hints.ai_family = AF_UNSPEC;<br>
> hints.ai_socktype = SOCK_STREAM;<br>
> hints.ai_flags = 0;<br>
> hints.ai_protocol = 0;<br>
><br>
> if ((errno = getaddrinfo(hostname, port, &hints, &result)))<br>
> error_exit("getaddrinfo: %s", gai_strerror(errno));<br>
><br>
> // try all address list(IPv4 or IPv6) until success<br>
> for (rp = result; rp; rp = rp->ai_next) {<br>
> if ((sock = socket(rp->ai_family, rp->ai_socktype, rp->ai_protocol))<br>
> == -1) {<br>
> perror_msg("Socket Error");<br>
> continue;<br>
> }<br>
> if (connect(sock, rp->ai_addr, rp->ai_addrlen) != -1)<br>
> break; // succeed in connecting to any server IP<br>
> else perror_msg("Connect Error");<br>
> close(sock);<br>
> }<br>
> freeaddrinfo(result);<br>
> if(!rp) error_exit("Can not connect to HTTP server");<br>
><br>
> return sock;<br>
> }<br>
><br>
> // make HTTP request header field<br>
> static void mk_fld(char *name, char *value) {<br>
> strcat(toybuf, name);<br>
> strcat(toybuf, ": ");<br>
> strcat(toybuf, value);<br>
> strcat(toybuf, "\r\n");<br>
> }<br>
><br>
> // make http request<br>
> static void mk_rq(char *path) {<br>
> strcpy(toybuf, "GET ");<br>
> strcat(toybuf, path);<br>
> strcat(toybuf, " HTTP/1.1\r\n");<br>
> }<br>
<br>
Why not sprintf() for these?<br>
<br></blockquote><div>I replaced mk_rq to sprintf.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
><br>
> // get http response body starting address and its length<br>
> static char *get_body(size_t len, size_t *body_len) {<br>
> unsigned int i;<br>
><br>
> for (i = 0; i < len-4; i++)<br>
> if (!strncmp(toybuf+i, "\r\n\r\n", 4)) break;<br>
><br>
> *body_len = len-i-4;<br>
> return toybuf+i+4;<br>
> }<br>
<br>
Why not strstr()?<br></blockquote><div><br></div><div>I need not only http body's address but also its length in toybuf.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
><br>
> void wget_main(void)<br>
> {<br>
> int sock;<br>
> struct httpinfo hi;<br>
> FILE *fp;<br>
> size_t len, body_len;<br>
> char *body, *result, *rc, *r_str, ua[18] = "toybox wget/", ver[6];<br>
><br>
> // TODO extract filename to be saved from URL<br>
> if (!(toys.optflags & FLAG_f))<br>
> help_exit("The filename to be saved should be needed.");<br>
> if (fopen(TT.filename, "r"))<br>
> error_exit("The file(%s) you specified already exists.", TT.filename);<br>
<br>
fopen can fail for other reasons. Why not use perror_exit?<br></blockquote><div><br></div><div>I did. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
><br>
> if(!toys.optargs[0]) help_exit("The URL should be specified.");<br>
> get_info(&hi, toys.optargs[0]);<br>
><br>
> sock = conn_svr(hi.hostname, hi.port);<br>
><br>
> // compose HTTP request<br>
> mk_rq(hi.path);<br>
> mk_fld("Host", hi.hostname);<br>
> strncpy(ver, TOYBOX_VERSION, 5);<br>
> strcat(ua, ver);<br>
> mk_fld("User-Agent", ua);<br>
> mk_fld("Connection", "close");<br>
> strcat(toybuf, "\r\n");<br>
><br>
> // send the HTTP request<br>
> len = strlen(toybuf);<br>
> if (write(sock, toybuf, len) != len) perror_exit("HTTP GET failed.");<br>
<br>
This gives something like: "HTTP GET failed.: Broken pipe"<br>
<br></blockquote><div>I shortened it. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
><br>
> // read HTTP response<br>
> if ((len = read(sock, toybuf, 4096)) == -1)<br>
> perror_exit("HTTP response failed.");<br>
> if (!strstr(toybuf, "\r\n\r\n"))<br>
> error_exit("HTTP response header is too long.");<br>
> body = get_body(len, &body_len);<br>
> result = strtok(toybuf, "\r");<br>
> strtok(result, " ");<br>
> rc = strtok(NULL, " ");<br>
> r_str = strtok(NULL, " ");<br>
><br>
> // HTTP res code check<br>
> // TODO handle HTTP 302 Found(Redirection)<br>
> if (strcmp(rc, "200")) error_exit("HTTP response: %s(%s).", rc, r_str);<br>
><br>
> if (!(fp = fopen(TT.filename, "w"))) perror_exit("File write error");<br>
> if (fwrite(body, sizeof(char), body_len, fp) != body_len)<br>
<br>
sizeof(char)=1<br></blockquote><div><br></div><div>I did it. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
> error_exit("File write is not successful.");<br>
> while ((len = read(sock, toybuf, 4096)) > 0)<br>
<br>
len is unsigned. So the case when read returns -1 is not correctly handled.<br></blockquote><div><br></div><div>I changed it to ssize_t type. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
> if (fwrite(toybuf, sizeof(char), len, fp) != len)<br>
> error_exit("File write is not successful.");<br>
> if (fclose(fp) == EOF) perror_exit("File Close Error");<br>
> }<br>
<span><font color="#888888"><br>
<br>
--Felix<br>
</font></span></blockquote></div><br></div></div></div>