[Swan-dev] libswan/addrtot.c: simplify so scan-build can understand it

[Andrew Cagney]

On Tue, 19 Feb 2019 at 12:36, D. Hugh Redelmeier <hugh at mimosa.com> wrote:
>
> | From: Andrew Cagney <andrew.cagney at gmail.com>
>
> | On Tue, 19 Feb 2019 at 00:33, D. Hugh Redelmeier
> | <hugh at vault.libreswan.fi> wrote:
> | >
> | > New commits:
> | > commit b9cd04d93e96ed04baf3a5d25eb50e1ea2d8370f
> | > Author: D. Hugh Redelmeier <hugh at mimosa.com>
> | > Date:   Tue Feb 19 00:21:22 2019 -0500
> | >
> | >     libswan/addrtot.c: simplify so scan-build can understand it
> |
> | BTW, what is scan-build?  A search suggests something in LLVM?
>
> (Darn, more hidden lore.  Something I rail against and yet am guilty of.)
>
> scan-build is a very convenient lint-like thing that uses clang (LLVM's C
> compiler).
>
> scan-build is a perl script that somehow gets the job done, but I haven't
> checked how.  It does have a man page.  Kind of an obscure name.
>
> How I use it:
>
>         make clean
>         scan-build make base
>
> The first line is superstitious: I want to make sure clang and gcc don't
> meet awkwardly, and I want to make sure all of Libreswan's code get's
> built.  Well, all that can be built on my machine, which doesn't run
> libreswan.
>
> How should we capture this lore?  The natural place would be the
> makefile, but then we get awfully meta.

Like the valgrind tests, add it to the testsuite?  Just wondering.

> | >     - the result is also easier for humans to read too.
> | >
> | >     - Since 30444640cd234d2e1d1e1f29bd565fda661c7b94 there are two copies
> | >       of addrtot.c and this only updates one!  The other is frozen.
> | >       Too bad that history will be lost when the other is deleted.
> | >
> | >     - There is a program to test this code: testing/ipcheck/.  "make base"
> | >       compiles the test program into OBJ.*/testing/ipcheck/ipcheck.
> | >       Unfortunately, nothing in the test suite runs it.  Run the program
> | >       manually and compare the output with (non-existent!) reference
> | >       output.
> |
> | Umm. "ipcheck" does include reference output (if there's a difference
> | it sends it to STDERR, and reports it as a failure count / status
> | code).
>
> I saw no evidence that our test suite runs it.

I wasn't suggesting that it was part of automated testing (I've not
been in any hurry to add it as, like for enumcheck, I suspect I'll be
the only one maintaining it).

> When I manually ran it, it just poured out logging of what looked to be
> correct tests.
>
> I tested by running it without my change and with my change and compared
> the output.  It was identical, so I declared victory.
>
> Where is the reference output?  I didn't see it in libreswan/testing/ipcheck/

For instance:

        static const struct test tests[] = {
                /* basic */
                { "127.0.0.1", 0, "127.0.0.1", },
                { "1:2::7:8", 0, "1:2:0:0:0:0:7:8", },
                /* different sepc */
                { "127.0.0.1", '/', "127/0/0/1", },
                { "1:2::7:8", '/', "1/2/0/0/0/0/7/8", },
                /* buffer overflow */
                { "255.255.255.255", 0, "255.255.255.255", },
                { "1111:2222:3333:4444:5555:6666:7777:8888", 0,
"1111:2222:3333:4444:5555:6666:7777:8888", },
        };


> BTW: unit testing, like this, is great!  So much faster and clearer.
>
> |  However, it is testing
> | str_address_{raw,cooked,sensitive,reversed}() and not addrtot() (to
> | your point "the result is also easier for humans to read too" it was
> | easier than trying to fix edge cases in addrtot()).
>
> Yikes!  Are you saying that I didn't actually test my changes?

Presumably you ran the testsuite.

Anyway,  testing.libreswan.org suggests no regressions.

> Henry build a bunch of unit tests into the library.  Generally in the
> source files themselves.  I guess they've been removed.  No history was
> available for this source file.

(Yea, embedding tests in the .c file was a common trick back then.)

The tests were derived from Henry's code vis:

/* ip_address tests, for libreswan
 *
 * Copyright (C) 2000  Henry Spencer.
 * Copyright (C) 2012 Paul Wouters <paul at libreswan.org>
 * Copyright (C) 2018 Andrew Cagney

I see some of my commits included "Revive tests and add them to
ipcheck." but not all.

> Good luck to everyone who encounters a bug in this untested change!
> I'm not around today.  If bitten, revert.
>
> |  Anyway, like for
> | subnettot() - 6a7bcbe5c1823cbc1df70d0a32dd3e5d36992a47 - I suspect
> | things are reaching the point where addrtot() can reduced to a
> | str_address_raw() wrapper; however not right now.
>
> I have no opinion at the moment (i.e. I haven't researched this).
>
> I was driven by a persistent scan-build warning (that was wrong).  So:
> bottom up; not driven by a broad vision.  Broad visions are good.
> _______________________________________________
> Swan-dev mailing list
> Swan-dev at lists.libreswan.org
> https://lists.libreswan.org/mailman/listinfo/swan-dev