[PATCH] Improve code coverage of network address functions
Hi hackers,
Recently I played with lcov [1]/messages/by-id/CAJ7c6TPYPF93+yWi=ThKiOsnhqLpeTmctMrJWz3xRQobGSY6BA@mail.gmail.com. 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)
- inet_client_addr()
- inet_client_port()
- inet_server_addr()
- inet_server_port()
The proposed patch fixes this. For the last four functions the return
values are not checked, only the fact that the functions are callable
and don't crash.
This improves code coverage of src/backend/utils/adt/network.c from
69.8% to 80.1%.
[1]: /messages/by-id/CAJ7c6TPYPF93+yWi=ThKiOsnhqLpeTmctMrJWz3xRQobGSY6BA@mail.gmail.com
--
Best regards,
Aleksander Alekseev
Attachments:
v1-0001-Improve-code-coverage-of-network-address-function.patchapplication/x-patch; name=v1-0001-Improve-code-coverage-of-network-address-function.patchDownload
From e3a19689a335bf5c0c6aecd19e6fe3d17d456b2b Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Thu, 31 Oct 2024 17:54:56 +0300
Subject: [PATCH v1] Improve code coverage of network address functions
The following functions were not covered by tests:
- abbrev(inet)
- set_masklen(cidr,int4)
- netmask(inet)
- hostmask(inet)
- inet_client_addr()
- inet_client_port()
- inet_server_addr()
- inet_server_port()
Correct this.
Aleksander Alekseev, reviewed by TODO FIXME
Discussion: TODO FIXME
---
src/test/regress/expected/inet.out | 104 +++++++++++++++++++++++++++++
src/test/regress/sql/inet.sql | 14 ++++
2 files changed, 118 insertions(+)
diff --git a/src/test/regress/expected/inet.out b/src/test/regress/expected/inet.out
index b6895d9ced..c903c6c93b 100644
--- a/src/test/regress/expected/inet.out
+++ b/src/test/regress/expected/inet.out
@@ -190,6 +190,72 @@ SELECT c AS cidr, masklen(c) AS "masklen(cidr)",
10.0.0.0/8 | 8 | 9.1.2.3/8 | 8
(4 rows)
+SELECT i AS inet, abbrev(i) AS "abbrev(inet)" FROM INET_TBL;
+ inet | abbrev(inet)
+------------------+------------------
+ 192.168.1.226/24 | 192.168.1.226/24
+ 192.168.1.226 | 192.168.1.226
+ 192.168.1.0/24 | 192.168.1.0/24
+ 192.168.1.0/25 | 192.168.1.0/25
+ 192.168.1.255/24 | 192.168.1.255/24
+ 192.168.1.255/25 | 192.168.1.255/25
+ 10.1.2.3/8 | 10.1.2.3/8
+ 10.1.2.3/8 | 10.1.2.3/8
+ 10.1.2.3 | 10.1.2.3
+ 10.1.2.3/24 | 10.1.2.3/24
+ 10.1.2.3/16 | 10.1.2.3/16
+ 10.1.2.3/8 | 10.1.2.3/8
+ 11.1.2.3/8 | 11.1.2.3/8
+ 9.1.2.3/8 | 9.1.2.3/8
+ 10:23::f1/64 | 10:23::f1/64
+ 10:23::ffff | 10:23::ffff
+ ::4.3.2.1/24 | ::4.3.2.1/24
+(17 rows)
+
+SELECT i AS inet, netmask(i) AS "netmask(inet)" FROM INET_TBL;
+ inet | netmask(inet)
+------------------+-----------------------------------------
+ 192.168.1.226/24 | 255.255.255.0
+ 192.168.1.226 | 255.255.255.255
+ 192.168.1.0/24 | 255.255.255.0
+ 192.168.1.0/25 | 255.255.255.128
+ 192.168.1.255/24 | 255.255.255.0
+ 192.168.1.255/25 | 255.255.255.128
+ 10.1.2.3/8 | 255.0.0.0
+ 10.1.2.3/8 | 255.0.0.0
+ 10.1.2.3 | 255.255.255.255
+ 10.1.2.3/24 | 255.255.255.0
+ 10.1.2.3/16 | 255.255.0.0
+ 10.1.2.3/8 | 255.0.0.0
+ 11.1.2.3/8 | 255.0.0.0
+ 9.1.2.3/8 | 255.0.0.0
+ 10:23::f1/64 | ffff:ffff:ffff:ffff::
+ 10:23::ffff | ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff
+ ::4.3.2.1/24 | ffff:ff00::
+(17 rows)
+
+SELECT i AS inet, hostmask(i) AS "hostmask(inet)" FROM INET_TBL;
+ inet | hostmask(inet)
+------------------+------------------------------------
+ 192.168.1.226/24 | 0.0.0.255
+ 192.168.1.226 | 0.0.0.0
+ 192.168.1.0/24 | 0.0.0.255
+ 192.168.1.0/25 | 0.0.0.127
+ 192.168.1.255/24 | 0.0.0.255
+ 192.168.1.255/25 | 0.0.0.127
+ 10.1.2.3/8 | 0.255.255.255
+ 10.1.2.3/8 | 0.255.255.255
+ 10.1.2.3 | 0.0.0.0
+ 10.1.2.3/24 | 0.0.0.255
+ 10.1.2.3/16 | 0.0.255.255
+ 10.1.2.3/8 | 0.255.255.255
+ 11.1.2.3/8 | 0.255.255.255
+ 9.1.2.3/8 | 0.255.255.255
+ 10:23::f1/64 | ::ffff:ffff:ffff:ffff
+ 10:23::ffff | ::
+ ::4.3.2.1/24 | 0:ff:ffff:ffff:ffff:ffff:ffff:ffff
+(17 rows)
+
SELECT c AS cidr, i AS inet FROM INET_TBL
WHERE c = i;
cidr | inet
@@ -261,6 +327,44 @@ SELECT set_masklen(inet(text(i)), 24) FROM INET_TBL;
::4.3.2.1/24
(17 rows)
+SELECT set_masklen(cidr(text(c)), 24) FROM INET_TBL;
+ set_masklen
+----------------
+ 192.168.1.0/24
+ 192.168.1.0/24
+ 192.168.1.0/24
+ 192.168.1.0/24
+ 192.168.1.0/24
+ 192.168.1.0/24
+ 10.0.0.0/24
+ 10.0.0.0/24
+ 10.1.2.0/24
+ 10.1.2.0/24
+ 10.1.0.0/24
+ 10.0.0.0/24
+ 10.0.0.0/24
+ 10.0.0.0/24
+ 10::/24
+ 10::/24
+ ::/24
+(17 rows)
+
+-- just making sure these functions are callable and don't crash
+SELECT 'ok' AS result
+FROM unnest(ARRAY[
+ text(inet_client_addr()),
+ text(inet_client_port()),
+ text(inet_server_addr()),
+ text(inet_server_port())
+ ]) AS x;
+ result
+--------
+ ok
+ ok
+ ok
+ ok
+(4 rows)
+
-- check that btree index works correctly
CREATE INDEX inet_idx1 ON inet_tbl(i);
SET enable_seqscan TO off;
diff --git a/src/test/regress/sql/inet.sql b/src/test/regress/sql/inet.sql
index 3910eac3bc..abf6a6ddae 100644
--- a/src/test/regress/sql/inet.sql
+++ b/src/test/regress/sql/inet.sql
@@ -46,6 +46,10 @@ SELECT c AS cidr, masklen(c) AS "masklen(cidr)",
i AS inet, masklen(i) AS "masklen(inet)" FROM INET_TBL
WHERE masklen(c) <= 8;
+SELECT i AS inet, abbrev(i) AS "abbrev(inet)" FROM INET_TBL;
+SELECT i AS inet, netmask(i) AS "netmask(inet)" FROM INET_TBL;
+SELECT i AS inet, hostmask(i) AS "hostmask(inet)" FROM INET_TBL;
+
SELECT c AS cidr, i AS inet FROM INET_TBL
WHERE c = i;
@@ -62,6 +66,16 @@ SELECT max(c) AS max, min(c) AS min FROM INET_TBL;
-- check the conversion to/from text and set_netmask
SELECT set_masklen(inet(text(i)), 24) FROM INET_TBL;
+SELECT set_masklen(cidr(text(c)), 24) FROM INET_TBL;
+
+-- just making sure these functions are callable and don't crash
+SELECT 'ok' AS result
+FROM unnest(ARRAY[
+ text(inet_client_addr()),
+ text(inet_client_port()),
+ text(inet_server_addr()),
+ text(inet_server_port())
+ ]) AS x;
-- check that btree index works correctly
CREATE INDEX inet_idx1 ON inet_tbl(i);
--
2.47.0
Hello Aleksander! I'm a beginner and I would like to see and try your
patch. However, I have never worked with coverage in regression tests for
PostgreSQL. Could you please tell me how it works and help me understand
the process? Thanks!
Best Regards, Stepan Neretin!
Hi Stepan,
Hello Aleksander! I'm a beginner and I would like to see and try your patch. However, I have never worked with coverage in regression tests for PostgreSQL. Could you please tell me how it works and help me understand the process? Thanks!
You are going to need some Linux distribution, GCC stack and lcov 1.16
[1]: https://github.com/linux-test-project/lcov/releases/tag/v1.16
problems with lcov 2.0+, this is being investigated [2]/messages/by-id/CAJ7c6TN+MCh99EZ8YGhXZAdnqvNQYir6E34B_mmcB5KsxCB00A@mail.gmail.com)
Apply the patch as usual ( git clone
git://git.postgresql.org/git/postgresql.git ; git am ... ) and run:
```
git clean -dfx && meson setup --buildtype debug
-DPG_TEST_EXTRA="kerberos ldap ssl" -Db_coverage=true -Dldap=disabled
-Dssl=openssl -Dcassert=true -Dtap_tests=enabled
-Dprefix=/home/eax/pginstall build && ninja -C build &&
PG_TEST_EXTRA=1 meson test -C build && ninja -C build coverage-html
```
Note the `-Db_coverage=true` and `ninja -C build coverage-html` parts.
Change `prefix` to an appropriate one. This will take longer than
usual. Your coverage report will be located in
build/meson-logs/coveragereport. You can compare the reports with and
without the patch to ensure that the patch improves code coverage.
[1]: https://github.com/linux-test-project/lcov/releases/tag/v1.16
[2]: /messages/by-id/CAJ7c6TN+MCh99EZ8YGhXZAdnqvNQYir6E34B_mmcB5KsxCB00A@mail.gmail.com
--
Best regards,
Aleksander Alekseev
I could not send it to the mailing list, so I'm resending it.
___
Hi Aleksander,
I have tested your patch.
I have confirmed that the coverage improves
to the expected value（69.8%->80.1%)
Your patch looks good to me.
## test and make coverage
source: commit 9a45a89c38f3257b13e09edf382e32fa28b918c2 (HEAD)
./configure --enable-coverage --enable-tap-tests --with-llvm CFLAGS=-O0
make check-world
make coverage-html
## Proposal to add a test for the set_masklen function
I think we could add the following test to
further improve the coverage.
Adding the following cases to the set_masklen()
test would further improve coverage.
* netmask = -1
* netmask > maxvalue(33 when ipv4)
```
-- check to treat netmask as maximum value when -1
SELECT set_masklen(cidr(text(c)), -1) FROM INET_TBL;
set_masklen
--------------------
192.168.1.0/32
192.168.1.0/32
192.168.1.0/32
192.168.1.0/32
192.168.1.0/32
192.168.1.0/32
10.0.0.0/32
10.0.0.0/32
10.1.2.3/32
10.1.2.0/32
10.1.0.0/32
10.0.0.0/32
10.0.0.0/32
10.0.0.0/32
10:23::f1/128
10:23::8000/128
::ffff:1.2.3.4/128
(17 rows)
-- check that rejects invalid netmask
SELECT set_masklen(inet(text(i)), 33) FROM INET_TBL;
ERROR: invalid mask length: 33
SELECT set_masklen(cidr(text(c)), 33) FROM INET_TBL;
ERROR: invalid mask length: 33
```
This would increase coverage from 80.1% to 80.5%.
The improvement value is small, but it would
be worth adding. What do you think?
As a side note,
the macaddr/macaddr8 type processing
in the convert_network_to_scalar() does not seem
to be testing. This is related to
the macaddr/macaddr8 type histograms and do not
appear to work in a simple test.
This should be considered for another time.
Best Regards,
Keisuke Kuroda
NTT Comware
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
Hi everyone,
Many thanks for all your great feedback.
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.
OK, I moved the named tests to src/test/ssl/t/003_sslinfo.pl:
```
$ PG_TEST_EXTRA=ssl meson test -C build --suite postgresql:ssl
$ less build/meson-logs/testlog.txt
[...]
ok 22 - inet_client_addr() returned non-NULL value
ok 23 - inet_client_port() returned non-NULL value
ok 24 - inet_server_addr() returned non-NULL value
ok 25 - inet_server_port() returned non-NULL value
[...]
```
Adding the following cases to the set_masklen()
test would further improve coverage.
* netmask = -1
* netmask > maxvalue(33 when ipv4)
Good idea. I included the proposed tests into the attached patch v2.
While on it I noticed the following comment:
```
-- check the conversion to/from text and set_netmask
```
Since we don't have set_netmask() I think the comment is wrong, so I
corrected it.
--
Best regards,
Aleksander Alekseev
Attachments:
v2-0001-Improve-code-coverage-of-network-address-function.patchapplication/x-patch; name=v2-0001-Improve-code-coverage-of-network-address-function.patchDownload
From cd27cd694b53c4c8914ef9467a7af69de91017d5 Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Thu, 31 Oct 2024 17:54:56 +0300
Subject: [PATCH v2] Improve code coverage of network address functions
The following functions were not covered by tests:
- abbrev(inet)
- set_masklen(cidr,int4)
- netmask(inet)
- hostmask(inet)
- inet_client_addr()
- inet_client_port()
- inet_server_addr()
- inet_server_port()
Correct this.
Aleksander Alekseev, reviewed by Keisuke Kuroda, Jacob Champion, Tom Lane
Discussion: https://postgr.es/m/CAJ7c6TOyZ9bGNrDK6Z3Q0gr9ow8ZpOm+=+01mpE0dsdH4C+u9A@mail.gmail.com
---
src/test/regress/expected/inet.out | 118 ++++++++++++++++++++++++++++-
src/test/regress/sql/inet.sql | 14 +++-
src/test/ssl/t/003_sslinfo.pl | 19 +++++
3 files changed, 149 insertions(+), 2 deletions(-)
diff --git a/src/test/regress/expected/inet.out b/src/test/regress/expected/inet.out
index b6895d9ced0..056b283bc32 100644
--- a/src/test/regress/expected/inet.out
+++ b/src/test/regress/expected/inet.out
@@ -190,6 +190,72 @@ SELECT c AS cidr, masklen(c) AS "masklen(cidr)",
10.0.0.0/8 | 8 | 9.1.2.3/8 | 8
(4 rows)
+SELECT i AS inet, abbrev(i) AS "abbrev(inet)" FROM INET_TBL;
+ inet | abbrev(inet)
+------------------+------------------
+ 192.168.1.226/24 | 192.168.1.226/24
+ 192.168.1.226 | 192.168.1.226
+ 192.168.1.0/24 | 192.168.1.0/24
+ 192.168.1.0/25 | 192.168.1.0/25
+ 192.168.1.255/24 | 192.168.1.255/24
+ 192.168.1.255/25 | 192.168.1.255/25
+ 10.1.2.3/8 | 10.1.2.3/8
+ 10.1.2.3/8 | 10.1.2.3/8
+ 10.1.2.3 | 10.1.2.3
+ 10.1.2.3/24 | 10.1.2.3/24
+ 10.1.2.3/16 | 10.1.2.3/16
+ 10.1.2.3/8 | 10.1.2.3/8
+ 11.1.2.3/8 | 11.1.2.3/8
+ 9.1.2.3/8 | 9.1.2.3/8
+ 10:23::f1/64 | 10:23::f1/64
+ 10:23::ffff | 10:23::ffff
+ ::4.3.2.1/24 | ::4.3.2.1/24
+(17 rows)
+
+SELECT i AS inet, netmask(i) AS "netmask(inet)" FROM INET_TBL;
+ inet | netmask(inet)
+------------------+-----------------------------------------
+ 192.168.1.226/24 | 255.255.255.0
+ 192.168.1.226 | 255.255.255.255
+ 192.168.1.0/24 | 255.255.255.0
+ 192.168.1.0/25 | 255.255.255.128
+ 192.168.1.255/24 | 255.255.255.0
+ 192.168.1.255/25 | 255.255.255.128
+ 10.1.2.3/8 | 255.0.0.0
+ 10.1.2.3/8 | 255.0.0.0
+ 10.1.2.3 | 255.255.255.255
+ 10.1.2.3/24 | 255.255.255.0
+ 10.1.2.3/16 | 255.255.0.0
+ 10.1.2.3/8 | 255.0.0.0
+ 11.1.2.3/8 | 255.0.0.0
+ 9.1.2.3/8 | 255.0.0.0
+ 10:23::f1/64 | ffff:ffff:ffff:ffff::
+ 10:23::ffff | ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff
+ ::4.3.2.1/24 | ffff:ff00::
+(17 rows)
+
+SELECT i AS inet, hostmask(i) AS "hostmask(inet)" FROM INET_TBL;
+ inet | hostmask(inet)
+------------------+------------------------------------
+ 192.168.1.226/24 | 0.0.0.255
+ 192.168.1.226 | 0.0.0.0
+ 192.168.1.0/24 | 0.0.0.255
+ 192.168.1.0/25 | 0.0.0.127
+ 192.168.1.255/24 | 0.0.0.255
+ 192.168.1.255/25 | 0.0.0.127
+ 10.1.2.3/8 | 0.255.255.255
+ 10.1.2.3/8 | 0.255.255.255
+ 10.1.2.3 | 0.0.0.0
+ 10.1.2.3/24 | 0.0.0.255
+ 10.1.2.3/16 | 0.0.255.255
+ 10.1.2.3/8 | 0.255.255.255
+ 11.1.2.3/8 | 0.255.255.255
+ 9.1.2.3/8 | 0.255.255.255
+ 10:23::f1/64 | ::ffff:ffff:ffff:ffff
+ 10:23::ffff | ::
+ ::4.3.2.1/24 | 0:ff:ffff:ffff:ffff:ffff:ffff:ffff
+(17 rows)
+
SELECT c AS cidr, i AS inet FROM INET_TBL
WHERE c = i;
cidr | inet
@@ -238,7 +304,7 @@ SELECT max(c) AS max, min(c) AS min FROM INET_TBL;
10:23::8000/113 | 10.0.0.0/8
(1 row)
--- check the conversion to/from text and set_netmask
+-- check the conversion to/from text and setting netmask
SELECT set_masklen(inet(text(i)), 24) FROM INET_TBL;
set_masklen
------------------
@@ -261,6 +327,56 @@ SELECT set_masklen(inet(text(i)), 24) FROM INET_TBL;
::4.3.2.1/24
(17 rows)
+SELECT set_masklen(cidr(text(c)), 24) FROM INET_TBL;
+ set_masklen
+----------------
+ 192.168.1.0/24
+ 192.168.1.0/24
+ 192.168.1.0/24
+ 192.168.1.0/24
+ 192.168.1.0/24
+ 192.168.1.0/24
+ 10.0.0.0/24
+ 10.0.0.0/24
+ 10.1.2.0/24
+ 10.1.2.0/24
+ 10.1.0.0/24
+ 10.0.0.0/24
+ 10.0.0.0/24
+ 10.0.0.0/24
+ 10::/24
+ 10::/24
+ ::/24
+(17 rows)
+
+-- check that netmask is treated as maximum value when it equals -1
+SELECT set_masklen(cidr(text(c)), -1) FROM INET_TBL;
+ set_masklen
+--------------------
+ 192.168.1.0/32
+ 192.168.1.0/32
+ 192.168.1.0/32
+ 192.168.1.0/32
+ 192.168.1.0/32
+ 192.168.1.0/32
+ 10.0.0.0/32
+ 10.0.0.0/32
+ 10.1.2.3/32
+ 10.1.2.0/32
+ 10.1.0.0/32
+ 10.0.0.0/32
+ 10.0.0.0/32
+ 10.0.0.0/32
+ 10:23::f1/128
+ 10:23::8000/128
+ ::ffff:1.2.3.4/128
+(17 rows)
+
+-- check that invalid netmask is rejected
+SELECT set_masklen(inet(text(i)), 33) FROM INET_TBL;
+ERROR: invalid mask length: 33
+SELECT set_masklen(cidr(text(c)), 33) FROM INET_TBL;
+ERROR: invalid mask length: 33
-- check that btree index works correctly
CREATE INDEX inet_idx1 ON inet_tbl(i);
SET enable_seqscan TO off;
diff --git a/src/test/regress/sql/inet.sql b/src/test/regress/sql/inet.sql
index 3910eac3bc4..a946d7d170b 100644
--- a/src/test/regress/sql/inet.sql
+++ b/src/test/regress/sql/inet.sql
@@ -46,6 +46,10 @@ SELECT c AS cidr, masklen(c) AS "masklen(cidr)",
i AS inet, masklen(i) AS "masklen(inet)" FROM INET_TBL
WHERE masklen(c) <= 8;
+SELECT i AS inet, abbrev(i) AS "abbrev(inet)" FROM INET_TBL;
+SELECT i AS inet, netmask(i) AS "netmask(inet)" FROM INET_TBL;
+SELECT i AS inet, hostmask(i) AS "hostmask(inet)" FROM INET_TBL;
+
SELECT c AS cidr, i AS inet FROM INET_TBL
WHERE c = i;
@@ -60,8 +64,16 @@ SELECT i, c,
SELECT max(i) AS max, min(i) AS min FROM INET_TBL;
SELECT max(c) AS max, min(c) AS min FROM INET_TBL;
--- check the conversion to/from text and set_netmask
+-- check the conversion to/from text and setting netmask
SELECT set_masklen(inet(text(i)), 24) FROM INET_TBL;
+SELECT set_masklen(cidr(text(c)), 24) FROM INET_TBL;
+
+-- check that netmask is treated as maximum value when it equals -1
+SELECT set_masklen(cidr(text(c)), -1) FROM INET_TBL;
+
+-- check that invalid netmask is rejected
+SELECT set_masklen(inet(text(i)), 33) FROM INET_TBL;
+SELECT set_masklen(cidr(text(c)), 33) FROM INET_TBL;
-- check that btree index works correctly
CREATE INDEX inet_idx1 ON inet_tbl(i);
diff --git a/src/test/ssl/t/003_sslinfo.pl b/src/test/ssl/t/003_sslinfo.pl
index b9eae8d641b..780c8b29452 100644
--- a/src/test/ssl/t/003_sslinfo.pl
+++ b/src/test/ssl/t/003_sslinfo.pl
@@ -191,4 +191,23 @@ foreach my $c (@cases)
"ssl_client_cert_present() for $c->{'opts'}");
}
+# Make sure the following functions are callable and don't crash.
+# Unlike SQL tests this TAP test uses TCP connections. This makes it
+# a good place for testing named functions due to better code coverage.
+my @inet_funcs = qw(
+ inet_client_addr
+ inet_client_port
+ inet_server_addr
+ inet_server_port
+);
+
+for my $f (@inet_funcs)
+{
+ $result = $node->safe_psql(
+ "certdb",
+ "SELECT $f() IS NULL;",
+ connstr => $common_connstr);
+ is($result, 'f', "$f() returned non-NULL value");
+}
+
done_testing();
--
2.48.1
The following review has been posted through the commitfest application:
make installcheck-world: tested, failed
Implements feature: tested, failed
Spec compliant: tested, failed
Documentation: tested, failed
Thank you for your patch!
I have confirmed that the coverage improves
to the expected value（69.7%->83.0%)
source: commit 75eb9766ec201b62f264554019757716093e2a2f(HEAD)
## add with-openssl option for ssltest
./configure --enable-coverage --enable-tap-tests --with-llvm --with-openssl CFLAGS=-O0
make check-world PG_TEST_EXTRA='ssl' -j 4 > /tmp/regress.log
make coverage-html
The new status of this patch is: Ready for Committer
On Tue, Jan 28, 2025 at 07:24:38AM +0000, keisuke kuroda wrote:
I have confirmed that the coverage improves
to the expected value（69.7%->83.0%)source: commit 75eb9766ec201b62f264554019757716093e2a2f(HEAD)
## add with-openssl option for ssltest
./configure --enable-coverage --enable-tap-tests --with-llvm --with-openssl CFLAGS=-O0
make check-world PG_TEST_EXTRA='ssl' -j 4 > /tmp/regress.log
make coverage-html
Aleksander has mentioned me this patch a couple of days ago. Forgot a
bit about it.
The SQL additions seem sensible to me at quick glance (will
double-check the coverage numbers a bit later). Thanks for compiling
that.
About the tests pushed to the SSL test suite, I'm +-0. 003_sslinfo.pl
is a bit better than the two others in the SSL test suite, still it
does not really fit into this file.
--
Michael
The following review has been posted through the commitfest application:
make installcheck-world: tested, failed
Implements feature: tested, failed
Spec compliant: tested, failed
Documentation: tested, failed
Sorry, I thought I checked with the commitfest App,
but it doesn't seem to be reflecting correctly.
The correct version is as follows.
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed
Hi,
The correct version is as follows.
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed
Thanks for your feedback!
About the tests pushed to the SSL test suite, I'm +-0. 003_sslinfo.pl
is a bit better than the two others in the SSL test suite, still it
does not really fit into this file.
So to clarify, you propose creating a new file for the test (still in
the ssl/ suite) or keep it as is?
I agree that this is not exactly the best place for the test. However
I'm not sure whether creating a new one, e.g.
ssl/t/004_code_coverage.pl will be much better considering the fact
that the test still has little (nothing) to do with SSL.
Personally I'm fine with either option though.
--
Best regards,
Aleksander Alekseev
On Tue, Jan 28, 2025 at 01:33:58PM +0300, Aleksander Alekseev wrote:
Thanks for your feedback!
set_masklen(inet) could be covered for the -1 case, and it was missing
in the patch submitted. For consistency with the other queries,
moving the call of abbrev(inet) with the existing abbrev(cidr) makes
more sense, I guess, and we could expand a bit the use of aliases in
the attributes even for the existing queries with broadcast() and
abbrev() to make the output nicer.
So to clarify, you propose creating a new file for the test (still in
the ssl/ suite) or keep it as is?I agree that this is not exactly the best place for the test. However
I'm not sure whether creating a new one, e.g.
ssl/t/004_code_coverage.pl will be much better considering the fact
that the test still has little (nothing) to do with SSL.Personally I'm fine with either option though.
To be honest, I don't what's the best course of action here :)
Sticking that into the SSL tests looks incorrect to me. If we care
only about the execution without the output, a SQL test is the most
common practice. People can also use pg_regress with custom
connection strings, but I agree that it limits the impact of these
additions in the default cases where the tests are run using a unix
domain socket and these return NULL.
The SQL tests don't fall into that category and they are nice
additions, so I have applied this part with the tweaks mentioned
above.
--
Michael