[pacman-dev] [arch-dev-public] [signoff] pacman 3.2.0

[Xavier]

On Mon, Aug 4, 2008 at 7:34 PM, Henning Garus
<henning.garus at googlemail.com> wrote:
>
> If my understanding of the libdownload code is correct "Command okay"
> occurs, because the server sends a 200 when libdownload expects
> something different. But I can't say where.
>
> I've been spending some time trying to reproduce this error, but
> syncing to a local ftp just works, with and without trailing slash.
>
>> I am a bit confused though now, it looks like the code
>> already handles multiple slashes. But well, I will need to
>> investigate it more.
>
> C is not my strong point, but I think the following lines in _ftp_cwd()
> should take care of multiple slashes:
>
>        while (*beg == '/')
>                        ++beg, ++i;

Note that you need to download at least 2 files in a row for this to
happen. Doing pacman -Sy <package> is enough because a first
connection is made for the -Sy, then a second for downloading the
package and it breaks there.

If you prefer, I have a simpler test case. I just took libdownload
source code (for helping debugging, I set debug=true in the Makefile
and then installed that), then I extended the samples/dl.c program to
reproduce the issue. I attach it here.
Then you just need to run it on one ftp server with two files :
./dl ftp://ftp.archlinux.org/core/os/i686/lastsync
ftp://ftp.archlinux.org/core/os/i686//packages.txt
But it is probably better to run your own ftp server for debugging /
playing purpose.
./dl ftp://localhost/foo ftp://localhost//bar

The relevant part of the code is indeed the one you showed, but you
have to look around it too:
        for (beg = file + i; beg < end; beg = file + i + 1) {
                while (*beg == '/')
                        ++beg, ++i;
                for (++i; file + i < end && file[i] != '/'; ++i)
                        /* nothing */;
                if(beg < end)
                        e = _ftp_cmd(conn, "CWD %.*s", file + i - beg, beg);
                if (e != FTP_FILE_ACTION_OK) {
                        _ftp_seterr(e);
                        return (-1);
                }
        }

In non trailing case, we have :
file : /core/os/i686/packages.txt
pwd : /core/os/i686

"end" points to the last / in file
"beg" points to the first character which differs between file and
pwd, which turns out to be exactly the same / as "end".

So here we don't have beg < end (we have beg == end) so the whole code
above is skipped and everything works fine.

In trailing case :
file : /core/os/i686//packages.txt
pwd : /core/os/i686

end points to the last / in file, and beg to the first different char,
which is the slash just before.
So here we have beg < end (beg == end - 1 ), so the above code is run,
the beg pointer skips the successive /, so eventually points to the
"p" of packages
So then we don't have beg < end anymore (beg == end + 1), so the
ftp_cmd command is not run.
But the final check is still run, and here is where our error appears :
                if (e != FTP_FILE_ACTION_OK) {
                        _ftp_seterr(e);
                        return (-1);
                }

So as you said, at this point e was set to 200 (FTP_OK), but from a
previous part of the code.
It looks very weird to check for the e error here, while on the
previous line, e was only set conditionally (only if beg < end).

And about finding where e was last set (to 200), I am not sure, maybe
at the beggining of _ftp_cwd :
        if ((e = _ftp_cmd(conn, "PWD")) != FTP_WORKING_DIRECTORY ||

Anyway, this was a very long explanation for a small thing in the end
:P Maybe it is easier to just run libdownload in debug mode, and play
with dl.c and gdb to get sense of it.
I still didn't figure out the whole thing yet, and I have still no
idea if there is something to fix and how.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dl.c
Type: text/x-csrc
Size: 2539 bytes
Desc: not available
URL: <http://archlinux.org/pipermail/pacman-dev/attachments/20080805/3f46ef67/attachment.c>