Re: [Bug-wget] Fwd: [GSoC] Extend concurrency support in Wget

[Yousong Zhou]

Hi,

On 28 March 2014 20:33, Jure Grabnar <address@hidden> 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