http_proxy client

[Alexey M. <al...@hs...>]

>>>>> "AM" == Alexey Morozov <mo...@no...> writes:

AM> I have committed the files into NCLI-1-11-1 branch. The http_proxy
AM> method looks Ok (I have tested it a bit), although authorized http
AM> proxy access method was not in place yet (does anybody really need
AM> it?).

AM> I had to change certain in the client.c and socket-client.h. Alex,
AM> please review it and tell if it's Ok.


http_proxy-client.c is ~400 lines.  That's twice as large as other
clients.  Whoa! 


First, proper handling of errors should be like this:

             return ncli->error(ncli, "proxy has closed connection");

And _not_ like

	     ncli->error(ncli, "proxy has closed connection");
	     return NCLI_FAILURE;

Actually, ncli->error() is not supposed to return.  I've played a bit
with __attribute__((noreturn)), but GCC doesn't allow to specify this
attribute for typedef or at least I could not do that.

That change alone would actually save couple dozens of lines. 

(This was documented in ncli-architecture.txt, though not so clearly,
maybe).


Second, the network-clients should never create each other.  That is,
the http_proxy client should never create a socket-client for itself.
Instead, it should rely on client.c to create socket-client, and then
to put the http_proxy client on top of it.

The http_proxy read/write/disconnect methods thus should simply call
the

   ncli->next->disconnect()

etc. 


The http_proxy connect method should parse the cvs_proxy env-var, call
the ncli_socket_connect() (don't confuse it with socket_connect()),
and try to negotiate the proxy-connection.


Third, it is actually ok to use regexes :) I once rewrote the
parse_cvsroot() with regexes to handle port numbers.  So, you can
safely rewrite, if that would make the code more clear (and I am sure
it would, especially considering that:


Fourth, when you'll rewrite to regexes, you may use extremely strict
and simple method of specifying the "cvs_proxy" environment variable
(and that's how it should be called, BTW: to be consistent with
http_proxy and ftp_proxy).  You could simply use the

    cvs_proxy=http://proxy.example.org:3128/

and that's all.  Note that there is _no_ default port as far as I
know.  So don't hassle with all that cases -- it's not worth it.  


Fifth, the abomination of connect_line is completely unneeded, sorry.
You could simply use the POSIX fdopen(), and 

    printf("CONNECT %s:%hu HTTP/1.1");

That'll save a lot of free(connect_line).   Don't forget to fflush().


Sixth, wrt handling HTTP-response from proxy.  I really think that you
could use the ncli_read_line(), read a line, and then simply sscanf()
it, instead of strcmp()'ing its various parts.  sscanf() turned out
extremely powerful for simple cases, and it seems like the
HTTP-response _is_ a simple case.


Whoa!  I hope that we'll greatly improve the http_proxy-client.c using
the above suggestions :)


AM> Please note in order to build things properly you have to re-run
AM> 'aclocal; autoconf; automake', 'cause I didn't commit local
AM> ./configure, it differs from server's one too much (377 lines and
AM> I'm not sure that's not my local misconfigurations/features).

--alexm