It might be worth also taking into account IPv6 addresses in your implementation.

Yes, agreed. This is what I was hinting at with the 1:2:3:::::5 IP
address above (except that I left out a colon). :-)

Ooops, sorry, it was buried in the ipv4 addresses and I didn't notice it

/me goes to buy some new eyes ;)

go malcolm!

Created attachment 16629 [details] [review]
Ignore some hosts when dealing with a proxy

This patch implements more or less everything that is promised and I
have tested it for a few situations (ph34r my IPv6 usermode linux
skills, d00dz! My brain hurts after setting all that up).

It _really_ needs a review though, since it feels very messy. All the
netmask messing around looks awkward, so rip into it and point out
what obvious function I am not using.

I haven't read the patch carefully yet, but I already have a small nit ;)
For the type field of struct ProxyHostAddr, some 'IPv6' and 'IPv4' constants  (some #define or enum) would be more readable than the hardcoded 4 and 6 imo.

I had a quick look, and it looks good.
Some comments though:

Why are you doing the massive g_strdup_printf stuff to calculate the
mask from the width? Can't you just use (~0)<<width or something.

Why do you need this:
+		else {
+			inet_pton (AF_INET, "255.255.255.255", &mask);
+		}
I didn't see mask used in that case.

You're not correctly handling IPv6 addresses that are just wrapped
IPv4 addresses. I.E. ::ff:xx:xx:xx:xx need to match xxx.xxx.xxx.xxx in
the ignore list.

Also, I'm slightly worried about the case where you have IPv6
addresses in the ignore list, but log in on a box with !HAVE_IPV6,
although handling the IPv6 address like a hostname *should* work.

Thanks for the review, guys. Addressing the comments in order:

Christophe: 
- agreed. I will make an enum with suitably named entries.

Alex:
- The width calculation: I need to pass a dotted quad to inet_pton, so
a certain amount of messing around seemed to be required. Are you
suggesting I just compute the value directly and then use ntohl() and
poke inside a struct in_addr directly? That should work, I guess and
it will make things a lot neater. The IPv6 case reduces to four tests
(struct in6_addr is an array of four uint32's) then.

- The 255.255.255.255 mask is so that we can be sure that the mask
structure is always filled in and so when comparing the IP addresses
later on, it will always be legal to do (in.s_addr & mask.s_addr).
However, the &mask bit is a blunder: it should be &elt->mask.

- I will fix IPv6-wrapped IPv4 addresses. I had forgotten about his case.

- I had intended for things to just fall through to the hostname case
when we encounter an IPv6 address without IPv6 support. The problem
with any other way of handling it is that we don't have the functions
available to process the addresses (or even detect that they are
IPv6). I will add a comment to clarify this to the next patch.

Created attachment 16668 [details] [review]
New patch incorporating review comments

Looks good from a quick look.
I don't know if it matters much, but couldn't yo do the IPv6 mask
calculation like this:
for (i=0; i < width/32; i++)
     mask[i] = 0xffff;
mask[i] = htonl (~0 << width % 32);

+        if (!has_error) {
+            gchar *dst = g_new0 (gchar, INET_ADDRSTRLEN);
+    
+            gl_ignore_addrs = g_slist_append (gl_ignore_addrs, elt);
+            DEBUG_HTTP (("Host %s/%s does not go through proxy.",
+                    hostname,
+                    inet_ntop(AF_INET, &elt->mask, dst, INET_ADDRSTRLEN)));
+            g_free (dst);
+        }

dst seems to only be used in the DEBUG_HTTP statement.
Could you #ifdef out the g_new0/g_free when DEBUG_HTTP_ENABLE isn't defined ?

I can make both those changes. However, Christophe, your one will make
the code less readable by scattering a couple of #ifdefs around and
any decent compiler is going to optimise away the use of dst in that
case anyway, since it can see that it is not used anywhere else.
Still, it's not that important to worry about so I will make the change.

Whoops. The fragment that Christophe posted is broken anyway. It will
not print a decent debug message when we are adding an IPv6 host. I'll
rewrite that bit completely (maybe make it an external function call
that can be #ifdef'd away in the non-debug case).

#ifdef ENABLE_HTTP_DEBUG
        gchar *dst = g_new0 (gchar, INET_ADDRSTRLEN);
        printf (("Host %s/%s does not go through proxy.",
        hostname,
        inet_ntop(AF_INET, &elt->mask, dst, INET_ADDRSTRLEN)));
        g_free (dst);
#endif
gl_ignore_addrs = g_slist_append (gl_ignore_addrs, elt);

seems readable enough to me, but that's your code, feel free to do what you think is better ;)

What's up with that bug ? Feature freeze is on monday (though I'm in
favour of considering it as a bug in the current proxy support rather
than a feature ;)

I thought I was waiting on you guys. So far Alex has only indicated
that he has had a "quick look". Do you want another patch
incorporating the trivial changes mentioned above or what?

Note, however, that it is *highly_*unlikely I will get the work done
for 44098 today. Work has been too busy the last couple of weeks for
me to devote much time to this. I am going to try to do it, but it may
not make it, which will render this bug pretty moot for 2.4, since
nobody will be able to use it. :-(

Oh, I thought you'd post a slightly updated patch since you said the
debug case in your last one didn't work. Even if it has no UI yet, I
think it's worth having it in, app writers know which key they are
supposed to use to get the hosts which shouldn't be proxied, and
people who really need this feature can set the key manually.

I'm fine with it going in as is. We can do more tuning later if needed.

is in.