Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#1593 closed task (complete)

Cleanup of socket creator code

Reported by: vorner Owned by: stephen
Priority: medium Milestone: Sprint-20120221
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket: Socket creator
Estimated Difficulty: 4 Add Hours to Ticket: 0
Total Hours: 3.5 Internal?: no

Description

The socket creator (and the corresponding tests) use macros and break some coding guidelines. They should be cleaned up.

Subtickets

Attachments (1)

sa_cast.diff (4.1 KB) - added by jinmei 8 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 8 years ago by jelte

  • Milestone changed from New Tasks to Next-Sprint-Proposed

comment:2 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 4

comment:3 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120221

comment:4 Changed 8 years ago by stephen

  • Owner set to stephen
  • Status changed from new to assigned

comment:5 Changed 8 years ago by stephen

  • Owner changed from stephen to UnAssigned

Cleanup now ready for review. I've also extended the comment where I found things a bit confusing.

comment:6 Changed 8 years ago by jinmei

  • Status changed from assigned to reviewing

(moving to the review queue)

comment:7 Changed 8 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:8 Changed 8 years ago by jinmei

Overall I like the cleanup made in the branch.

general

  • I've made a couple of additional commits for further style cleanups. I believe they are non controversial.
  • Using exceptions is probably a good idea, but maybe we'd avoid using isc_throw and isc Exception classes. isc::Exception relies on <string>, which adds otherwise-unnecessary dependency on the C++ standard library from the socket creator. I guess we'd like to minimize dependency from the socket creator in general (for security reasons). And, in this case, since we explicitly catch the exception and don't use the what() string anyway, it seems we can simply define SocketCreatorError? (and its derived classes) as independent classes (not even derived from std::exception).
  • I'd like to avoid using reinterpret_cast unless there's absolutely no other alternative (and when we know what we are doing) such as for converting integer to a pointer so we can always be aware of where exactly we are using this beast. I'd suggest extending the existing pointer conversion utility in lib/util/io and using it (attaching a patch). In this case this conversion should actually be totally valid because we are using sockaddr* just as an opaque type.
  • adding comments to clarify the magic characters (these were a perfect example of 'read only code', which can be only understood by the original author) like this is good:
                    writeMessage(output_fd, "S", 1);   // ... in the socket() call
    
    but I wonder we'd even make them even more reader-friendly by introducing more descriptive constant variables like this:
    const char* const SOCKET_FAILURE = "S"; // or this could be just char
    

main.cc

We could avoid having a mutable variable, although in this case it may
be a matter of taste:

main() {
    try {
        run(0, 1); // Read commands from stdin, output to stdout
    } catch (const SocketCreatorError& ec) {
        return (ec.getExitCode());
    }
    return (0);
}

..., and while looking at it, I wonder whether this one is better than
hardcoding 0 and 1:

        run(stdin, stdout); // Read commands from stdin, output to stdout

