DISPATCH-960 - Resolving service port when using http listener #294
Conversation
src/amqp.c
Outdated
| @@ -73,11 +76,30 @@ const char * const QD_AMQP_COND_FRAME_SIZE_TOO_SMALL = "amqp:frame-size-too-smal | |||
| const char * const QD_AMQP_PORT_STR = "5672"; | |||
| const char * const QD_AMQPS_PORT_STR = "5671"; | |||
|
|
|||
| const char * const QD_AMQP_DFLT_PROTO = "tcp"; | |||
|
|
|||
| int qd_port_int(const char* port_str) { | |||
| if (!strcmp(port_str, QD_AMQP_PORT_STR)) return QD_AMQP_PORT_INT; | |||
There was a problem hiding this comment.
I understand that this is not part of the pull request, but aren't these lines (with strcmp) redundant with the strtoul below?
There was a problem hiding this comment.
You're right. Maybe I should submit a new PR without the strcmp, as service name is now being resolved when strtoul returns 0. And also, using static values for amqp/amqps can also be a problem, in case someone changes the default values at /etc/services (no supposed to happen, but it is possible).
fgiorgetti
force-pushed
the
fgiorgetti-DISPATCH-960
branch
from
1fbfb14
to
b7f1e8b
Compare
|
Removed as suggested. It seems to be working just fine for me. |
fgiorgetti
force-pushed
the
fgiorgetti-DISPATCH-960
branch
from
b7f1e8b
to
d3baf98
Compare
There was a problem hiding this comment.
I tried it and it works. The only problem is that when I use nonsensical symbolic port name, such as asdfgh, I get running dispatch with listener on a random port, instead of an error.
After merging the doctest PR, I'd like to look into this more and see if I can fix anything, with the help of unittests.
| char *buf = calloc(buf_len, sizeof(char)); | ||
|
|
||
| // Service port is resolved | ||
| if ( !getservbyname_r(port_str, QD_AMQP_DFLT_PROTO, &serv_info, buf, buf_len, &serv_info_res) ) { |
There was a problem hiding this comment.
This should also check if serv_info_res == NULL.
| if ( !getservbyname_r(port_str, QD_AMQP_DFLT_PROTO, &serv_info, buf, buf_len, &serv_info_res) ) { | ||
| n = ntohs(serv_info.s_port); | ||
| } else { | ||
| n = -1; |
There was a problem hiding this comment.
n is unsigned, so assigning -1 to it is wrong
| char *buf = calloc(buf_len, sizeof(char)); | ||
|
|
||
| // Service port is resolved | ||
| if ( !getservbyname_r(port_str, QD_AMQP_DFLT_PROTO, &serv_info, buf, buf_len, &serv_info_res) ) { |
There was a problem hiding this comment.
Also, getservbyname_r is unavailable on macOS, there getservbyname is already reentrant.
|
@jiridanek Im confused by the updates here. You left comments suggesting there are things needing fixed with this, but then merged it without adjusting those bits yourself, and the JIRA is still just 'Open'. Was the merge an accident? |
|
@gemmellr It already does what it is supposed to do, so I thought it is ready to be merged. The improvement is there, macOS travis job is allowed to fail, and the behavior on invalid protocol name is not any worse than it was before. I did not close the Jira because I was planning to revisit the issue regarding my comments, either myself when there is the c_unittests pr merged, which should be soon now, or see if @fgiorgetti maybe fixes it in the meantime. edit: if what I've done is questionable, which I acknowledge it probably is, I'll learn for the future. Anyways, i wouldn't've spotted the macOS failure until I rebased and merged the PR, because the PR is older than the macOS job in Travis. |
|
Ok. If there are comments like 'x is wrong', 'should also do x', and 'gets a random port instead of error when doing ' (I was unclear if that was also true previously or not, but it seems like part of the same issue ither way) I'd personally leave it unmerged until addressed one way or anotehr by someone or other...or perhaps resolve the original JIRA and raise another to consider as seperate issues later if not important. Fair re: OSX failure and the job not existing (rebases can always be pushed to forks for testing first though hehe) |
| struct servent serv_info; | ||
| struct servent *serv_info_res; | ||
| int buf_len = 4096; | ||
| char *buf = calloc(buf_len, sizeof(char)); |
There was a problem hiding this comment.
This can be allocated on stack, so alloca or enum {buf_len = 4096 }; char buf[buf_len];
|
This PR is a resubmit of #284. |
As suggested by @ted-ross, I am re-submitting this PR again now using getservbyname_r which is multi-thread safe.
If you find any other possible issue, please let me know.