> On Jan. 8, 2015, 4:22 p.m., Ben Mahler wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp, lines 448-460 > > <https://reviews.apache.org/r/29288/diff/8/?file=813329#file813329line448> > > > > I didn't take a look at the rest of this, sorry for the drive-by > > comment. A very quick stylistic note: > > > > This is exposing a Try<bool> to surface the case of an unknown family > > type. However, in other places in the code you're explicitly ABORTing for > > unknown family types. > > > > That tells me that you should guard against the construction of these > > invalid Address objects in the first place, rather than allowing them and > > exposing the Error later during a call to isLoopback. > > > > Make sense? > > Evelina Dumitrescu wrote: > I initially done that. I have made the family field intentionally private > and also the signatures of the constructors not to allow to specify the > family type as parameter. I've been advised to do so if someone might > accidentally change the code later. Probably it's better just to add a > Note/Warning about this. From might point of view, the idea of continuously > checking the family type makes the code a lot more complicated. > > Evelina Dumitrescu wrote: > Or we can use ABORT instead of Error to avoid the Try.
the above feedback was mine. it is currently substantially guarded against invalid families, but changes to this code (or badly written code that stomps memory, which we hopefully never have) might cause an invalid family. As such, I think being defensive is good. As long as we're consistent, I don't mind if it's ABORT or Error. In general, avoiding ABORT is good, but in this case it would be programmer error rather than user error, so may be a good use-case for ABORT. - Dominic ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29288/#review67340 ----------------------------------------------------------- On Jan. 8, 2015, 9:35 a.m., Evelina Dumitrescu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29288/ > ----------------------------------------------------------- > > (Updated Jan. 8, 2015, 9:35 a.m.) > > > Review request for mesos, Benjamin Hindman, Dominic Hamon, Joris Van > Remoortere, and Niklas Nielsen. > > > Bugs: MESOS-1919 > https://issues.apache.org/jira/browse/MESOS-1919 > > > Repository: mesos-git > > > Description > ------- > > Created the inner class Address encapsulated inside the IP class. > The class uses a union with the in_addr and in6_addr fields. > I considered that the The MasterInfo protobuffers should have both an ipv4 > and an ipv6 field. > I intend to use the same Classifiers, addition, removal and update of > container filters, but write different encode/decode functions for IPv4/ICMP > and IPv6/ICMPv6 because the processing of the protocol headers differ. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp > 80578e2207d5436a4414f56417a0fee06ca9a7e9 > 3rdparty/libprocess/3rdparty/stout/tests/net_tests.cpp > 425132e5d7c3770be4a5a39feea5a2f22179b871 > > Diff: https://reviews.apache.org/r/29288/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Evelina Dumitrescu > >
