[prev in list] [next in list] [prev in thread] [next in thread]
List: illumos-developer
Subject: Re: [developer] [REVIEW] move internet address manipulation functions to libc
From: "Toomas Soome via illumos-developer" <developer () lists ! illumos ! org>
Date: 2018-07-25 9:56:54
Message-ID: C7906C4E-62F9-4DD3-9419-42DAE6AB945A () me ! com
[Download RAW message or body]
> On 25 Jul 2018, at 12:44, Yuri Pankov <yuripv@yuripv.net> wrote:
>
> Toomas Soome via illumos-developer wrote:
> > > On 25 Jul 2018, at 00:37, Yuri Pankov <yuripv@yuripv.net> wrote:
> > >
> > > Toomas Soome via illumos-developer wrote:
> > > > > On 22 Jul 2018, at 14:36, Yuri Pankov <yuripv@yuripv.net> wrote:
> > > > >
> > > > > webrev: http://cr.illumos.org/~webrev/yurip/ilinet/
> > > > >
> > > > > As I have looked at Rob's 9458 review, I have noticed that he's adding \
> > > > > -lnsl just to get the inet_ntop(), and that's unfortunate. So here's a \
> > > > > small subset of "get rid of libsocket and libnsl" change moving the inet_* \
> > > > > functions to libc, which must be there IMO.
> > > > LGTM. Any plans to move the rest of the libnsl?
> > >
> > > Toomas,
> > >
> > > I have updated this several times, for the build issues and according to \
> > > Robert's review, could you please take another look?
> > LGTM.
> > I want to complain some, however (sorry).
> > 1. especially with such large changes, it really would help to see the history of \
> > the changes. I know RB is not perfect but it still is a lot better than webrev. \
> > Or lets just have gerrit or phabricator up?:P
>
> Yeah, I didn't expect this to grow, sorry. And, yes, can we have gerrit or \
> phabricator, pretty please.
> > 2. this idea of having "full" change is producing those monster patches which \
> > actually should be implemented as series of smaller ones. There is no reason to \
> > have inet_ntop() and friends moved and the consumer makefiles cleaned up in the \
> > same patch. This only makes reviewing harder than it should be and it will make \
> > distro merges hard.
>
> Well, there *IS* a reason - Makefiles changes are *REQUIRED* to get clean build, I \
> doubt any advocate will accept RTI having mail_msg with 100+ lines of runtime \
> checks noise. It's not like I'm trying to force some unrelated changes in this one \
> simply being lazy to go through separate review :P
oh, I see, all good then.
rgds,
toomas
------------------------------------------
illumos: illumos-developer
Permalink: https://illumos.topicbox.com/groups/developer/T09d66a35c7325d85-M373289b7c0f0c4902bb23300
Delivery options: https://illumos.topicbox.com/groups/developer/subscription
[Attachment #3 (unknown)]
<html><html><html><head><meta content="text/html; charset=utf-8" \
http-equiv="Content-Type" /></head><body class="" style="word-wrap: break-word; \
-webkit-nbsp-mode: space; line-break: after-white-space;"><br class="" /><div><br \
class="" /><blockquote class="" type="cite"><div class="">On 25 Jul 2018, at 12:44, \
Yuri Pankov <<a class="" href="mailto:yuripv@yuripv.net">yuripv@yuripv.net</a>> \
wrote:</div><br class="Apple-interchange-newline" /><div class=""><span class="" \
style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; \
font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: \
normal; text-align: start; text-indent: 0px; text-transform: none; white-space: \
normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; \
float: none; display: inline !important;">Toomas Soome via illumos-developer \
wrote:</span><br class="" style="caret-color: rgb(0, 0, 0); font-family: Helvetica; \
font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; \
letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; \
white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; \
text-decoration: none;" /><blockquote class="" style="font-family: Helvetica; \
font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; \
letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; \
text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; \
-webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; text-decoration: \
none;" type="cite"><blockquote class="" type="cite">On 25 Jul 2018, at 00:37, Yuri \
Pankov <<a class="" href="mailto:yuripv@yuripv.net">yuripv@yuripv.net</a>> \
wrote:<br class="" /><br class="" />Toomas Soome via illumos-developer wrote:<br \
class="" /><blockquote class="" type="cite"><blockquote class="" type="cite">On 22 \
Jul 2018, at 14:36, Yuri Pankov <<a class="" \
href="mailto:yuripv@yuripv.net">yuripv@yuripv.net</a>> wrote:<br class="" /><br \
class="" />webrev: <a class="" \
href="http://cr.illumos.org/~webrev/yurip/ilinet/">http://cr.illumos.org/~webrev/yurip/ilinet/</a><br \
class="" /><br class="" />As I have looked at Rob's 9458 review, I have noticed \
that he's adding -lnsl just to get the inet_ntop(), and that's unfortunate. \
So here's a small subset of "get rid of libsocket and libnsl" \
change moving the inet_* functions to libc, which must be there IMO.<br class="" \
/><br class="" /></blockquote>LGTM. Any plans to move the rest of the libnsl?<br \
class="" /></blockquote><br class="" />Toomas,<br class="" /><br class="" />I have \
updated this several times, for the build issues and according to Robert's \
review, could you please take another look?<br class="" /><br class="" \
/></blockquote>LGTM.<br class="" />I want to complain some, however (sorry).<br \
class="" />1. especially with such large changes, it really would help to see the \
history of the changes. I know RB is not perfect but it still is a lot better than \
webrev. Or lets just have gerrit or phabricator up?:P<br class="" /></blockquote><br \
class="" style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; \
font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: \
normal; text-align: start; text-indent: 0px; text-transform: none; white-space: \
normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" \
/><span class="" style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: \
12px; font-style: normal; font-variant-caps: normal; font-weight: normal; \
letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; \
white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; \
text-decoration: none; float: none; display: inline !important;">Yeah, I didn't \
expect this to grow, sorry. And, yes, can we have gerrit or phabricator, pretty \
please.</span><br class="" style="caret-color: rgb(0, 0, 0); font-family: Helvetica; \
font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; \
letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; \
white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; \
text-decoration: none;" /><br class="" style="caret-color: rgb(0, 0, 0); font-family: \
Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; \
font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; \
text-transform: none; white-space: normal; word-spacing: 0px; \
-webkit-text-stroke-width: 0px; text-decoration: none;" /><blockquote class="" \
style="font-family: Helvetica; font-size: 12px; font-style: normal; \
font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: \
auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; \
widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; \
-webkit-text-stroke-width: 0px; text-decoration: none;" type="cite">2. this idea of \
having “full” change is producing those monster patches which actually \
should be implemented as series of smaller ones. There is no reason to have \
inet_ntop() and friends moved and the consumer makefiles cleaned up in the same \
patch. This only makes reviewing harder than it should be and it will make distro \
merges hard.<br class="" /></blockquote><br class="" style="caret-color: rgb(0, 0, \
0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: \
normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: \
0px; text-transform: none; white-space: normal; word-spacing: 0px; \
-webkit-text-stroke-width: 0px; text-decoration: none;" /><span class="" \
style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; \
font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: \
normal; text-align: start; text-indent: 0px; text-transform: none; white-space: \
normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; \
float: none; display: inline !important;">Well, there *IS* a reason - Makefiles \
changes are *REQUIRED* to get clean build, I doubt any advocate will accept RTI \
having mail_msg with 100+ lines of runtime checks noise. It's not like \
I'm trying to force some unrelated changes in this one simply being lazy to go \
through separate review :P</span><br class="" style="caret-color: rgb(0, 0, 0); \
font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: \
normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: \
0px; text-transform: none; white-space: normal; word-spacing: 0px; \
-webkit-text-stroke-width: 0px; text-decoration: none;" /><br class="" \
style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; \
font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: \
normal; text-align: start; text-indent: 0px; text-transform: none; white-space: \
normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" \
/></div></blockquote><br class="" /></div><div>oh, I see, all good \
then.</div><div><br class="" /></div><div>rgds,</div><div>toomas</div><br class="" \
/><div id="topicbox-footer" style="margin:10px 0 0;border-top:1px solid \
#ddd;border-color:rgba(0,0,0,.15);padding:7px 0;">
<strong><a href="https://illumos.topicbox.com/latest" \
style="color:inherit;text-decoration:none">illumos</a></strong> / illumos-developer \
/ see <a href="https://illumos.topicbox.com/groups/developer">discussions</a>
+
<a href="https://illumos.topicbox.com/groups/developer/members">participants</a>
+
<a href="https://illumos.topicbox.com/groups/developer/subscription">delivery \
options</a> <a href="https://illumos.topicbox.com/groups/developer/T09d66a35c7325d85-M373289b7c0f0c4902bb23300" \
style="float:right">Permalink</a> </div>
</body></html></html></html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic