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.
Here are some remarks I got looking at the patch.
It inadvertently undoes the latest fnmatch move.
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. For example, this cast:
(unsigned char *)(&addr->addr_v4.s_addr) would not be necessary if the
address were defined as unsigned char[4].
I don't understand the new PASSIVE flag to lookup_host.
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.
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;
}
