[DTrace-devel] [PATCH 2/2] Support two representations for embedded IPv4 addresses
Eugene Loh
eugene.loh at oracle.com
Fri Sep 15 23:56:44 UTC 2023
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
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:.
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.
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.
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.
> 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...
More information about the DTrace-devel
mailing list