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

[Jure Grabnar]

Hello,

thanks for your feedback! I corrected the first patch.

On 31 March 2014 04:01, Yousong Zhou <address@hidden> wrote:

> 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.


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

0001-Fix-metalink-issues-when-type-is-not-present.patch
Description: Text Data

0002-Fix-some-compiler-warnings.patch
Description: Text Data