Define Ipv6Instance, implementation and tests. Not yet used except in… #520

jamessynge · 2017-02-28T21:49:04Z

… tests. Partially addressing #351

… tests.

mattklein123 · 2017-02-28T22:38:15Z

include/envoy/network/address.h

+  /**
+   * @return the 16-byte IPv6 address in network byte order.
+   */
+  virtual std::string address() const PURE;


std::array<uint8_t, 16> ?

mattklein123 · 2017-02-28T22:40:52Z

source/common/network/address_impl.cc

+
+std::string Ipv6Instance::Ipv6Helper::makeFriendlyAddress() const {
+  char str[INET6_ADDRSTRLEN];
+  if (nullptr == inet_ntop(AF_INET6, &address_.sin6_addr, str, INET6_ADDRSTRLEN)) {


I don't think this can actually happen? I would just ASSERT that it returns non-null if you want (might want to fix IPv4 code above to do the same thing).

mattklein123 · 2017-02-28T22:44:08Z

source/common/network/address_impl.cc

+  ip_.ipv6_.address_.sin6_family = AF_INET;
+  ip_.ipv6_.address_.sin6_port = htons(port);
+
+  // TODO: Consider pulling this out of ctor flow and into a static method used to construct


I know there is a larger offline conversation happening around exceptions right now. If possible I would rather not change course and/or code style (and pollute this PR) until we finish that offline conversation. Assuming we are using exceptions, this is the right way to structure this code (and throwing exceptions out of constructors is fine). If we are not using exceptions and are going to actively remove them, a whole lot of code has to be changed, and it seems like we should do it at that point if that actually transpires.

cc @tschroed

Comments removed.

@jamessynge did you forget to push a commit somewhere? You mention addressing comments in a commit below but I don't see a change here.

* change return type of Ipv6::address to std::array<uint8_t, 16> * Replace one check with an ASSERT * Remove some comments

jamessynge · 2017-03-02T22:08:42Z

Are you looking for any other changes?

mattklein123 · 2017-03-02T22:11:43Z

@jamessynge the TODO for comments is still there as well as the error checking for inet_ntop. It sounds like you fixed that but I don't see the delta. Did you forget to push something?

jamessynge · 2017-03-02T23:45:46Z

How odd! I'll double check when back in the office. On Mar 2, 2017 5:11 PM, "Matt Klein" <notifications@github.com> wrote: @jamessynge <https://github.com/jamessynge> the TODO for comments is still there as well as the error checking for inet_ntop. It sounds like you fixed that but I don't see the delta. Did you forget to push something? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#520 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABYmU1_XMXW2JtschGpiRYGq0Kl1Ebz9ks5rhz6hgaJpZM4MO7fu> .

* Replace one if check with an ASSERT. * Remove some comments.

jamessynge · 2017-03-03T19:49:32Z

Apparently I suck at git (still new to the tool), so somehow I lost some changes that I'm pretty sure I made. Oh well, they were easy to repeat. PTAL

mattklein123

looks good, tiny nit

mattklein123 · 2017-03-03T21:24:50Z

source/common/network/address_impl.cc

+  memset(&ip_.ipv6_.address_, 0, sizeof(ip_.ipv6_.address_));
+  ip_.ipv6_.address_.sin6_family = AF_INET;
+  ip_.ipv6_.address_.sin6_port = htons(port);
+  if (address != "") {


nit: !address.empty()

Done (if I didn't mess up the git foo again).

mattklein123 · 2017-03-04T18:24:57Z

source/common/network/address_impl.cc

+}
+
+int Ipv6Instance::socket(SocketType type) const {
+  return ::socket(AF_INET, flagsFromSocketType(type), 0);


@jamessynge sorry I didn't notice this before, but this should be AF_INET6. (I'm sure you would have noticed that when you start using this). In follow ups can you make sure that there is full coverage for the new code? There is a bunch missing now.

jamessynge · 2017-03-04T18:53:54Z

Will do, and thanks for noticing.

…

On Mar 4, 2017 1:25 PM, "Matt Klein" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In source/common/network/address_impl.cc <#520 (comment)>: > +} + +Ipv6Instance::Ipv6Instance(uint32_t port) : Ipv6Instance("", port) {} + +int Ipv6Instance::bind(int fd) const { + return ::bind(fd, reinterpret_cast<const sockaddr*>(&ip_.ipv6_.address_), + sizeof(ip_.ipv6_.address_)); +} + +int Ipv6Instance::connect(int fd) const { + return ::connect(fd, reinterpret_cast<const sockaddr*>(&ip_.ipv6_.address_), + sizeof(ip_.ipv6_.address_)); +} + +int Ipv6Instance::socket(SocketType type) const { + return ::socket(AF_INET, flagsFromSocketType(type), 0); @jamessynge <https://github.com/jamessynge> sorry I didn't notice this before, but this should be AF_INET6. (I'm sure you would have noticed that when you start using this). In follow ups can you make sure that there is full coverage for the new code? There is a bunch missing now. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#520 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABYmUzjbobVcuvYTPTKJMP6RNGWxG3nqks5riax8gaJpZM4MO7fu> .

envoyproxy#520)

* Add integration tests. * add fault_inject_test * format * Update comment. * fix format.

Description: after a bit more discussion we have decided to go ahead and make buffering configurable. This PR revives #456 Risk Level: med - adds new surface area to the API Testing: local apps and CI Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

Define Ipv6Instance, implementation and tests. Not yet used except in…

13cdaf0

… tests.

mattklein123 reviewed Feb 28, 2017

View reviewed changes

mattklein123 closed this Mar 1, 2017

mattklein123 reopened this Mar 1, 2017

jamessynge added 2 commits March 1, 2017 10:44

Address feedback on PR envoyproxy#520:

279b3fb

* change return type of Ipv6::address to std::array<uint8_t, 16> * Replace one check with an ASSERT * Remove some comments

clang-format

a67a4e9

Recover lost commit:

ce8b0fb

* Replace one if check with an ASSERT. * Remove some comments.

mattklein123 reviewed Mar 3, 2017

View reviewed changes

Use empty() instead of =="" for checking if string is empty.

77aa695

mattklein123 approved these changes Mar 3, 2017

View reviewed changes

mattklein123 merged commit c932d60 into envoyproxy:master Mar 4, 2017

mattklein123 reviewed Mar 4, 2017

View reviewed changes

tschroed pushed a commit to tschroed/envoy that referenced this pull request Apr 10, 2017

Define Ipv6Instance, implementation and tests. Not yet used except in… (

6af8df1

envoyproxy#520)

rshriram pushed a commit to rshriram/envoy that referenced this pull request Oct 30, 2018

Add some integration tests (envoyproxy#520)

5d54418

* Add integration tests. * add fault_inject_test * format * Update comment. * fix format.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define Ipv6Instance, implementation and tests. Not yet used except in… #520

Define Ipv6Instance, implementation and tests. Not yet used except in… #520

jamessynge commented Feb 28, 2017

mattklein123 Feb 28, 2017

jamessynge Mar 1, 2017

mattklein123 Feb 28, 2017

jamessynge Mar 1, 2017

mattklein123 Feb 28, 2017

jamessynge Mar 1, 2017

mattklein123 Mar 2, 2017

jamessynge commented Mar 2, 2017

mattklein123 commented Mar 2, 2017

jamessynge commented Mar 2, 2017 via email

jamessynge commented Mar 3, 2017

mattklein123 left a comment

mattklein123 Mar 3, 2017

jamessynge Mar 3, 2017

mattklein123 Mar 4, 2017

jamessynge commented Mar 4, 2017 via email

Define Ipv6Instance, implementation and tests. Not yet used except in… #520

Define Ipv6Instance, implementation and tests. Not yet used except in… #520

Conversation

jamessynge commented Feb 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamessynge commented Mar 2, 2017

mattklein123 commented Mar 2, 2017

jamessynge commented Mar 2, 2017 via email

jamessynge commented Mar 3, 2017

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamessynge commented Mar 4, 2017 via email