Hello, thanks for your feedback! I corrected the first patch.
On 31 March 2014 04:01, Yousong Zhou <[email protected]> wrote: > Hi, > > On 28 March 2014 20:33, Jure Grabnar <[email protected]> wrote: > > Hi, > > > > Thank you Yousong. I've listened to your advice and changed type of > > resource->type to > > enum url_scheme. Now it looks much cleaner. > > Using enum is a step forward. > > > @@ -134,7 +135,20 @@ 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) > > + { > > + /* Append "://" to resource type so url_scheme() > recognizes type */ > > + char *temp_url = malloc ( strlen ( (*resources)->type) + > 4); > > + sprintf (temp_url, "%s://", (*resources)->type); > > + > > + resource->type = url_scheme (temp_url); > > + > > + free (temp_url); > > + } > This is a little hacky. Adding a utility function like > url_scheme_str_to_enum() will be better. I added url_scheme_str_to_enum() function. > > + else > > + resource->type = url_scheme (resource->url); > > + > > resource->location = ((*resources)->location ? xstrdup > ((*resources)->location) : NULL); > > resource->preference = (*resources)->preference; > > resource->maxconnections = (*resources)->maxconnections; > > @@ -143,7 +157,7 @@ parse_metalink(char *input_file) > > (file->resources) = resource; > > } > > > > - for (checksums = (*files)->checksums; *checksums; ++checksums) > > + for (checksums = (*files)->checksums; checksums && *checksums; > ++checksums) > > Good catch. Should do the same NULL check for (*files)->resources. > > > { > > mlink_checksum *checksum = malloc (sizeof(mlink_checksum)); > > > > > > <...> > > > @@ -215,19 +229,25 @@ elect_resources (mlink *mlink) > > > > while (res_next = res->next) > > { > > - if (strcmp(res_next->type, "ftp") && strcmp(res_next->type, > "http")) > > + if (schemes_are_similar_p (res_next->type, SCHEME_INVALID)) > > { > > res->next = res_next->next; > > free(res_next); > > + > > + --(file->num_of_res); > > } > > else > > res = res_next; > > } > > res = file->resources; > > - if (strcmp(res->type, "ftp") && strcmp(res->type, "http")) > > + if (schemes_are_similar_p (res->type, SCHEME_INVALID)) > > { > > file->resources = res->next; > > If I am right, this will set it to NULL if file->num_of_res is 1. > You're right. I took a second look and it's ok. I was afraid res->next was a dangling pointer. > > - free(res); > > + free (res); > > + > > + --(file->num_of_res); > > + if (!file->num_of_res) > > + file->resources = NULL; > > So explicitly setting it to NULL is not needed. > > > } > > } > > } > > > > > > > I also added check for whenever there's no resources available to > download a > > file. > > > > Second patch remains unchanged. > > > > Regards, > > > > > > Jure Grabnar > Regards, Jure Grabnar
From 67297ae805d42af60fbcce4f804353259e3d8015 Mon Sep 17 00:00:00 2001 From: Jure Grabnar <[email protected]> Date: Mon, 31 Mar 2014 21:37:54 +0200 Subject: [PATCH 1/2] Fix metalink issues when type is not present --- src/ChangeLog | 14 ++++++++++++++ src/metalink.c | 23 ++++++++++++++++------- src/metalink.h | 4 +++- src/retr.c | 10 ++++++++++ src/url.c | 22 ++++++++++++++++++++++ src/url.h | 1 + 6 files changed, 66 insertions(+), 8 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index 537a707..74d061e 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,3 +1,17 @@ +2014-03-31 Jure Grabnar <[email protected]> + + * metalink.h: Change type of 'mlink_resource.type' to enum url_scheme. + Include url.h. + * metalink.c (parse_metalink): Find out resources' protocol type from URL if + not present in <url> tag in metalink file. + Include url.h. + (elect_resources): Do not crash when protocol type is NULL. + Count number of resources still available, if zero, assign NULL. + * url.c: + (url_scheme_str_to_enum): New function: extracts protocol type from string + without "://" suffix. + * retr.c: Add check for number of resources available. Do not crash when zero. + 2014-03-26 Darshit Shah <[email protected]> * ftp.c (getftp): Rearrange parameters to fix compiler warning diff --git a/src/metalink.c b/src/metalink.c index 76db2fa..4ff565d 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 @@ -119,7 +120,7 @@ parse_metalink(char *input_file) file->chunk_checksum = NULL; file->num_of_res = file->num_of_checksums = 0; - for (resources = (*files)->resources; *resources; ++resources) + for (resources = (*files)->resources; resources && *resources; ++resources) { mlink_resource *resource; @@ -134,7 +135,12 @@ 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 = url_scheme_str_to_enum ((*resources)->type); + else + resource->type = url_scheme (resource->url); + resource->location = ((*resources)->location ? xstrdup ((*resources)->location) : NULL); resource->preference = (*resources)->preference; resource->maxconnections = (*resources)->maxconnections; @@ -143,7 +149,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)); @@ -215,19 +221,23 @@ elect_resources (mlink *mlink) while (res_next = res->next) { - if (strcmp(res_next->type, "ftp") && strcmp(res_next->type, "http")) + if (schemes_are_similar_p (res_next->type, SCHEME_INVALID)) { res->next = res_next->next; free(res_next); + + --(file->num_of_res); } else res = res_next; } res = file->resources; - if (strcmp(res->type, "ftp") && strcmp(res->type, "http")) + if (schemes_are_similar_p (res->type, SCHEME_INVALID)) { file->resources = res->next; - free(res); + free (res); + + --(file->num_of_res); } } } @@ -301,7 +311,6 @@ delete_mlink(mlink *metalink) while (res) { xfree_null (res->url); - xfree_null (res->type); xfree_null (res->location); res_temp = res; diff --git a/src/metalink.h b/src/metalink.h index 3906e22..264320c 100644 --- a/src/metalink.h +++ b/src/metalink.h @@ -32,6 +32,8 @@ as that of the covered work. */ #ifndef MLINK_H #define MLINK_H +#include "url.h" + typedef struct metalink_piece_hash { struct metalink_piece_hash *next; @@ -53,7 +55,7 @@ typedef struct metalink_resource struct metalink_resource *next; char *url; - char *type; + enum url_scheme type; char *location; int preference; int maxconnections; diff --git a/src/retr.c b/src/retr.c index 8c361de..d89fc83 100644 --- a/src/retr.c +++ b/src/retr.c @@ -1095,6 +1095,16 @@ retrieve_from_file (const char *file, bool html, int *count) file = mlink->files; while (file) { + if (!file->num_of_res) + { + logprintf (LOG_VERBOSE, _("Downloading %s failed. File could " + "not be downloaded from any of the " + "URLs listed in metalink file.\n"), + file->name); + file = file->next; + continue; + } + memset (thread_ctx, '\0', opt.jobs * (sizeof *thread_ctx)); /* If chunk_size is too small, set it equal to MIN_CHUNK_SIZE. */ diff --git a/src/url.c b/src/url.c index a237fcf..95babfd 100644 --- a/src/url.c +++ b/src/url.c @@ -440,6 +440,28 @@ url_scheme (const char *url) return SCHEME_INVALID; } +/* Returns the scheme type from string + if the scheme is supported, or + SCHEME_INVALID if not. */ + +enum url_scheme +url_scheme_str_to_enum (const char *str) +{ + int i; + + for (i = 0; supported_schemes[i].name; i++) + if (0 == strncasecmp (str, supported_schemes[i].name, + strlen (supported_schemes[i].name))) + { + if (!(supported_schemes[i].flags & scm_disabled)) + return (enum url_scheme) i; + else + return SCHEME_INVALID; + } + + return SCHEME_INVALID; +} + #define SCHEME_CHAR(ch) (c_isalnum (ch) || (ch) == '-' || (ch) == '+') /* Return 1 if the URL begins with any "scheme", 0 otherwise. As diff --git a/src/url.h b/src/url.h index 6d18ed8..cb26ef4 100644 --- a/src/url.h +++ b/src/url.h @@ -111,6 +111,7 @@ void url_set_file (struct url *, const char *); void url_free (struct url *); enum url_scheme url_scheme (const char *); +enum url_scheme url_scheme_str_to_enum (const char *str); bool url_has_scheme (const char *); bool url_valid_scheme (const char *); int scheme_default_port (enum url_scheme); -- 1.9.0
From ec06f14020f2e79f91abb1d686791db5301fe4af Mon Sep 17 00:00:00 2001 From: Jure Grabnar <[email protected]> Date: Mon, 31 Mar 2014 21:39:28 +0200 Subject: [PATCH 2/2] Fix some compiler warnings --- src/ChangeLog | 18 ++++++++++++++++++ src/iri.h | 2 +- src/metalink.c | 11 +++++------ src/multi.c | 8 ++++++++ src/multi.h | 2 -- src/progress.c | 1 - src/recur.c | 2 ++ src/retr.c | 3 +-- 8 files changed, 35 insertions(+), 12 deletions(-) diff --git a/src/ChangeLog b/src/ChangeLog index 74d061e..8651e35 100644 --- a/src/ChangeLog +++ b/src/ChangeLog @@ -1,5 +1,23 @@ 2014-03-31 Jure Grabnar <[email protected]> + * 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-31 Jure Grabnar <[email protected]> + * metalink.h: Change type of 'mlink_resource.type' to enum url_scheme. Include url.h. * metalink.c (parse_metalink): Find out resources' protocol type from URL if 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 4ff565d..ec6223c 100644 --- a/src/metalink.c +++ b/src/metalink.c @@ -187,7 +187,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" @@ -219,7 +218,7 @@ elect_resources (mlink *mlink) if (!res) continue; - while (res_next = res->next) + while ((res_next = res->next)) { if (schemes_are_similar_p (res_next->type, SCHEME_INVALID)) { @@ -257,7 +256,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) @@ -428,7 +427,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); @@ -463,10 +462,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 d89fc83..f5cf2dc 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; -- 1.9.0
