Re: [Bug-wget] [Bug-Wget] Issues with Metalink support

[Jure Grabnar]

On Sun, 8 Jun 2014 10:35:30 +0530
Darshit Shah <address@hidden> wrote:

> On Sun, May 11, 2014 at 11:28 PM, Ángel González <address@hidden>
> wrote:
> > On 07/05/14 23:46, Jure Grabnar wrote:
> >>
> >> Hello,
> >>
> >> I wrote two patches regarding this issue based on your suggestions.
> >>
> >> The 1st one is crucial for retrieve_from_file() function: it fixes
> >> 2 memory corruption bugs.
> >>
> >> The 2nd patch is more of an experiment than a real deal and is
> >> intended to get some feedback. It changes parallel download to one
> >> temporary file which is located in the selected directory.
> >>
> >> Before download starts it uses posix_fallocate() to allocate space
> >> and mkstemp() for unique name. After download is completed it uses
> >> rename() to rename temporary file to the final file name.
> >>
> >> After posix_fallocate() each thread opens file with fopen("wb").
> >
> > You could use w+b, even though you're not going to read from it.
> >
> >
> >> I opted for fopen() rather than messing around with file
> >> descriptors because I believe it's more portable. I don't know how
> >> Windows would react to file descriptors and I don't have a proper
> >> Windows system to test it out.
> >
> > It works fine.
> > On Windows, FILE* are a layer on top of fds, which are themselves a
> > layer over HANDLEs. To
> > make things more complex, gnulib provides a different abstraction
> > to wget. But it should work. The only special bit would be the need
> > to add O_BINARY, which
> > gnulib should already be doing for you.
> >
> >
> >
> >> Now, fopen("wb") means file, which was fallocate'd, is
> >> truncated to zero but after first request from the thread, which is
> >> reponsible for the last chunk, it would grow back to at least
> >> file_size
> >> - chunk_size. I'm also not sure how devastating that is.
> >
> > It's up to the filesystem, but I think it would be better to do
> > open (or dup) + fdopen()
> > + fseek rather than the fopen(, "wb"); It also allows you to
> > dispense with the barrier.
> >
> >
> >
> >> I'm attaching a handmade Metalink file which downloads a 50MB file
> >> for testing purposes. Currently all threads connect to the same
> >> server and I understand we don't support such behaviour but I
> >> guess 2-3 threads for testing purpose don't hurt anyone. :)
> >>
> Does anyone have any objections to the above patches? Else we can
> merge them.
I'm sending updated 2nd patch. It uses fopen(,"r+b") and doesn't need
barrier. I tested quite a bit and it works ok. The only problem comes
when number of thread is >=8, the program sometimes crashes. I tried
debugging it with gdb and valgrind but without avail - it doesn't crash
(Heisenbug). Through core dump I found out crash is quite random
(probably race condition?). It's either accessing free'd pointer or
freeing already free'd pointer. I don't want to spend too much time
fixing it right now, because when downloading of a single file is done,
downloading Metalink should migrate to it aswell (this bug might
be gone then). 

I believe this patch is not connected to the bug though, because
Wget crashes even if I use the original code.


Best Regards,

Jure Grabnar

0001-Download-to-single-temporary-file.patch
Description: Text Data