> On Jan. 9, 2015, 12:22 a.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. > > Dominic Hamon wrote: > 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.
Yes being defensive by checking this invariant is a good thing. I'm just saying let's use ABORT to enforce it, rather than making the caller deal with it by exposing a Try<>. - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29288/#review67340 ----------------------------------------------------------- On Jan. 15, 2015, 7:18 p.m., Evelina Dumitrescu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29288/ > ----------------------------------------------------------- > > (Updated Jan. 15, 2015, 7:18 p.m.) > > > Review request for mesos, Benjamin Hindman, Dominic Hamon, Jie Yu, 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 > >
