Hi, On 28 March 2014 20:33, Jure Grabnar <grabna...@gmail.com> 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. > + 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. > - 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