Thank you for you feedback Darshit. I changed my proposal according to your advices. Hopefully a new version is better.
I'm also sending corrected patches, again thanks to your review, Darshit. First patch allows Metalink to have optional argument "type" in <url> field. Where type is not present, it extracts protocol type from URL string. Second patch fixes few compiler warnings. Details are written in ChangeLog. Regards, Jure Grabnar On 20 March 2014 13:13, Darshit Shah <[email protected]> wrote: > I have a few comments about your proposal. > > 1. When downloading multiple files from multiple servers, there is > also the use case where you may download multiple files while each is > downloaded using multiple threads. Think about this: > > I have a 2 files I'm trying to download, both are 4 GB each. I'd > rather download them with 4 jobs each using 8 different servers than > as 2 jobs in all. It is a case which should otherwise be covered int > he implementation, but it should be there in your proposal too. > > Think about how this might be implemented, and how this *might* differ > from the other cases. > > 2. You state re-implement Metalink 3.0 code as the first point in your > timeline. So, during this period will you not be looking at Metalink > 4.0 compatibility at all? If so, when will you be looking at Metalink > 4? Just mention it somewhere. > > 3. It looks slightly off when the dates on your timeline are not > continuous. Don't put any breaks there. You can mention Weekend, if > you want, but it should be documented. It's just easier to read when > the dates flow continuously. > > 4. If you have exams, relax for that period. Take some time off. Don't > mess up both your code and your exams. I suggest you concentrate on > your exams during that time. > > 5. You give yourself 5 days to merge code and write documentation. > That's not happening. Trust me. The merging process will take a lot > more time. You're going to be writing a lot of code that edits lots of > files in mailine. This code will not be simple to merge directly. > Also, documentation takes time. > > There's also some other higher level comments about your timeline and work > flow: > > You wish to implement/fix Metalink first. But Metalink requires > concurrent downloading capability. > You may want to rethink the order in which you're implementing these > features. > > On Wed, Mar 19, 2014 at 7:40 PM, Jure Grabnar <[email protected]> wrote: > > Hi, > > > > my proposal is available on > > > http://www.google-melange.com/gsoc/proposal/public/google/gsoc2014/toomanysecrets/5629499534213120 > > I'm looking forward to your review. > > > > Regards, > > > > > > Jure Grabnar > > > > > > > > On 18 March 2014 01:06, Darshit Shah <[email protected]> wrote: > >> > >> Hi Jure, > >> > >> Thanks for your patches. However, I do have a few comments about the > same: > >> > >> 1. Trailing Whitespaces: This is essentially extra whitespaces at the > >> end of a line or on a blank line. See [1] and [2] for more > >> information. > >> 2. The indentation is mostly right, but sometimes off. > >> 3. Your first patch is missing a ChangeLog. Every commit must be > >> accompanied by a ChangeLog entry, no matter how trivial it is. > >> 4. Your 2nd patch seems to revert things from the first one. This > >> usually means some cleanup is needed. > >> > >> I'm not completely sure of some of the details of the lines you change > >> in your second patch, but they seem a little sketchy. I'll have to dig > >> into the code and check it out. > >> > >> Also, for a non-trivial (>10 LoC) patch, you'll first need to submit > >> your copyright assignment to the FSF. > >> Giuseppe will arrange for the documents as soon as your patch is ready. > >> > >> The code however, does fix a segfault and maybe a few compiler > >> warnings. When it fixes something, an explanation is usually a nice > >> idea. > >> > >> [1] > >> > http://codeimpossible.com/2012/04/02/Trailing-whitespace-is-evil-Don-t-commit-evil-into-your-repo-/ > >> [2] > >> > https://stackoverflow.com/questions/1583406/why-does-git-care-about-trailing-whitespace-in-my-files > >> > >> On Mon, Mar 17, 2014 at 6:56 PM, Jure Grabnar <[email protected]> > wrote: > >> > Hi, > >> > > >> > this patch fixes some of compiler warnings. I was uncertain for the > >> > remaining ones (5) - I believe some of them might be stubs for > upcoming > >> > features. > >> > > >> > Best Regards, > >> > > >> > > >> > Jure Grabnar (toomanysecrets) > >> > >> > >> > >> -- > >> Thanking You, > >> Darshit Shah > > > > > > > > -- > Thanking You, > Darshit Shah >
From 149cba12e3687d3197360a639b0cb1365361dc68 Mon Sep 17 00:00:00 2001 From: Jure Grabnar <[email protected]> Date: Thu, 20 Mar 2014 17:59:59 +0100 Subject: [PATCH 1/2] Fix metalink issues when type is not present. --- src/ChangeLog | 6 ++++++ src/metalink.c | 38 +++++++++++++++++++++++++++++++++----- 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index f222037..6e4729c 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,3 +1,9 @@ +2014-03-20 Jure Grabnar <[email protected]> + + * metalink.c (parse_metalink): Find out resources' protocol type from URL. + Do not crash when there're no resources. + (elect_resources): Do not crash when protocol type is NULL. + 2014-03-04 Giuseppe Scrivano <[email protected]> * http.c (modify_param_value, extract_param): Aesthetic change. diff --git a/src/metalink.c b/src/metalink.c index 76db2fa..b8cdd28 100644 --- a/src/metalink.c +++ b/src/metalink.c @@ -42,6 +42,7 @@ as that of the covered work. */ #include "sha256.h" #include "metalink.h" #include "utils.h" +#include "url.h" #define HASH_TYPES 3 @@ -134,7 +135,34 @@ parse_metalink(char *input_file) ++(file->num_of_res); resource->url = xstrdup ((*resources)->url); - resource->type = ((*resources)->type ? xstrdup ((*resources)->type) : NULL); + + if ((*resources)->type) + resource->type = xstrdup((*resources)->type); + else + { + enum url_scheme res_scheme = url_scheme (resource->url); + switch (res_scheme) + { + case SCHEME_HTTP: + case SCHEME_HTTPS: + resource->type = "http"; + break; + case SCHEME_FTP: + resource->type = "ftp"; + break; + default: + resource->type = NULL; + } + + int len_url = strlen (resource->url); + if (len_url >= 8) + { + const char *url_ending = &(resource->url[len_url-8]); + if (0 == strncasecmp (url_ending, ".torrent", 8)) + resource->type = "bittorrent"; + } + } + resource->location = ((*resources)->location ? xstrdup ((*resources)->location) : NULL); resource->preference = (*resources)->preference; resource->maxconnections = (*resources)->maxconnections; @@ -143,7 +171,7 @@ parse_metalink(char *input_file) (file->resources) = resource; } - for (checksums = (*files)->checksums; *checksums; ++checksums) + for (checksums = (*files)->checksums; checksums && *checksums; ++checksums) { mlink_checksum *checksum = malloc (sizeof(mlink_checksum)); @@ -170,7 +198,7 @@ parse_metalink(char *input_file) if((chunk_checksum = (*files)->chunk_checksum)) { mlink_chunk_checksum *chunk_sum; - + if(!chunk_checksum->type) logprintf (LOG_VERBOSE, "PARSE METALINK: Skipping chunk checksum" " due to missing type information.\n"); @@ -215,7 +243,7 @@ elect_resources (mlink *mlink) while (res_next = res->next) { - if (strcmp(res_next->type, "ftp") && strcmp(res_next->type, "http")) + if (res_next->type == NULL || (strcmp(res_next->type, "ftp") && strcmp(res_next->type, "http"))) { res->next = res_next->next; free(res_next); @@ -224,7 +252,7 @@ elect_resources (mlink *mlink) res = res_next; } res = file->resources; - if (strcmp(res->type, "ftp") && strcmp(res->type, "http")) + if (res->type == NULL || (strcmp(res->type, "ftp") && strcmp(res->type, "http"))) { file->resources = res->next; free(res); -- 1.9.0
From 74c6f535a93da4ccb20aa1e902eb1e2b8df903a9 Mon Sep 17 00:00:00 2001 From: Jure Grabnar <[email protected]> Date: Thu, 20 Mar 2014 17:16:31 +0100 Subject: [PATCH 2/2] Fix some compiler warnings --- src/ChangeLog | 19 +++++++++++++++++++ src/http.c | 5 +---- src/iri.h | 2 +- src/metalink.c | 13 ++++++------- src/multi.c | 8 ++++++++ src/multi.h | 2 -- src/progress.c | 1 - src/recur.c | 2 ++ src/retr.c | 4 ++-- 9 files changed, 39 insertions(+), 17 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index 6e4729c..c7068f0 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,5 +1,24 @@ 2014-03-20 Jure Grabnar <[email protected]> + * http.c (gethttp): Remove unused variable 'tmp2'. + * iri.h: Fix macro parse_charset(str) to not cause 'left-hand operand of + comma expression has no effect'. + * metalink.c (parse_metalink): Remove unused variable 'piece_hash'. + (elect_resources): Add parentheses to assigment. + (elect_checksums): Same. Add cast to (unsigned char *). + (veriy_file_hash): Add casts to (char *). + * multi.h: Remove prototype of static function segmented_retrieve_url(). + * multi.c: Include "retr.h". Add prototype of static function + segmented_retrieve_url(); + (collect_thread): Add return value to suppress compiler warning. + (segmented_retrieve_url): Same. + * recur.c (start_retrieve_url): Same. + * progress.c: Removed unused variable 'header'. + * retr.c (retrieve_from_file): Remove unused variables 'i', 'url_err', + 'retries'. + +2014-03-20 Jure Grabnar <[email protected]> + * metalink.c (parse_metalink): Find out resources' protocol type from URL. Do not crash when there're no resources. (elect_resources): Do not crash when protocol type is NULL. diff --git a/src/http.c b/src/http.c index c5c4d8a..fa627a7 100644 --- a/src/http.c +++ b/src/http.c @@ -2872,9 +2872,6 @@ read_header: char *tmp = strchr (type, ';'); if (tmp) { - /* sXXXav: only needed if IRI support is enabled */ - char *tmp2 = tmp + 1; - while (tmp > type && c_isspace (tmp[-1])) --tmp; *tmp = '\0'; @@ -2882,7 +2879,7 @@ read_header: /* Try to get remote encoding if needed */ if (opt.enable_iri && !opt.encoding_remote) { - tmp = parse_charset (tmp2); + tmp = parse_charset (tmp + 1); if (tmp) set_content_encoding (iri, tmp); } diff --git a/src/iri.h b/src/iri.h index e759e45..c88d94f 100644 --- a/src/iri.h +++ b/src/iri.h @@ -56,7 +56,7 @@ void set_content_encoding (struct iri *i, char *charset); extern struct iri dummy_iri; -#define parse_charset(str) (str, NULL) +#define parse_charset(str) (NULL) #define find_locale() NULL #define check_encoding_name(str) false #define locale_to_utf8(str) (str) diff --git a/src/metalink.c b/src/metalink.c index b8cdd28..539a2d0 100644 --- a/src/metalink.c +++ b/src/metalink.c @@ -137,7 +137,7 @@ parse_metalink(char *input_file) resource->url = xstrdup ((*resources)->url); if ((*resources)->type) - resource->type = xstrdup((*resources)->type); + resource->type = xstrdup ((*resources)->type); else { enum url_scheme res_scheme = url_scheme (resource->url); @@ -209,7 +209,6 @@ parse_metalink(char *input_file) chunk_sum->type = (chunk_checksum->type ? xstrdup (chunk_checksum->type) : NULL); for (piece_hashes = chunk_checksum->piece_hashes; *piece_hashes; ++piece_hashes) { - mlink_piece_hash piece_hash; if(!chunk_checksum->type) { logprintf (LOG_VERBOSE, "PARSE METALINK: Skipping chunk checksum" @@ -241,7 +240,7 @@ elect_resources (mlink *mlink) if (!res) continue; - while (res_next = res->next) + while ((res_next = res->next)) { if (res_next->type == NULL || (strcmp(res_next->type, "ftp") && strcmp(res_next->type, "http"))) { @@ -275,7 +274,7 @@ elect_checksums (mlink *mlink) if (!csum) continue; - while (csum_next = csum->next) + while ((csum_next = csum->next)) { /* Traverse supported hash types & break if csum->type is the same. */ for (i = 0; i < HASH_TYPES; ++i) @@ -447,7 +446,7 @@ verify_file_hash (const char *filename, mlink_checksum *checksums) return 1; } else - metalink_hashes[j] = checksum->hash; + metalink_hashes[j] = (unsigned char *) checksum->hash; } for (i = 0; !metalink_hashes[i]; ++i); @@ -482,10 +481,10 @@ verify_file_hash (const char *filename, mlink_checksum *checksums) /* Turn byte-form hash to hex form. */ for(j = 0 ; j < digest_sizes[req_type]; ++j) - sprintf(file_hash + 2 * j, "%02x", hash_raw[j]); + sprintf((char *) file_hash + 2 * j, "%02x", hash_raw[j]); lower_hex_case(metalink_hashes[req_type], 2 * digest_sizes[req_type]); - if (strcmp(metalink_hashes[req_type], file_hash)) + if (strcmp((char *) metalink_hashes[req_type], (char *) file_hash)) { logprintf (LOG_VERBOSE, "Verifying(%s) failed: %s hashes are different.\n", filename, supported_hashes[i]); diff --git a/src/multi.c b/src/multi.c index 4b22b2e..211c449 100644 --- a/src/multi.c +++ b/src/multi.c @@ -38,11 +38,14 @@ as that of the covered work. */ #include <unistd.h> #include "multi.h" +#include "retr.h" #include "url.h" static struct range *ranges; char **files; +static void *segmented_retrieve_url (void *); + /* Allocate space for temporary file names. */ void init_temp_files() @@ -220,6 +223,9 @@ collect_thread (sem_t *retr_sem, struct s_thread_ctx *thread_ctx) (thread_ctx[k].range)->is_assigned = 0; return k; } + + /* Should not happen. */ + return -1; } /* The function which is being called by pthread_create in spawn_thread(). It @@ -235,4 +241,6 @@ segmented_retrieve_url (void *arg) false, ctx->i, true, ctx->range); ctx->terminated = 1; sem_post (ctx->retr_sem); + + return NULL; } diff --git a/src/multi.h b/src/multi.h index ad9bd21..ec53e85 100644 --- a/src/multi.h +++ b/src/multi.h @@ -84,6 +84,4 @@ int spawn_thread (struct s_thread_ctx*, int, int); int collect_thread (sem_t *, struct s_thread_ctx *); -static void * segmented_retrieve_url (void *); - #endif /* MULTI_H */ diff --git a/src/progress.c b/src/progress.c index 44582d8..5ec6b91 100644 --- a/src/progress.c +++ b/src/progress.c @@ -302,7 +302,6 @@ progress_update (void *progress, wgint howmuch, double dltime) void progress_finish (void *progress, double dltime) { - struct progress_header *header = progress; { struct progress_header *it, *prev = NULL; diff --git a/src/recur.c b/src/recur.c index 38f4d00..c0c9628 100644 --- a/src/recur.c +++ b/src/recur.c @@ -190,6 +190,8 @@ start_retrieve_url (void *arg) false, ctx->i, true, NULL); ctx->terminated = 1; sem_post (ctx->retr_sem); + + return NULL; } #endif diff --git a/src/retr.c b/src/retr.c index 8c361de..9ff46a7 100644 --- a/src/retr.c +++ b/src/retr.c @@ -1075,8 +1075,7 @@ retrieve_from_file (const char *file, bool html, int *count) if(opt.metalink_file && mlink) { - int i, j, r, ranges_covered, chunk_size, url_err, retries, ret, dt=0; - pthread_t thread; + int j, r, ranges_covered, chunk_size, retries, ret, dt=0; sem_t retr_sem; uerr_t status; mlink_file* file; @@ -1487,6 +1486,7 @@ rotate_backups(const char *fname) } static bool no_proxy_match (const char *, const char **); +static bool no_proxy_match (const char *, const char **); /* Return the URL of the proxy appropriate for url U. */ -- 1.9.0
