Re: apt https method v2
On Fri, 5 Apr 2002, Jason Gunthorpe wrote:
> I'm not convinced that introducing the new Connection class instead of
> changing the semantics of CircleBuf is really best. Seems to add alot of
> extra text all over the place. If you look at carefully, the only reason
> serverstate has the FD at all is so it can call select. It only needs the
> FD for this, and it does not need the other baggage. The ugly cast inside
> your dirved circlebufs is probably a fair indicator that something is
> wrong. [see below for a more complete solution]
Ummm.. so which direction do you want me to try? Should I try moving the
Connect/Read/Write/Close/Open stuff into Connection or not (which will
make an even bigger diff!)
And if - how do you think Connection should look like? Should I do a http
version and subclass it into ConnectionSSL?
The problem that arises when doing this, and why you see the ugly cast, is
that in C++ it is impossible to have a virtual method that uses as a
parameter a subclass:
class ServerStateSSL: ServerState {
virtual bool Read( ConnectionHandleSSL c);
}
will not work. If there's one single place where I have to pass a
subclassed connection Handle as parameter in a derived class to a virtual
method then I will have to cast.
> The makefile stuff is fine.
>
> + GetAccessParams(&Acquire, &DefaultPort, &ProxyStr);
>
> This is C++.. references do exist.
Ack.
> 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);
Ack.
> 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)
>
> 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();
Ack - iif I should move everything into Connection.
> Indeed, the overall size of the patch and the total additional code could
> be considerably reduced if..
> - Connection actually represented a conneciton and could do things with it
> - CircleBuf was not subclassed
> - Connection had connect, close, read and write methods
Yes - see on the bottom of the email.
> 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's only there to math connect.cc. One can do ftps or whatever
protocolSSL the same way as https, so they could share the connect_ssl. If
that's too far reached I can move that into https.
> Comment at the top of https.cc should describe the interface to openssl.
Something like:
SSL_read(arg, arg): reads from an SSL stream
[...]
Or what exactly do you envision?
> Ugly:
>
> + Res = SSL_read(((ConnectionHandleSSL *)Connection)->Ssl,
> + Buf + (InP%Size),LeftRead());
>
> How about Res = Connection->Read(..); and completely remove the duplicated
> and virtual CircleBuf functions.
Ack - see below.
> 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.
Certainly, ack! :-)
> > 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.
OK.
So what I need to know to proceed without reimplementing everything
the next time over again in a different way:
a) do you want me to move all the methods that are Connection specifif
into the class?
b) I'd subclass Connection for the SSL case? Does that match your
aesthetics?
*t
-----------------------------------------------------------------------
Tomas Pospisek
Infinite Justice for the World:
http://www.heise.de/tp/deutsch/inhalt/co/11621/11621_2.jpg
------------------------------------------------------------------------
--
To UNSUBSCRIBE, email to deity-request@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
Reply to: