----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49724/#review141082 -----------------------------------------------------------
Basic idea make good sense to me. Details need fixing. The lack of the hint AI_PASSIVE is of course not your code, but it's still wrong. I'm not sure I like the look of some of the resulting URIs though - I'm not sure they are all unambiguous either, so the parser probably needs to be more careful. proton-c/bindings/cpp/include/proton/url.hpp (line 56) <https://reviews.apache.org/r/49724/#comment206469> You've documented the behaviour when defaults=true I think not when defaults=false. proton-c/bindings/cpp/include/proton/url.hpp (line 86) <https://reviews.apache.org/r/49724/#comment206471> Given the new comment block do you really want to keep the defaults boolean in the constructor? proton-c/src/posix/io.c (line 130) <https://reviews.apache.org/r/49724/#comment206475> You really need the AI_PASSIVE hint set here? Given your other comment you may also want AI_V4MAPPED proton-c/src/posix/io.c (line 132) <https://reviews.apache.org/r/49724/#comment206474> I don't think listening to a null port makes any sense. The man page for getaddrinfo notes that only one of node or service can be null. service being null is only useful if you have an actual port already and you fill that in yourself later, in the context of this code it makes no sense to allow service to be NULL. proton-c/src/posix/io.c (line 176) <https://reviews.apache.org/r/49724/#comment206479> NULL node = loopback address - fine NULL service again makes no sense. proton-c/src/url.c (line 105) <https://reviews.apache.org/r/49724/#comment206473> No it doesn't: you can't simultaneously bind to ::0 and 0.0.0.0. That requires 2 sockets. All you can do is bind to ::0 with IPv4 connections mapped to IPv6. This is not the same and different OSes default to whether mapping is on by default or not and even whether it is allowed. - Andrew Stitcher On July 6, 2016, 9:33 p.m., Alan Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49724/ > ----------------------------------------------------------- > > (Updated July 6, 2016, 9:33 p.m.) > > > Review request for qpid, Andrew Stitcher, Cliff Jansen, Gordon Sim, and > Kenneth Giusti. > > > Bugs: PROTON-1251 > https://issues.apache.org/jira/browse/PROTON-1251 > > > Repository: qpid-proton-git > > > Description > ------- > > Made all components of a pn_url optional, an empty string is a legal URL. > pn_connect and pn_listen treat an empty string as a NULL when calling > getaddrinfo. > > This means that an omitted hostname is treated as the OS default address, so > listening on an empty URL listens on both IPv4 and IPv6 addresses, connecting > to an empty URL connects using the OS default IPv4/v6. > > Partial URLs that lack a host name are treated consistently ":8888" means > listen (on v4 and v6) or connect (on the OS default protocol) to port 8888 on > the local host. > > > Diffs > ----- > > examples/cpp/README.dox be088bea01ea7d63fa5b09d523e04001568cf39d > examples/cpp/broker.cpp bb3aefb78ded515c16b3a4fda05a648e102084d9 > examples/cpp/client.cpp dd22ccdaa1e5ac76538df87bcfa3db9a6a15ffda > examples/cpp/connection_options.cpp > f9935a89384c156ac1cb902a8b0e965ed405e3ee > examples/cpp/direct_recv.cpp edbc8b62892127c53b9b839a384a412e4556d1cb > examples/cpp/direct_send.cpp d8427b72e259e33f8007189d1de1efc44e7c9c0e > examples/cpp/example_test.py bb7c48debc09d2f28f945f2b96597f44ce6d4739 > examples/cpp/flow_control.cpp 99f832d5117bd228ee7b433bebc774186aa3f8b2 > examples/cpp/helloworld.cpp fa95af0b3b4d650b02ec84ac7e35ef6d237c8ed5 > examples/cpp/helloworld_direct.cpp 5421e62d4b623771693d0c080985f0b5cd9f3cd7 > examples/cpp/mt/broker.cpp 29fe97fa8b58c343db44811b06b8c7a8f0b8f3cd > examples/cpp/queue_browser.cpp 649716b216bb89a4e9940de704c73a9e6d8687f0 > examples/cpp/scheduled_send.cpp 280c7374d77bfbba86e711c99d961acbe6376e0a > examples/cpp/scheduled_send_03.cpp 5d3aaac7e06fb84856b9ea6ee3529afa077c9b99 > examples/cpp/selected_recv.cpp 2c1a6b6d058f31a6b1094a37836411763707d759 > examples/cpp/server.cpp ec24f9f812b92f13b11c10a5b5f5e20c87dc0e9c > examples/cpp/server_direct.cpp 220934f10f5b2b05931fd93eb78eb89d420f3eeb > examples/cpp/simple_recv.cpp 76219416c0774195a088958f922890260ef89bf5 > examples/cpp/simple_send.cpp 3ce72b40463175400e3db8d5d8ea46b9acecbbb3 > examples/cpp/ssl.cpp 67394b55fc81eb6bb56a6cbdb7c037e5330cae71 > examples/cpp/ssl_client_cert.cpp 42b659353e5343e82f569457c022fb7b662676ff > examples/cpp/tutorial.dox 56345a1e4cd8675186b69d7f76235d75ff16cfb1 > examples/go/example_test.go 6de309e2e50da5459a737d27dcc28fc277e52b56 > proton-c/bindings/cpp/include/proton/url.hpp > 0fabe1e170ecae76595562b13d23e7841b47b67b > proton-c/bindings/cpp/src/url.cpp 34616bdf89646a875eecca21123a1a44b33df7a1 > proton-c/bindings/go/src/qpid.apache.org/amqp/url_test.go > 99b656dac510a894ee5354d1003cb1120afe0f25 > proton-c/src/posix/io.c c0dc425c02061b08245d48ceb7a4a37cc9ba2c82 > proton-c/src/url.c 566e91ed1e8186268b2d76b600b2ff93e84d133e > proton-c/src/windows/io.c 4a87fd2b03a2fd49d1aea343cb331d495a6eb341 > > Diff: https://reviews.apache.org/r/49724/diff/ > > > Testing > ------- > > > Thanks, > > Alan Conway > >