sockcreator.cc

  • suggestion: I wonder whether handleRequest() (btw I renamed it from handle_request to be more aligned with the coding guideline) if we changed it to something like this:
        const int sock_type = getFamily(type[0]);
        const sockaddr* addr = getSockAddr(type[1]);
        ...
        } else {
            // Error.  Tell the client.
            writeMessage(output_fd, "E", 1);           // Error occurred...
            const char error_code = getErrorCode(result);
            writeMessage(output_fd, &error_code, sizeof(error_code));
    
    and getXXX is responsible for the conversion and throwing the exception for the unexpected cases. That way we won't be bothered with the mostly impossible cases of protocolError/IternalError in the main code logic. But if you think it's too much or don't think it a good idea, I'm okay with keeping the current version.

sockcreator.h

  • If you mean passing the FD through output_fd by "message/socket", I don't think it's correct. FDs are passed through a separate UNIX domain socket.
    /// \param output_fd File descriptor of the stream to which the output
    ///        (message/socket or error message) is written.
    

Changed 8 years ago by jinmei

comment:9 Changed 8 years ago by jinmei

  • Owner changed from jinmei to stephen

comment:10 follow-up: Changed 8 years ago by stephen

  • Owner changed from stephen to jinmei

I've made a couple of additional commits for further style cleanups. I believe they are non controversial.

No problems - they were some things I missed.

Using exceptions is probably a good idea, but maybe we'd avoid using isc_throw and isc Exception classes. isc::Exception relies on <string>, ...

I had thought that but had used isc::Exception for consistency - all code in BIND 10 uses the same base exception type and same way of throwing exceptions. The other thought was that although the failure is returned via a status code to the environment, we might in future want to log the reason for the failure via a call to syslog(), in which case a string would be useful.

...in this case, since we explicitly catch the exception and don't use the what() string anyway, it seems we can simply define SocketCreatorError (and its derived classes) as independent classes (not even derived from std::exception).

In fact there is no need even to define SocketCreatorError; we could just throw (and catch) an "int".

I don't mind either way - it depends if you think the arguments above outweigh the arguments for changing them.

I'd like to avoid using reinterpret_cast unless there's absolutely no other alternative (and when we know what we are doing) such as for converting integer to a pointer so we can always be aware of where exactly we are using this beast. I'd suggest extending the existing pointer conversion utility in lib/util/io and using it (attaching a patch). In this case this conversion should actually be totally valid because we are using sockaddr* just as an opaque type.

The code that was replaced was similar to your patch, i.e.

A* a;
B* b = static_cast<B*>(static_cast<void*>(a));

In other words, going from a pointer to one type to a pointer to an incompatible type via a void* pointer. I understand the point about sockaddr as an opaque type (so it is an "allowable" incompatible cast), but the same method could be used for an "invalid" incompatible cast, e.g. to end up having sockaddr* variable pointing to an int. So I changed the code to use reinterpret_cast on the grounds that if it walks like a duck and it quacks like a duck... Also, it is shorter and simpler to understand.

An alternative that avoids a cast is a union:

union {
    sockaddr     addr;
    sockaddr_in  addr_in;
    sockaddr_in6 addr_in6;
};

This would emphasise that the different structs are alternative ways of representing the same data. For the time being I have left the code as it is (with reinterpret_cast) but am happy to change it.

adding comments to clarify the magic characters (these were a perfect example of 'read only code', which can be only understood by the original author) like this is good ... but I wonder we'd even make them even more reader-friendly by introducing more descriptive constant variables like this

Possible, but the REAME description of the protocol explicitly talks about the individual characters sent back. To make it easier to relate that description to the code I would suggest that we leave the literal text as is.

We could avoid having a mutable variable, although in this case it may be a matter of taste

For historical reasons I prefer functions to have one entry point and one exit point. But in this case the code is so trivial it doesn't really matter so I've adopted your suggestion.

and while looking at it, I wonder whether this one is better than hardcoding 0 and 1:

run(stdin, stdout); // Read commands from stdin, output to stdout

stdin/stdout are FILE* variables, but I have altered it to use STDIN_FILENO and STDOUT_FILENO.

suggestion: I wonder whether handleRequest() (btw I renamed it from handle_request to be more aligned with the coding guideline) if we changed it to something like this:

I did this for the suggested getFamily() (renamed to getSocketType() to avoid confusion with the address family) and getErrorCode(). (Incidentally I used a mutable type (c.f. your comments on main.cc) to avoid compiler warnings in getSocketType(), as protocolError() does not return. Although not strictly necessary for getErrorCode() - the compiler recognises that isc_throw will not return - it was used to emphasise the similarity of the code structure to that of getSocketType().)

The problem with the suggested getSockAddr() function is that the sockaddr pointer is a pointer to another data structure. Either that would have to be allocated statically within getSockAddr() or allocated dynamically with the caller responsbile for deallocating it. Either seemed to make the code more complicated so I left it as is.

If you mean passing the FD through output_fd by "message/socket", I don't think it's correct. FDs are passed through a separate UNIX domain socket.

As I understand the fs_send code, it is done via the CMSG macros which "are used to create and access control messages (also called ancillary data) that are not a part of the socket payload" (cmsg(3) man page). output_fd appears to be the socket used for communication. I have updated the header though.

comment:11 in reply to: ↑ 10 Changed 8 years ago by jinmei

Replying to stephen:

Using exceptions is probably a good idea, but maybe we'd avoid using isc_throw and isc Exception classes. isc::Exception relies on <string>, ...

[...]

I don't mind either way - it depends if you think the arguments above outweigh the arguments for changing them.

I'm personally okay with keeping isc::Exception with <string>. Others
(e.g. Michal) may have a different opinion, however, so my suggestion
is to keep the current version for the purpose of this ticket and
confirm the intent at bind10-dev. Also, if we keep using it, I'd
explicitly add dependency to libexception in the Makefile.am:

b10_sockcreator_LDADD += $(top_builddir)/src/lib/exceptions/libexceptions.la

I suspect currently it happens to work because libutil_io internally
adds this dependency. But if the sock creator becomes a direct user
of it, it's fragile to rely on the indirect dependency setup.

I'd like to avoid using reinterpret_cast [...]

The code that was replaced was similar to your patch, i.e.

A* a;
B* b = static_cast<B*>(static_cast<void*>(a));

Yes, I know.

In other words, going from a pointer to one type to a pointer to an incompatible type via a void* pointer. I understand the point about sockaddr as an opaque type (so it is an "allowable" incompatible cast), but the same method could be used for an "invalid" incompatible cast, e.g. to end up having sockaddr* variable pointing to an int. So I changed the code to use reinterpret_cast on the grounds that if it walks like a duck and it quacks like a duck... Also, it is shorter and simpler to understand.

I agree the above nested static_cast is almost equally evil as
reinterpret_cast in that you could abuse it (and the intent is often
unclear - whether the developer was confident it was actually safe or
just tried to deceive the compiler). The convertSockAddr() templates
are an attempt to at least make the intent clearer: they are supposed
to be used for only converting between compatible sockaddr variants
when it's known to be safe, although the template interface does not
prevent abuse. If we want we could even perform static checker to
enforce to specify the templated type explicitly and must be some
sockaddr(_xxx).

In that sense, it won't be much different if we add detailed comments
on why we use reinterpret_cast and why it's actually safe in this
specific case. In fact, I'd like to propose doing so wherever we use
reinterpret_cast in our code (and it would be checked in review) -
that would discourage an easy use of really dangerous reinterpret_cast
in development. And, by reducing the number of cases where
reinterpret_cast is used, we can more easily check if we follow this
convention via a simple grep and eye-check. If grep produces 100
matches that would be more difficult. For this reason I'd still
prefer avoiding reinterpret_cast in this case.

An alternative that avoids a cast is a union:

union {
    sockaddr     addr;
    sockaddr_in  addr_in;
    sockaddr_in6 addr_in6;
};

This would emphasise that the different structs are alternative ways of representing the same data. For the time being I have left the code as it is (with reinterpret_cast) but am happy to change it.

Right, in the specific case of sockaddr variants, this is equally
safe. My concern with this approach is that it could convey a false
sense of message that we could abuse union to deceive the compiler.
Detailed comments about it may help discourage such attempts, though.

Going back to the branch, I'd prefer avoiding reinterpret_cast
somehow. If you are okay with it and have a stronger preference on
one of the alternatives, I'm okay with that if we can make it clearer
why we do this, why it's safe in this specific case, and that the same
workaround shouldn't easily be adopted just to deceive the compiler
(especially if you use an alternative that is different from the
existing convertSockAddr() framework that is dedicated for that kind
of purpose); if you still think reinterpret_cast is the best among all
approaches, I think we should discuss project-wide to reach consensus.

adding comments to clarify the magic characters (these were a perfect example of 'read only code', which can be only understood by the original author) like this is good ... but I wonder we'd even make them even more reader-friendly by introducing more descriptive constant variables like this

Possible, but the REAME description of the protocol explicitly talks about the individual characters sent back. To make it easier to relate that description to the code I would suggest that we leave the literal text as is.

Okay.

and while looking at it, I wonder whether this one is better than hardcoding 0 and 1:

run(stdin, stdout); // Read commands from stdin, output to stdout

stdin/stdout are FILE* variables, but I have altered it to use STDIN_FILENO and STDOUT_FILENO.

Oops, I was confused. Confirmed the revised code.

suggestion: I wonder whether handleRequest() (btw I renamed it from handle_request to be more aligned with the coding guideline) if we changed it to something like this: [...]

The problem with the suggested getSockAddr() function is that the sockaddr pointer is a pointer to another data structure. Either that would have to be allocated statically within getSockAddr() or allocated dynamically with the caller responsbile for deallocating it. Either seemed to make the code more complicated so I left it as is.

Okay.

If you mean passing the FD through output_fd by "message/socket", I don't think it's correct. FDs are passed through a separate UNIX domain socket.

As I understand the fs_send code, it is done via the CMSG macros which "are used to create and access control messages (also called ancillary data) that are not a part of the socket payload" (cmsg(3) man page). output_fd appears to be the socket used for communication. I have updated the header though.

Oops, you're right, I was confused on this, too. I didn't know we can
pass FD via the interprocess channel with the stdin/stdout interfaces.
So the original description was actually correct, but the revised
version is also okay.

Finally, I made a few more trivial style cleanups.

comment:12 Changed 8 years ago by jinmei

  • Owner changed from jinmei to stephen

comment:13 follow-up: Changed 8 years ago by stephen

  • Owner changed from stephen to jinmei

Also, if we keep using it, I'd explicitly add dependency to libexception in the Makefile.am

Missed that - done.

Going back to the branch, I'd prefer avoiding reinterpret_cast somehow. If you are okay with it and have a stronger preference on one of the alternatives...

Not really. It was more of a case of pointing out that however much we are avoiding the keyword "reinterpret_cast", that is exactly what we are doing. However, I've applied the contents of the patch you attached to the ticket and the code now uses convertSockAddr().

As this is a refactor, not changing functionality or fixing a bug, I was not planning on a ChangeLog entry.

comment:14 in reply to: ↑ 13 Changed 8 years ago by jinmei

The branch is now okay for merge for me.

comment:15 Changed 8 years ago by jinmei

  • Owner changed from jinmei to stephen

comment:16 Changed 8 years ago by stephen

  • Resolution set to complete
  • Status changed from reviewing to closed

Merged to master in commit aacfc3fee9cbf5a2b7945c165cace1af81877f5f

comment:17 Changed 8 years ago by jinmei

  • Total Hours changed from 0 to 3.5
Note: See TracTickets for help on using tickets.