v3, was: Re: apt https method v2
As allways v3:
http://sourcepole.ch/sources/software/apt_https/apt_0.5.4-0.3.diff.gz
And from now on also for those who want to go testing straight away
without building:
http://sourcepole.ch/sources/software/apt_https/apt_0.5.4-0.3_i386.deb
On Fri, 5 Apr 2002, Jason Gunthorpe wrote:
> The makefile stuff is fine.
>
> + GetAccessParams(&Acquire, &DefaultPort, &ProxyStr);
>
> This is C++.. references do exist. Good idea otherwise
Done.
> This can't be right:
> + string DefProxy = _config->Find(Acquire);
> + string SpecificProxy = _config->Find(Acquire + ServerName.Host);
>
> The 2nd should likely be:
> + string SpecificProxy = _config->Find(Acquire + "::" +ServerName.Host);
Thanks, done.
> DefaultPort and ServerName.Access are at odds, they really need to
> be matched:
> + if (Connect(Host,Port, ServerName.Access.c_str(), // Access == http[s]
> + DefaultPort, Connection->ServerFd,TimeOut,Owner) == false)
I do not understand you here. What is not OK here? It's the Connect method
being used the same way it was in the unpatched version. What's the
problem with that? I do not understand the semantics of Connect having
Host, Port, Method, and Default Port in in the first place?
> If a connection is ment to encapsulate the SSL session as well as it's
> other gunk the this:
> + close(Connection->ServerFd);
> Could read:
> Connection->Close();
Done. Additionaly I can move GetAccessParams into ConnectionHandle as
well. This will require some more moving around of code, but then we can
get rid of ServerStateSSL and HttpMethodSSL alltogether. If you can, have
a quick look at it - it will require among other things splitting up
configuration reading, which is a bit awkward.
And since we're at it, I could kill https.* alltogether and move the
remaining ConnectionHandle class into connection.* where it IMHO belongs
to. That way I'd get rid of connection_ssl.* as well. The problem is: to
be *really* clean, we'd have to clean up ftp.* as well, so it uses
ConnectionHandle and company as well - and I'm not going to do that now
(I've allready spent to much time shuffling around code).
> I'm confused why the connect_ssl.cc file exists at all. Suck it in the
> https file with the rest of the SSL functions.
It can be used for all kinds of other ssl tunnels as well, as f.ex. ftps.
Still waiting for your opinion. Integration into https.* is no problem.
> Comment at the top of https.cc should describe the interface to openssl.
Done.
> Ugly:
>
> + Res = SSL_read(((ConnectionHandleSSL *)Connection)->Ssl,
> + Buf + (InP%Size),LeftRead());
Anihilated.
> ServerState is created/destroyed many times during normal operation. I see
> + In = new CircleBuf(64*1024);
> + Out = new CircleBuf(4*1024);
> + Connection = new ConnectionHandle();
>
> But no corrisponding deletes anywhere in the patch, thus it is leaking
> memory.
Should be fine now. Btw. - shouldn't ~HttpMethod also:
delete File;
?
> > One other question - while hacking at apt, the many clog's were very
> > use- and helpful to me to see what's happening "behind the scenes".
> > Would you mind to keep them in if I'd replace them by s.th. like:
>
> I prefer to remove them and use strace instead.
The problem with strace is that you only see the system calls there. But
you have no clue what's happening _inside_ the program. I much prefer to
see what's going on in the program itself.
I have left the debuging stuff inside for now. If you're fine with the
rest I can take my debuging stuff out and send you a clean version.
Inline:
As you'll see in the patch I have defined most of ConnectionHandle*
methods as inline. I have no idea whether gcc is clever enough to replace
simple wrapper methods by the function that is being wrapped, so I've done
it that way. If you don't like it I can take it out. If you agree with it,
then there are many other places where the method should be inlined.
*t
-----------------------------------------------------------------------
Tomas Pospisek
sourcepole - Linux & Open Source Solutions
http://sourcepole.com
Elestastrasse 18, 7310 Bad Ragaz, Switzerland
Tel:+41 (81) 330 77 13, Fax:+41 (81) 330 77 12
------------------------------------------------------------------------
--
To UNSUBSCRIBE, email to deity-request@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Reply to: