-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Resolve hostname as IP #13
base: hydro-devel
Are you sure you want to change the base?
Conversation
if ((he = gethostbyname(ip_address_.c_str())) != NULL) | ||
{ | ||
struct in_addr **addr_list | ||
= (struct in_addr **)he->h_addr_list; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you have to cast it to an struct in_addr
rather than leave it as the normal char **
that it normally returns? I'm referencing this document:
http://www.gnu.org/software/libc/manual/html_node/Host-Names.html
Also, if you must cast it, please use the c++ style static_cast
.
The patch seems reasonable to me, other than my minor comments. |
e00a239
to
6705c07
Compare
Updated about in_addr issue, as described in here |
I see, makes sense. I'll let @trainman419 merge. |
This should probably include some kind of error handling and a useful message if the hostname lookup fails. As currently implemented, if the hostname lookup fails, it just passes the hostname to the prosilica library. Re-using the |
I wrote a simple testcode of gethostbyname and result is:
Legal IP should be successfully resolved as the same IP. And it can detect hostname (and malformed IP address) by return value of gethostbyname. |
I know how gethostbyname works. I know that it will accept an IP address and produce the correct result. I'm worried that users won't realize that they can supply a hostname in the ip_address parameter, because the parameter name implies that the node is only expecting an IP address. This is why I think there should be a new, explicit hostname parameter, so that future users know that they're allowed to specify their camera by hostname. You should include explicit error-handling for when the gethostbyname call fails, with a concise and readable message indicating that it tired to do a hostname lookup, and that the hostname lookup failed. |
I see. Let me summarize;
|
6705c07
to
c314035
Compare
c314035
to
2279972
Compare
Updated
|
prosilica::Camera
only read ip address. Need to resolve hostname as IP to pass it toprosilica::Camera