BIND 10 #1452: add UDP socket passing to fd_X handling
BIND 10 Development
do-not-reply at isc.org
Fri Dec 16 23:12:58 UTC 2011
#1452: add UDP socket passing to fd_X handling
-------------------------------------+-------------------------------------
Reporter: jelte | Owner: jinmei
Type: task | Status: reviewing
Priority: major | Milestone:
Component: DDNS | Sprint-20111220
Keywords: | Resolution:
Defect Severity: N/A | Sensitive: 0
Feature Depending on Ticket: DDNS | Sub-Project: DNS
Add Hours to Ticket: 0 | Estimated Difficulty: 5
Internal?: 0 | Total Hours: 0
-------------------------------------+-------------------------------------
Comment (by jinmei):
Replying to [comment:8 vorner]:
Thanks for the review.
> Replying to [comment:5 jinmei]:
> It's quite OK to keep in mind, even when large code-wise.
You're very good at reading/reviewing a large patch:-)
> First, the library now uses exceptions, so it must link to it. I pushed
a makefile fix, so it now compiles for me.
Ack, thanks.
> The tests fail for me:
> {{{
> [ RUN ] ForwardTest.pushTooFast
> socketsession_unittest.cc:685: Failure
> Expected: multiPush(forwarder_, *getSockAddr("192.0.2.1", "53").first,
large_text_.c_str(), large_text_.length()) throws an exception of type
SocketSessionError.
> Actual: it throws nothing.
> [ FAILED ] ForwardTest.pushTooFast (0 ms)
> }}}
>
> My guess for this is, linux takes the buffer size as some kind of hint
only and makes its own mind about the proper size and 3 messages fit
without a problem.
Ah, okay, google told me that Linux doubles the specified buffer size
by setsockopt(). So, increasing it to 4 (or perhaps 5) should be
enough. I've changed it to 10 to be a bit conservative (commit
8fda3f0). As commented in that commit, if it still makes it fail we
should take a more reliable approach, but for now I prefer the brevity.
> Also, is the „receptor“ name a good one? It reminds me with the little
parts of cells that detect which chemicals are around and what temperature
it is. Shouldn't it be a „recipient“, for the one who receives?
Actually I was not sure if this naming is a good one. So I have no
problem with changing it. I've changed it to "Receiver" (not a strong
preference over "Recipient", but the former is a bit shorter and would
look more consistent compared to "Forwarder", in that they share the
suffix of "er").
> I'm not sure about how the blocking reads are used and such.
For the moment I wanted to keep the interface simpler; asynchronous
read will make it more complicated while in many cases the
communication between these two processes is not expected to be
blocking (if it blocks, it's quite likely that the receiver side is
too busy and will have to drop some of the request anyway). Note also
that 2 full-DNS messages are quite large for the expected usage. In
many cases each session data should be much smaller.
That said, I don't deny that it could be improved in a future
version. For now I'd move forward with this style of operation and
see how practical it is.
> Also, I heard some system (linux is one of them) can cause a spurious
wakeup on a select for a socket that has no data to read.
If this happens on the UNIX domain socket we are using for the socket
session passing, that would indeed be a problem. In that case we
should consider a workaround (but note that blocking read on the
receiver side is not new in this implementation - we already do this
in the current recv_fd() interface. So we should've seen it if it
really happens at some noticeable level of frequency).
> Also, there are several mostly minor issues:
> * Should the SA_TYPE in template be upper case? Don't we use it for
constants and enum values? This is type, so it should be CamelCase:
> {{{#!c++
> template <typename SA_TYPE>
> }}}
Hmm, maybe. It was not a strong intent, but I kind of intended to
make it clear that it's a template parameter, not a real type name.
But that's not in the common guideline, and in any case it wouldn't
matter much for such a small function. So I changed it to SAType.
> * Should we use cerrno instead of errno.h? The same applies for
stdint.h and string.h.
Hmm, perhaps. The header files provided with g++ don't make them so
different, but in terms of honor namespaces cerrno (etc) should be
better than errno.h (and in any case my first branch wasn't
consistent; I used both cassert and errno.h). I changed the code to
use cXXX versions as much as possible ("cstdint" doesn't seem to be a
standard or at least not so portable, so I didn't use it).
> * Should the local includes be done first, before other headers, to
ensure they don't use something from the system headers included above?
Hmm, I don't know. Is that a common practice? I have no problem with
reordering them that way if it's more common, but in that case I think
we should rather make it in our project-wide guideline. For now I've
not make a change on this point.
> * Should this use `UNIX_MATH_PATH` instead?
> {{{#!c++
> (sizeof(impl.sock_un_.sun_path) - 1 < unix_file.length())
> }}}
You mean UNIX_MAX_PATH? I thought it was not portable.
> * If you already check the size, why do you use the (more complicated)
strncpy instead of strcpy? Is there a trick I'm forgetting?
> {{{#!c++
> strncpy(impl.sock_un_.sun_path, unix_file.c_str(),
> sizeof(impl.sock_un_.sun_path));
> }}}
It was a kind of defense of depth (or being paranoid). Especially
because it's beyond the super trivial level (using a sizeof for a
structure member - see above why, using string::length(), etc), I
thought it could be possible that the length check may have a bug. I
wanted to make sure that even with such a case it at least doesn't
cause things like buffer overflow (this is also the reason for the use
of assert).
Maybe we should at least clarify the point? e.g.
{{{#!c++
// the copy should be safe due to the above check, but we'd be rather
// paranoid about making it 100% sure even if the check has a bug
(with
// triggering the assertion in the worse case)
strncpy(impl.sock_un_.sun_path, unix_file.c_str(),
sizeof(impl.sock_un_.sun_path));
assert(impl.sock_un_.sun_path[sizeof(impl.sock_un_.sun_path) - 1] ==
'\0');
}}}
> * I don't see why the „rest“ of the buffer with the path should be
zeroed.
At least my man page states strncpy ensures that:
{{{
The strncpy() function copies at most n characters from s2 into s1.
If s2
is less than n characters long, the remainder of s1 is filled with
`\0'
characters. Otherwise, s1 is not terminated.
}}}
Isn't it part of standard?
> * Where does the magic number „2“ come from here?
> {{{#!c++
> impl.sock_un_len_ = 2 + unix_file.length();
> }}}
Ah, okay, it should have (better) been sizeof(uint16_t). Changed.
(in case it's still not clear, it's for the "2-byte header length").
> * Should the right side be impl.sock_un_len_? Nobody anywhere assigned
to sock_un_len_ and I don't see it anywhere.
> {{{#!c++
> #ifdef HAVE_SA_LEN
> impl.sock_un_.sun_len = sock_un_len_;
> #endif
> }}}
Good, catch, you're right. This made me notice that I should have
included config.h (which would have made the bug visible for me).
Fixed both points.
> * As described in the man page (The freeaddrinfo() function frees the
memory that was allocated for the dynamically allocated linked list res.),
the freeaddrinfo should be called only once on the whole result ‒ it frees
the whole list, not on each of the nodes there. I guess there was no
problem here, as it produces only single-element lists here, but it's
needlessly complicated and, in principle, wrong, as it may create double-
free errors.
I'm not sure what's wrong about this point. addrinfo_list_ should be
a list of the head pointer of the addrinfo chain. So this shouldn't
cause duplicate free:
{{{#!c++
vector<struct addrinfo*>::const_iterator it;
for (it = addrinfo_list_.begin(); it != addrinfo_list_.end();
++it) {
freeaddrinfo(*it);
}
}}}
(if that's what you are talking about). Or am I missing something or
misunderstanding you?
--
Ticket URL: <http://bind10.isc.org/ticket/1452#comment:9>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list