[DTrace-devel] [PATCH 2/2] Support two representations for embedded IPv4 addresses

[Kris Van Hees]

On Fri, Sep 15, 2023 at 07:56:44PM -0400, Eugene Loh via DTrace-devel wrote:
> Okay, R-b on the two-patch series with the following comments.
> 
> In the commit message:
>         DTrace's inet_ntoa6() represents mapped and embedded IPv4 address
> address -> addresses

Thanks.

>         using the regular IPv4 representation, whereas inet_ntop() uses a
>         representation with a :: prefix followed by a regular IPv4 address.
> Well, not exactly right since sometimes the prefix is ::ffff:.

Yes, I can spell that out more.

>         The implementation uses an undocumented 2nd argument to inet_ntoa6()
>         to indicate whether prefix format should be generated or not.
> Just say we're adding an optional 2nd arg.

Sure.

> Also, in bpf/inet_ntoa6, is_ntop isn't a great name since those semantics
> are available to inet_ntoa6 as well.  Maybe rename in code           and in 
> comments to use_pref or something.
> 
> Also (I haven't actually tried the code), does the second arg have to be 1
> for "with prefix" semantics, or will the presence of any second arg do the
> job?  That is, in dt_cg_subr_inet_ntoa6, flag=arg->dn_list and then we test
> flag, which is non-NULL regardless of the value of the flag.

Hm yes, that is a good point.  RIght now, it gives the ntop behaviour when
any 2nd argument is supplied which is a bit confusing as API.  I'll change it
to be required to be an int and then determine if it is 0 or non-0.

I think I'll name the argument 'strict' since the prefix-form is the strict
representation of an IPv4-in-IPv6 address.

> And....
> 
> On 9/15/23 19:36, Kris Van Hees wrote:
> > On Fri, Sep 15, 2023 at 05:30:43PM -0400, Eugene Loh via DTrace-devel wrote:
> > > Yipes.  Once again, I didn't get the email in my inbox.  Yick.
> > > 
> > > Anyhow, the big question is whether we are changing the interface.  The
> > > commit message says the patch is introducing an undocumented arg.  Well, the
> > > test suite has a number of tests that check that not too many args are being
> > > passed to a function.  (Not every function is so tested, but arguably that's
> > > a short-coming of the test suite rather than an intentional policy of
> > > allowing too many args for all those untested functions.)  So, it seems to
> > > me that either we are changing the inet_ntoa6() interface (and should
> > > document that) or we are breaking a test (that we to date have declined to
> > > write).  Is it legal for a D user to write inet_ntop(af, addr, 0)?  How
> > > about inet_ntop(af, addr, 0, 0)?  How should D handle those two cases?  Or
> > > are we using an optional arg that a D user will not be able to use?
> > We are not changing the interface in a substantial way.  We are extending it
> > a tiny bit by allowing inet_ntoa6(addr, int).  If int is 0, the regular
> > inet_ntoa6() behaviour results (no :: prefix) whereas if int is not 0, then
> > we yield a result with :: prefix.
> > 
> > So, this is a completely backwards compatible change, and if anyone wants to
> > use the new feature, they certainly can.
> > 
> > I have no problem documenting it, but I also do not see that there is any
> > issue if this remains undocumented until documentation gets updated for other
> > things as well.
> 
> IMHO, No rush on doc'ing... we have a backlog anyhow.

Indeed.

> > I also do not mind adding tests that ensure that only two arguments can be
> > supplied, and that the proper behaviour results from not supplying this 2nd
> > argument, or from supplying 0 vs non-0 values.
> 
> Cool.  Yeah, slowly we can fill in those test gaps.
> 
> > I do not see any reason why we would prevent users from using this, nor is
> > that really something we can easily do.
> > 
> > > Incidentally,
> > > 
> > > +.Lipv4_2:
> > > +       /* Output IPv4 address prefixed by ::ffff: (if is_ntop is set). */
> > > +       ldxw    %r0, [%fp + -8]         /* restore is_ntop */
> > > +       jeq     %r0, 0, .Lipv4
> > > +       stb     [%r9 + 0], ':'
> > > +       stb     [%r9 + 1], ':'
> > > +       add     %r9, 2
> > > +       mov     %r1, -1
> > > +       mov     %r2, %r9
> > > +       call    write_hex16
> > > +       add     %r9, %r0
> > > +       stb     [%r9 + 0], ':'
> > > +       add     %r9, 1
> > > +       ja      .Lipv4
> > > +
> > >   .Lipv4:
> > > 
> > > No need for the ja since you fall through to that label anyhow. Also,
> > > instead of the "add %r9,..." instructions and everything in-between, how
> > > about simply
> > True, good point.
> > 
> > > stb [%r9+2], 'f'
> > > stb [%r9+3], 'f'
> > > stb [%r9+4], 'f'
> > > stb [%r9+5], 'f'
> > > stb [%r9+6], ':'
> > > add %r9, 7
> > > 
> > > Fewer instructions, easier to read.
> > Also on this.
> > 
> > > Maybe this weird difference between ntoa6 and ntop is just not worth solving
> > > (until we run out of other things to do)?
> > That becomes a bit irrelevant given that it is already implemented...
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel