On Wed, 8 Oct 2003, Hrvoje Niksic wrote:
> Mauro Tortonesi <[EMAIL PROTECTED]> writes:
>
> > so, i am asking you: what do you think of these changes?
>
> Overall they look very good! Judging from the patch, a large piece of
> the work part seems to be in an unexpected place: the FTP code.
yes, i have added support for LPRT and LPSV, and refactored existing code.
i still have to work on the code, but the main problem remains probably
the duplication of ftp_port and ftp_pasv, which have two different
versions (one for the IPv6-enabled case and the other for IPv4-only
case).
> Here are some remarks I got looking at the patch.
>
> It inadvertently undoes the latest fnmatch move.
sorry. i am working on an old wget cvs release. i will get up-to-date with
the latest cvs changes ASAP.
> I still don't understand the choice to use sockaddr and
> sockaddr_storage in a application code.
> They result in needless casts and (to me) uncomprehensible code.
well, using sockaddr_storage is the right way (TM) to write IPv6 enabled
code ;-)
quoting RFC3493 section 3.10:
One simple addition to the sockets API that can help application
writers is the "struct sockaddr_storage". This data structure can
simplify writing code that is portable across multiple address
families and platforms. This data structure is designed with the
following goals.
- Large enough to accommodate all supported protocol-specific address
structures.
- Aligned at an appropriate boundary so that pointers to it can be
cast as pointers to protocol specific address structures and used
to access the fields of those structures without alignment
problems.
The sockaddr_storage structure contains field ss_family which is of
type sa_family_t. When a sockaddr_storage structure is cast to a
sockaddr structure, the ss_family field of the sockaddr_storage
structure maps onto the sa_family field of the sockaddr structure.
When a sockaddr_storage structure is cast as a protocol specific
address structure, the ss_family field maps onto a field of that
structure that is of type sa_family_t and that identifies the
protocol's address family.
using a union like:
struct wget_sockaddr {
struct sockaddr;
struct sockaddr_in;
struct sockaddr_in6;
};
is not an elegant solution, and is probably not safe because of compiler
alignments. see the chapter about struct sockaddr_storage in:
http://www.kame.net/newsletter/19980604
> For example, this cast: (unsigned char *)(&addr->addr_v4.s_addr) would
> not be necessary if the address were defined as unsigned char[4].
in_addr is the correct structure to store ipv4 addresses. using in_addr
instead of unsigned char[4] makes much easier to copy or compare ipv4
addresses. moreover, you don't have to care about the integer size in
64-bits architectures.
> I don't understand the new PASSIVE flag to lookup_host.
well, that's a problem. to get a socket address suitable for bind(2), you
must call getaddrinfo with the AI_PASSIVE flag set. for instance, if you
call:
getaddrinfo(NULL, "ftp", &hints, &res)
with the AI_PASSIVE flag, you get the :: port 21 and 0.0.0.0 port 21
socket addresses, while calling getaddrinfo without the AI_PASSIVE flag
returns the ::1 port 21 and 127.0.0.1 port 21 addresses.
the passive flag for lookup_host is a very unelegant hack, but i haven't
found a way to get rid of it, yet. any suggestion?
> In lookup_host, the comment says that you don't need to call
> getaddrinfo_with_timeout, but then you call getaddrinfo_with_timeout.
> An oversight?
>
> You removed this code:
>
> - /* ADDR is defined to be in network byte order, which is what
> - this returns, so we can just copy it to STORE_IP. However,
> - on big endian 64-bit architectures the value will be stored
> - in the *last*, not first four bytes. OFFSET makes sure that
> - we copy the correct four bytes. */
> - int offset = 0;
> -#ifdef WORDS_BIGENDIAN
> - offset = sizeof (unsigned long) - sizeof (ip4_address);
> -#endif
>
> But the reason the code is there is that inet_aton is not present on
> all architectures, whereas inet_addr is. So I used only inet_addr in
> the IPv4 case, and inet_addr stupidly returned `long', which requires
> some contortions to copy into a uchar[4] on 64-bit machines. (I see
> that inet_addr returns `in_addr_t' these days.)
>
> If you intend to use inet_aton without checking, there should be a
> fallback implementation in cmpt.c.
are there __REALLY__ systems which do not support inet_aton? their ISVs
should be ashamed of themselves...
however, yours seemed to me an ugly hack, so i have temporarily removed
it. as you say, it would be probably better to provide a fallback
implementation of inet_aton in cmpt.c.
> I note that you elided TYPE from ip_address if ENABLE_IPV6 is not
> defined. That (I think) results in code duplication in some places,
> because the code effectively has to handle the IPv4 case twice:
>
> #ifdef ENABLE_IPV6
> switch (addr->type)
> {
> case IPv6:
> ... IPv6 handling ...
> break;
> case IPv4:
> ... IPv4 handling ...
> break;
> }
> #else
> ... IPv4 handling because TYPE is not present without ENABLE_IPV6 ...
> #endif
>
> If it would make your life easier to add TYPE in !ENABLE_IPV6 case, so
> you can write it more compactly, by all means do it. By "more
> compactly" I mean something code like this:
>
> switch (addr->type)
> {
> #ifdef ENABLE_IPV6
> case IPv6:
> ... IPv6 handling ...
> break;
> #endif
> case IPv4:
> ... IPv4 handling ...
> break;
> }
that's a question i was going to ask you. i supposed you were against
adding the type member to ip_address in the IPv4-only case, so i had to
made all those "#ifdef ENABLE_IPV6" switches.
i'll change my code and add the type member to ip_address. i think that
it's the best solution.
P.S. please notice that by caching the string representation of IP
addresses instead of their network representation, the code
could become much more elegant and simple.
--
Aequam memento rebus in arduis servare mentem...
Mauro Tortonesi [EMAIL PROTECTED]
[EMAIL PROTECTED]
[EMAIL PROTECTED]
Deep Space 6 - IPv6 with Linux http://www.deepspace6.net
Ferrara Linux User Group http://www.ferrara.linux.it
