Hi,

On Sat, Apr 28, 2012 at 4:56 AM, Daniel Stenberg <daniel_at_haxx.se> wrote:
> On Sat, 28 Apr 2012, Tatsuhiro Tsujikawa wrote:
>
>> I finally updated my 3year-old Metalink support patch to the curl master
>> branch. The patch is attached.
>
>
> Thanks Tatsuhiro! I think the time has come for us to get proper metalink
> support added. Maybe not just yet though, see below...
>
>
>> The Metalink support is enabled when configure option --with-libmetalink
>> is used. It defaults to be disabled.
>
>
> ...
>

First of all, thank you for reviewing my patch. I feel sorry that I
submitted the patch with that very bad quality...

The improved patch is attached. For the details, see my comment below.

> I fell into quite a number of problems immediately and I'd like to share
> them with you (all):
>
>  1 - building without libmetalink fails to compile since you've hidden a
>     define (GETOUT_METALINK) the code used outside the #ifdefs. (see
> attached
>     patch)
>

Patch applied. I fixed compile error without metalink support as well.

>  2 - when (1) is fixed, the build still warns and since I build with -Werror
>     that means failure. The header logic needs to be fixed.
>

I use --enable-debug --enable-warings --enable-werror configure
options and now compiles fine. If I need more options, please tell me.

>  3 - I then proceeded and downloaded and intalled libmetalink in a custom
>     install path to try it out. configure --with-libmetalink didn't work
>     and I had to patch configure.ac first to properly use the custom path
>     I gave it. (see second attached patch)
>

Patch applied. Thanks!

>  4 - now I could run configure and it found the lib and it started to build
>     only to fail as soon as it tried to build the libmetalink using code:
>
>     metalink_types.h:50:1: error: function declaration isn’t a prototype
>       [-Werror=strict-prototypes]
>     metalink_error.h:53:8: error: C++ style comments are not allowed in ISO
>       C90 [-Werror]
>
>     So I had to edit my Makefile to make it build, but it when spewed out
>     warnings many times. I figure a fix would make curl use -isystem or
>     something instead of -I for the includes when gcc is used, to make it
>     inhibit the complaints.
>

I fixed libmetalink upstream source. Check out latest branch from bzr
repoistory:
https://code.launchpad.net/~metalink-dev/libmetalink/trunk

>  5 - when I had edited my Makefile I found more funny warnings:
>
>     tool_metalink.c:733:5: warning: implicit declaration of function
>       'easysrc_perform' [-Wimplicit-function-declaration]
>     tool_metalink.c:733:5: warning: nested extern declaration of
>       'easysrc_perform' [-Wnested-externs]
>

I migrated Metalink support in operate() and removed operatemetalink()
from tool_metalink.c.

>  6 - I don't think the --metalink option should be conditional, I would
> prefer
>     it to output a friendly message in case curl was built without metalink
>     support instead of just saying that the option is unknown.
>

I made --metalink always available regardless of Metalink support.
Also added warning message to the user if --metalink is used when
Metalink support is not enabled.

>
>> This adds --metalink option to curl.
>> You can give Metalink file to curl like this:
>>    $ curl -O --metalink foo.metalink
>
>
> Is this really how you want to use it? I mean, how you a typical user get
> that foo.metalink file in the first place? Also, what exactly does -O mean
> here? What would the code do if we don't provide a -O for metalink?
>

For normal usecase, users don't use metalink file directly. Instead
they give URI to the metalink file to curl and expect it to download
the file described in it.
The first patch simply did not incorporate this usecase. In the
attached patch, I added this functionality. If the content-type of
downloaded file is application/metalink+xml, then get URLs from it and
add them to download queue.
Currently, we need metalink file must be saved in the local file, for
example, using -O, to make this feature work.

So now you can download file using:

  $ src/curl -O 'http://curl.haxx.se/metalink.cgi?curl=tar.lzma'

I think directly reading local metalink file is useful for debugging.
The metalink file creators can check Metalink file without uploading
it to the web server.

For -O, I wondered we should use curl default behavior for Metalink
download as well. I personally think user expects curl to download
file with the filename described in Metalink file. So in the attached
patch, I did this and now -O is not needed for metalink:

  $ src/curl --metalink foo.metalink

>
>> The functionality is very limited. It only tries next resource if the
>> download is not successful (e.g., 404 Not found).
>
>
> How did you come up with that logic? Since a metalink file is supposed to
> specify existing and working URLs, wouldn't a tool then more or less be
> expected to try the next link pretty much no matter which error you get? Or
> put another way: why would getting a 5xx or a 3xx code not be subject for
> another attempt?
>

Well, the previous patch is not good enough in this area.
I agree with you that we should try next resource if the download is
not complete, not just when 40x status code. In the attached patch, I
changed the code so that the next resource is tried if the status code
is neither 200 nor 206 or the return code of curl_easy_perform is not
CURLE_OK.

>
>> I admit that the patch is in very early stage. There are many duplicated
>> code.
>
>
> Yes. To such an extent that I really cannot see myself merging the code with
> all this duplication. I think we need to come up with a way that avoid that
> first.
>
>
>> Maybe it would be better to integrate Metalink function to operate() in
>> tool_operate.c but there is the difference in control flow (for example, in
>> Metalink, we don't use URI globbing. Instead we need another loop through
>> all resources in Metalink file) so we will need many if-else cause.
>
>
> It is basically metalink instead of globbing so I don't think it would have
> to become _that_ messy.
>

Thanks for the pointer. I migrated Metalink support in operate() function.

> Ok, let me end with what happened when I eventually tried to run my new
> build:
>
> $ ./src/curl -O --metalink metalink
> ERROR: code=902
> $ echo $?
> 1
>
> The metalink file was downloaded from here:
> http://curl.haxx.se/metalink.cgi?curl=tar.lzma
>

902 code means metalink file could not be opened.
I did not encountered this error. Possible cause is that metalink file
location is wrong.
I built libmetalink for both expat and libxml2 and tested.
Both works fine.

The attached tar file contains all patches so far.
If you prefer just a single patch against the latest git branch, then
I'll do in that way.

Best regards,

Tatsuhiro Tsujikawa

> --
>
>  / daniel.haxx.se
> -------------------------------------------------------------------
> List admin: http://cool.haxx.se/list/listinfo/curl-users
> FAQ:        http://curl.haxx.se/docs/faq.html
> Etiquette:  http://curl.haxx.se/mail/etiquette.html
>

-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-users
FAQ: http://curl.haxx.se/docs/faq.html
Etiquette: http://curl.haxx.se/mail/etiquette.html