Обсуждение: Re: [PATCH] Improve code coverage of network address functions

On Thu, Oct 31, 2024 at 9:30 AM Aleksander Alekseev
<aleksander@timescale.com> wrote:
> Recently I played with lcov [1]. In the process it was discovered that
> the following functions are not executed by our tests:
>
> - abbrev(inet)
> - set_masklen(cidr,int4)
> - netmask(inet)
> - hostmask(inet)

The new tests for the first four look reasonable to me.

> - inet_client_addr()
> - inet_client_port()
> - inet_server_addr()
> - inet_server_port()

These may be more controversial. (Personally, I'm -0.5.) I agree that
making sure they exist/don't crash is a benefit, but to use my machine
as an example, the interesting code with crash potential in
inet_server_addr() still isn't exercised during `meson test`. (A test
driver in src/test/modules, which could pull the socket information to
verify it, might be a better way to go.)

Thanks!
--Jacob

Jacob Champion <jacob.champion@enterprisedb.com> writes:
> On Thu, Oct 31, 2024 at 9:30 AM Aleksander Alekseev
> <aleksander@timescale.com> wrote:
>> Recently I played with lcov [1]. In the process it was discovered that
>> the following functions are not executed by our tests:
>> 
>> - abbrev(inet)
>> - set_masklen(cidr,int4)
>> - netmask(inet)
>> - hostmask(inet)

> The new tests for the first four look reasonable to me.

Agreed.

>> - inet_client_addr()
>> - inet_client_port()
>> - inet_server_addr()
>> - inet_server_port()

> These may be more controversial. (Personally, I'm -0.5.) I agree that
> making sure they exist/don't crash is a benefit, but to use my machine
> as an example, the interesting code with crash potential in
> inet_server_addr() still isn't exercised during `meson test`.

Yeah, I think that on normal testing setups where the test client is
connecting via a Unix socket, we are not going to get any useful
coverage this way.  It used to be that we might get coverage on
Windows builds, but even that isn't true anymore IIUC.  So I'm
inclined to leave this out as not worth the cycles.

> (A test
> driver in src/test/modules, which could pull the socket information to
> verify it, might be a better way to go.)

To do anything interesting, the test would have to make the server
open a TCP port, which would be rightly seen as a security hazard.
So it'd have to be confined to a not-run-by-default test case.

Maybe we could add this to the existing src/test/ssl/ tests,
which already deal with that hazard?

            regards, tom lane

On Mon, Jan 20, 2025 at 11:35 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> To do anything interesting, the test would have to make the server
> open a TCP port, which would be rightly seen as a security hazard.
> So it'd have to be confined to a not-run-by-default test case.

Yeah.

> Maybe we could add this to the existing src/test/ssl/ tests,
> which already deal with that hazard?

That seems okay in the short term. (But it certainly highlights our
lack of a "PG_TEST_EXTRA=loopback-is-fine" mode...)

--Jacob

Jacob Champion <jacob.champion@enterprisedb.com> writes:
> On Mon, Jan 20, 2025 at 11:35 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Maybe we could add this to the existing src/test/ssl/ tests,
>> which already deal with that hazard?

> That seems okay in the short term. (But it certainly highlights our
> lack of a "PG_TEST_EXTRA=loopback-is-fine" mode...)

Part of my thought here is that these functions are not worth their
very own TAP test, with all the overhead that implies of firing up
a new database instance.  So I was looking for something we could
fold them into.  I agree the SSL tests are focused on something
else, but still...

            regards, tom lane

On Mon, Jan 20, 2025 at 12:27 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Part of my thought here is that these functions are not worth their
> very own TAP test, with all the overhead that implies of firing up
> a new database instance.  So I was looking for something we could
> fold them into.

By themselves, yeah, probably not. Having a separate place for a
TCP-focused suite might decrease the activation energy for testing
other network features, though.

In any case, Aleksander, I don't mean to sign you up for all that; the
`ssl` suite also seems good enough to me if you're interested in
pursuing that side of the patch further.

Thanks!
--Jacob