Re: [MERGE] Connection pinning patch

On Tue, 2008-09-23 at 01:31 +0300, Tsantilas Christos wrote:
 
> > *B1* The ConnStateData::getPinnedInfo seems to have serious
> > side-effects like closing file descriptors and removing close handlers.
> > This is not a simple constant getter! Please rename and mention that the
> > call may close the descriptor in the method description.
> >
> Maybe it is better to split in two methods one method which check pinned
> info state and if it is required close the invalid pinning connection
> and an other one which simply returns the pinned info....

Sounds good to me. This may also help to remove some code duplication
among the two new ::getPinned* methods.

> > *B2* Something potentially wrong with descriptor management here:
> >
> >> + int pinning_fd = pinning.fd;
> >> + if (pinning_fd < 0)
> >> + return -1;
> >> +
> >> + if (pinning.auth && request && strcasecmp(pinning.host, request->GetHost()) != 0) {
> >> + comm_close(pinning_fd);
> >> + return -1;
> >> + }
> >
> > If the if-statement condition is true, the pinning.fd is pointing is a
> > closing descriptor. Is that what you want?
>
> Or just the pinning.fd is not set. I think in both cases returns the
> correct value....

The returned value is correct.

> The pinning.fd will set in clientPinnedConnectionClosed comm_close
> handler. I think it is better to not touch it in this function....

My wording was clumsy, sorry. Let me rephrase. I am worried that
pinning.fd is positive but the corresponding descriptor is closing. I am
worried that some cleanup code will look at a positive pinning.fd and
try to use it. Can that happen before clientPinnedConnectionClosed is
dialed?

I think there are two different approaches of descriptor management:

1) Invalidate the descriptor (fd) when calling comm_close. This protects
the code from using a closing descriptor.

2) Invalidate the descriptor (fd) when comm_close calls us. This avoids
the illusion that we know when the descriptor starts closing. We do not
know that, in general, because some other code can close our descriptor.

I think your code above is using approach #2 and I was kind of rooting
for approach #1. I am not sure which approach is better, but it would be
nice to be consistent.

Does anybody have an opinion on which approach should be used, in
general (other than the opinion that the whole mess should be replaced
with a better API; which is correct, but not relevant for this
project)?

If there is one approach that should be used in most cases, I will add
it to Comm API notes.

Thank you,

Alex.
Received on Mon Sep 22 2008 - 22:56:58 MDT