Unit tests and bug fixes for XmlRpcSocket #1218
Conversation
trainman419
commented
Nov 3, 2017
•
trainman419
commented
- Add system call mocks for socket functions, and use these to test the XmlRpcSocket class.
- Improve the address resolution error messages to include a description the error generated by getaddrinfo.
- Fix an existing TODO where EWOULDBLOCK was not handled as a fatal error on Linux.
- Fix get_port so that it checks the return code from getsockname; this prevents it from using the socket data uninitialized if getsockname fails.
- Make a few of the string arguments to XmlRpcSocket const so that they're compatible with string literals.
Add system call mocks for socket functions, and use these to test the XmlRpcSocket class. Improve the address resolution error messages to include a description the error generated by getaddrinfo. Fix an existing TODO where EWOULDBLOCK was not handled as a fatal error on Linux. Fix get_port so that it checks the return code from getsockname; this prevents it from using the socket data uninitialized if getsockname fails. Make a few of the string arguments to XmlRpcSocket const so that they're compatible with string literals.
| ) | ||
| set_target_properties(test_socket PROPERTIES | ||
| LINK_FLAGS | ||
| "-Wl,--wrap=accept -Wl,--wrap=bind -Wl,--wrap=close -Wl,--wrap=connect -Wl,--wrap=getaddrinfo -Wl,--wrap=getsockname -Wl,--wrap=listen -Wl,--wrap=read -Wl,--wrap=setsockopt -Wl,--wrap=select -Wl,--wrap=socket -Wl,--wrap=write -Wl,--wrap=fcntl -Wl,--wrap=freeaddrinfo" |
There was a problem hiding this comment.
I would expect these arguments to fail on Windows?
| @@ -0,0 +1,1368 @@ | |||
|
|
|||
There was a problem hiding this comment.
Which license should I use here? I tend towards BSD but the rest of the library is licensed LGPL.
There was a problem hiding this comment.
Either one would be fine. Embedding BSD inside an LGPL project is not a problem, though you should update the package.xml to declare both. If everything else is LGPL it's simpler/"cleaner" to follow the exisiting projects license. So I guess I'd suggest defaulting to BSD if you're ok with it and it's more liberal.
There was a problem hiding this comment.
Ok; added an LGPL copyright block to all of the files that I've added in this and the previous PR.
|
|
||
| // The auto-generated mocks here don't use their parameters, so we disable | ||
| // that warning. | ||
| #pragma GCC diagnostic ignored "-Wunused-parameter" |
There was a problem hiding this comment.
Does this line need to be wrapped in a conditional in order to not fail on Windows?
There was a problem hiding this comment.
I'm not sure. If this is built using gcc on windows it's probably fine, but the MSVC compiler may reject it.
I don't have a windows development environment so I can't test this, and REP3 does not specify Windows support or a target compiler or toolchain for Windows either, so I don't think this support should be mandatory.
There was a problem hiding this comment.
Since some (brave) users do build ROS from source on Windows I would rather not introduce changes which are known ahead of time to break that. (Their might be regressions on Windows due to unforeseen side effects which we can't check without build infrastructure - we can't do anything about that atm.)
There was a problem hiding this comment.
Ok; I've wrapped both of these in feature guards so that we won't build the mocks on Windows and won't use the GCC diagnostic pragma unless __GNUC__ is defined.
I don't have a Windows machine and don't know how to set up the development environment on it, so I don't have any way to test this.
There was a problem hiding this comment.
Since e.g. clang also supports this I would suggest using #ifndef _WIN32 instead.
| ) | ||
| set_target_properties(test_socket PROPERTIES | ||
| LINK_FLAGS | ||
| "-Wl,--wrap=accept -Wl,--wrap=bind -Wl,--wrap=close -Wl,--wrap=connect -Wl,--wrap=getaddrinfo -Wl,--wrap=getsockname -Wl,--wrap=listen -Wl,--wrap=read -Wl,--wrap=setsockopt -Wl,--wrap=select -Wl,--wrap=socket -Wl,--wrap=write -Wl,--wrap=fcntl -Wl,--wrap=freeaddrinfo" |
There was a problem hiding this comment.
I would expect these arguments to fail on Windows?
| set_target_properties(test_socket PROPERTIES | ||
| LINK_FLAGS | ||
| "-Wl,--wrap=accept -Wl,--wrap=bind -Wl,--wrap=close -Wl,--wrap=connect -Wl,--wrap=getaddrinfo -Wl,--wrap=getsockname -Wl,--wrap=listen -Wl,--wrap=read -Wl,--wrap=setsockopt -Wl,--wrap=select -Wl,--wrap=socket -Wl,--wrap=write -Wl,--wrap=fcntl -Wl,--wrap=freeaddrinfo" | ||
| ) |
There was a problem hiding this comment.
The block within the if and endif should be indented one level.
|
@ros-pull-request-builder retest this please |
|
Thank you for the patch and for iterating on it. |
