[bind10-dev] Socket creator and low-level code

[Michal 'vorner' Vaner]

Hello

On Wed, Oct 20, 2010 at 11:38:26PM +0900, JINMEI Tatuya / 神明達哉 wrote:
> Some specific examples: the current version have code like this
> (omitting the error handling for simplicity).
> 
> ssize_t
> read_data(const int fd, void *buffer_v, const size_t length) {
>     unsigned char *buffer(static_cast<unsigned char *>(buffer_v));
>     size_t rest(length), already(0);
>     while (rest) { // Stil something to read
>         ssize_t amount(read(fd, buffer, rest));
>         if (amount) {
>             already += amount;
>             rest -= amount;
>             buffer += amount;
>         } else { // EOF
>             return already;
>         }
>     }
>     return already;
> }
> 
> Using the read(2) system call is probably okay (higher level
> abstraction like ASIO is probably overkilling for this purpose), but
> I'd like to restrict the use of raw pointer only for that operation.
> 
> We could do, for example, as follows (although I admit this may not be
> sufficiently clean - this is an example to explain the concept):
> 
> ssize_t
> read_data(const int fd, std::vector<uint8_t>& buffer, const size_t length) {
>     uint8_t readbuf[length];
>     size_t rest(length);
> 
>     while (rest > 0) {
>         const ssize_t amount(read(fd, readbuf, rest));
>         if (amount > 0) {
>             buffer.insert(buffer.end(), readbuf, readbuf + amount);
>             rest -= amount;
>         } else {
>             break;
>         }
>     }
>     return (buffer.size());
> }
> 
> Also, instead of doing this:
>                         READ(static_cast<char *>(static_cast<void *>(
>                             &addr_in.sin_port)), 2);

Ups, I really left the casts here? They are unnecessary, that's artefact from
time I tried having char *buffer in read. READ(&variable, sizeof variable) looks
better.

> I'd introduce a wrapper to extract a uint16_t value from a read
> buffer, and do something like this:
> 
> 	       		vector<uint8_t> read_buf;
> 	       		readData(input_fd, read_buf, 2)
>                         addr_in.sin_port = htons(getUint16(read_buf));
> 
> where getUint16() is
> 
> uint16_t
> getUint16(const vector<uint8_t>& buffer) {
>     return ((buffer.at(0) << 8) | buffer.at(1));
> }

While you say pointers are dangerous, I would like to say, pointers are not
dangerous like a tiger, which bites you if it has bad mood, so the only
reasonable way to do is to keep away from tigers. It is dangerous in the same
sense as a chain saw. You should really read the manual and be careful while
using it, but if you do it right, you are safe.

Saying that, I would like to point out why I think this interface you propose is
bad (and propose a different one later on).

Let's first look at what pointer problems might arise:
• Memory leak ‒ this can not happen, nothing is allocated dynamically.
• Dereferencing invalid pointer ‒ this also can not happen. Nothing is
  allocated, so NULL is not passed somewhere by accident neither freed memory
  nor uninitialized pointer, for the same reason as above.
• Passing a buffer and not it's size, so running out of it (strcpy, gets…). It
  is not used here.
• Passing wrong size of buffer, so running out of it. This can happen. However,
  your interface has the same problem and it is on 2 places instead of one (at
  the read and at the conversion function).

So, it does not really solve anything. Furthermore, it is more complicated,
there's need for the conversion functions. Another place for a bug to appear and
it is unnecessary. Your getUint16 is, for example, wrong, because it expects the
data to come in big endian. The only place I send 16bit number is port and I
expect it to be big endian, however, I send a host-encoded integer as well and
it would have to differentiate the endianness and if someone wanted to pass a
normal 16 bit unsigned, he couldn't use your function. And it just adds a layer
of conversion there and back where currently is nothing (unnecessary glue).

Furthermore, it is logically wrong. Usually, C++ type checks should prevent you
from using the wrong type. However, because the bits you have are usually not an
array of bytes, but some type, it actually prevents you from using the _correct_
type. You have the correct bits, but look at them wrongly. The most correct
conversion here would be reinterpret_cast, which looks awful.

The fact that the data is copied several times on the path is just a bad bonus.


So, my suggestion about interface of the read and write is:
• Leave read_data and write_data as only wrappers around read and write and keep
  their interface, because it is both common and known, therefore poses no
  surprise.
• Add wrapper around them to be used in normal circumstances, like:

template<typename T>
bool
readVar(int fd, T &variable) {
  return (read_data(fd, &variable, sizeof(typename T)) == sizeof(typename T));
}

This is much more usable. Does it seem right for you as well, regarding the
pointer-cleanness and so? It solves even the wrong-length bug and in case
someone would like to send more complicated data structures (like std::string),
a template-specified function could be written and solve it (thought I doubt
there will be need for that any time).

There are 2 more places where pointers are used. There's memset, because the
sockaddr_* structures have no constructors. It could be overcome by using a
global variable (so it is zeroed at the start of program) at the cost of reusing
it.

Other place is the interface of BSD sockets, namely bind. I have no idea what to
do with this. But I use it only as a reference to the correct sockaddr_*
structure. I maybe should add an assert to ensure it is really set.


According to Evans notes, it is not compiled as C because there's no other C
code in the project and we would need to teach the build system that and because
it uses C++ header files with namespaces. I wanted to use C (C99 has some really
nice features), but I decided not to because of this.

Thank you

Have a nice day

-- 
I still miss Windows, but my aim is getting better.

Michal 'vorner' Vaner
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <https://lists.isc.org/pipermail/bind10-dev/attachments/20101021/7f928f41/attachment.bin>