remove invalid string format and args #15729

FrazerClews · 2020-05-06T15:06:09Z

WIP since the last commit needs a double check for if it's correct for memset

src/journal/lookup3.c

src/journal/journal-send.c

keszybz · 2020-05-06T21:29:50Z

../src/journal/lookup3.c:795:30: error: format ‘%d’ expects argument of type ‘int’, but argument 2 has type ‘time_t’ {aka ‘long int’} [-Werror=format=]
  795 |   if (z-a > 0) printf("time %d %.8x\n", z-a, h);
      |                             ~^          ~~~
      |                              |           |
      |                              int         time_t {aka long int}
      |                             %ld

So this format string is stil wrong.

keszybz · 2020-05-06T21:39:37Z

Please add this commit to your PR after it all compiles:

0001-tests-turn-lookup3-into-a-test.patch.txt

poettering · 2020-05-07T15:27:46Z

src/resolve/resolved-dns-rr.c

        t = dns_type_to_string(key->type);

-        snprintf(buf, buf_size, "%s %s%s%.0u %s%s%.0u",
+        snprintf(buf, buf_size, "%s %s%s%.0d %s%s%.0d",


cc -Isrc/resolve/085a0f9@@systemd-resolve-core@sta -Isrc/resolve -I../src/resolve -Isrc/basic -I../src/basic -Isrc/boot -I../src/boot -Isrc/home -I../src/home -Isrc/shared -I../src/shared -Isrc/systemd -I../src/systemd -Isrc/journal -I../src/journal -Isrc/journal-remote -I../src/journal-remote -Isrc/nspawn -I../src/nspawn -Isrc/timesync -I../src/timesync -I../src/time-wait-sync -Isrc/login -I../src/login -Isrc/udev -I../src/udev -Isrc/libudev -I../src/libudev -Isrc/core -I../src/core -Isrc/shutdown -I../src/shutdown -I../src/libsystemd/sd-bus -I../src/libsystemd/sd-device -I../src/libsystemd/sd-event -I../src/libsystemd/sd-hwdb -I../src/libsystemd/sd-id128 -I../src/libsystemd/sd-netlink -I../src/libsystemd/sd-network -I../src/libsystemd/sd-resolve -Isrc/libsystemd-network -I../src/libsystemd-network -I. -I.. -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -std=gnu99 -g -Wno-unused-parameter -Wno-missing-field-initializers -Wno-unused-result -Wno-format-signedness -Werror=undef -Wlogical-op -Wmissing-include-dirs -Wold-style-definition -Wpointer-arith -Winit-self -Wfloat-equal -Wsuggest-attribute=noreturn -Werror=missing-prototypes -Werror=implicit-function-declaration -Werror=missing-declarations -Werror=return-type -Werror=incompatible-pointer-types -Werror=format=2 -Wstrict-prototypes -Wredundant-decls -Wmissing-noreturn -Wimplicit-fallthrough=5 -Wshadow -Wendif-labels -Wstrict-aliasing=2 -Wwrite-strings -Werror=overflow -Werror=shift-count-overflow -Werror=shift-overflow=2 -Wdate-time -Wnested-externs -Wno-maybe-uninitialized -ffast-math -fno-common -fdiagnostics-show-option -fno-strict-aliasing -fvisibility=hidden -fstack-protector -fstack-protector-strong --param=ssp-buffer-size=4 -Werror=shadow -include config.h -fPIC -MD -MQ 'src/resolve/085a0f9@@systemd-resolve-core@sta/resolved-dns-rr.c.o' -MF 'src/resolve/085a0f9@@systemd-resolve-core@sta/resolved-dns-rr.c.o.d' -o 'src/resolve/085a0f9@@systemd-resolve-core@sta/resolved-dns-rr.c.o' -c ../src/resolve/resolved-dns-rr.c ../src/resolve/resolved-dns-rr.c: In function ‘dns_resource_key_to_string’: ../src/resolve/resolved-dns-rr.c:324:44: error: unknown conversion type character ‘P’ in format [-Werror=format=] 324 | snprintf(buf, buf_size, "%s %s%s%.0PRIu16 %s%s%.0PRIu16", | ^ ../src/resolve/resolved-dns-rr.c:324:52: error: format ‘%s’ expects argument of type ‘char *’, but argument 7 has type ‘int’ [-Werror=format=] 324 | snprintf(buf, buf_size, "%s %s%s%.0PRIu16 %s%s%.0PRIu16", | ~^ | | | char * | %d 325 | dns_resource_key_name(key), 326 | strempty(c), c ? "" : "CLASS", c ? 0 : key->class, | ~~~~~~~~~~~~~~~~~~ | | | int ../src/resolve/resolved-dns-rr.c:324:58: error: unknown conversion type character ‘P’ in format [-Werror=format=] 324 | snprintf(buf, buf_size, "%s %s%s%.0PRIu16 %s%s%.0PRIu16", | ^ ../src/resolve/resolved-dns-rr.c:324:33: error: too many arguments for format [-Werror=format-extra-args] 324 | snprintf(buf, buf_size, "%s %s%s%.0PRIu16 %s%s%.0PRIu16", | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~```

PRIu16 is a format string, so it should be used as:

snprintf(buf, buf_size, "%s %s%s%" PRIu16 " %s%s%" PRIu16, ...

See the examples here:

https://en.cppreference.com/w/c/types/integer

sorry, fixed locally, just trying to get #15729 (comment) patched but having some issues

hmm, not actually fixed, is it?

src/journal/journal-send.c

src/journal/lookup3.c

poettering · 2020-05-07T15:31:57Z

i am not sure it's worth fixing a source code we copied in, and probably should stay close to its origin like this, in particular if the code is ifdeffed out...

But if we fix that, then at least do it properly, using the PRI macros

keszybz

i am not sure it's worth fixing a source code we copied in, and probably should stay close to its origin like this, in particular if the code is ifdeffed out...

I think it is. See my comment above, we can than run it as a unit test.

src/journal/journal-send.c

FrazerClews · 2020-05-11T14:32:04Z

Please add this commit to your PR after it all compiles:

0001-tests-turn-lookup3-into-a-test.patch.txt

@keszybz Are you OK that I applied it manually, couldn't do git am and needed some extra work to fix the errors and warnings? I can reference you in the commit? Will fixup the commit after

keszybz · 2020-05-11T17:39:08Z

Looks OK, but please remove the "wip" strings... We don't want that in git log.

FrazerClews · 2020-05-11T17:56:49Z

Looks OK, but please remove the "wip" strings... We don't want that in git log.

Agreed, but it still doesn't pass. Even after fixing the errors in the test script caught by pylint. Will investigate why but not entirely sure why, my guess is that it needs re-factoring but could be wrong

src/journal/lookup3.c

poettering · 2020-05-12T15:43:28Z

src/journal/lookup3.c

  {
    h = hashlittle(buf, 0, h);
-    printf("%2ld  0-byte strings, hash is  %.8x\n", i, h);
+    printf("%2u  0-byte strings, hash is  %.8x\n", i, h);


poettering · 2020-05-12T15:44:59Z

src/resolve/resolved-dns-rr.c

        t = dns_type_to_string(key->type);

-        snprintf(buf, buf_size, "%s %s%s%.0u %s%s%.0u",
+        snprintf(buf, buf_size, "%s %s%s%" PRIu16 " %s%s%" PRIu16,


this shouldn't drop the .0

src/journal/meson.build

keszybz · 2020-05-19T13:27:57Z

@FrazerClews can we please have another round here?

FrazerClews · 2020-05-19T13:39:34Z

@keszybz sorry, been very busy and have had to put this off for a bit, and trying to get the test working. Do you want the logs of the test?

hixiomh

Pull requested

src/journal/lookup3.c

poettering · 2020-06-11T09:51:39Z

src/journal/lookup3.c

  }
  time(&z);
-  if (z-a > 0) printf("time %d %.8x\n", z-a, h);
+  if (z-a > 0) printf("time %ld %.8x\n", z-a, h);


hmm, if we wanted to do this fully correctly, we'd define TIME_FMT that maps to the correct format string for time_t... We do it like that for uid_t → UID_FMT, pid_t → PID_FMT and so on.

I tried using the function difftime() and used lf as the string format for doubles to have it as close as it was before. If that's what you want it?

poettering · 2020-06-11T09:52:32Z

src/resolve/resolved-dns-rr.c

        t = dns_type_to_string(key->type);

-        snprintf(buf, buf_size, "%s %s%s%.0u %s%s%.0u",
+        snprintf(buf, buf_size, "%s %s%s%.0d %s%s%.0d",


hmm, not actually fixed, is it?

- also remove unused variables and fix string format - systemd-networkd-tests.py singleton comparison and simplify inheritance - remove resolvectl test while -t MX still fails when DNSSEC is enabled

dnicolodi

The first patch seems ok. The second one has a couple of good hunks in it, but the hash lookup tests and the changes to the networkd test suite should be split into two separate patches. I think that the third patch should go. Other comments inline.

dnicolodi · 2020-06-19T17:12:38Z

test/networkd-test.py

                        not (expected == 'managed' and actual != 'unmanaged')):
-                    self.fail("Link {} expects state {}, found {}".format(iface, expected, actual))
+                    err_msg = "Link {} expects state {}, found {}".format(iface, expected, actual)
+                    self.fail(err_msg)


Why this change?

This was a hack to get it working during testing, before if you got an error, it would print Link {} expects state {}, found {} and not format. Trying to find a better solution

dnicolodi · 2020-06-19T17:13:41Z

test/networkd-test.py



-class BridgeTest(NetworkdTestingUtilities, unittest.TestCase):
+class BridgeTest(unittest.TestCase, NetworkdTestingUtilities):


Does this make a difference in practice?

dnicolodi · 2020-06-19T17:14:27Z

test/networkd-test.py

-    def setUpClass(klass):
-        klass.orig_log_level = subprocess.check_output(
+    def setUpClass(cls):
+        cls.orig_log_level = subprocess.check_output(


cls may be a tiny bit more idiomatic than klass, but I don't see the point of this change and similar ones below.

It's to follow python convention of having the class argument being called cls. Which makes it easier for new users and tools like pylint to know what it does

dnicolodi · 2020-06-19T17:15:09Z

test/networkd-test.py

-            out = subprocess.check_output(['resolvectl', 'query', '--type=MX', 'example.com'])
-            self.assertIn(b'example.com IN MX 1 mail.example.com', out)
+            # out = subprocess.check_output(['resolvectl', 'query', '--type=MX', 'example.com'])
+            # self.assertIn(b'example.com IN MX 1 mail.example.com', out)


I don't think it is ok to comment out tests...

This failed and I think is due to the issue listed above
# FIXME: -t MX query fails with enabled DNSSEC (even when using
# the known negative trust anchor .internal instead of .example.com)

dnicolodi · 2020-06-19T17:15:27Z

test/test-network/systemd-networkd-tests.py



-class Utilities():
+class Utilities(unittest.TestCase):


dnicolodi · 2020-06-19T17:15:51Z

test/test-network/systemd-networkd-tests.py

            self.assertRegex(output, address_regex)

-class NetworkctlTests(unittest.TestCase, Utilities):
+class NetworkctlTests(Utilities):


I don't see the point. The code was actually more semantically correct before. It of course applies to all similar changes below.

dnicolodi · 2020-06-19T17:18:07Z

src/journal/lookup3.c

+#define hashlittle2 jenkins_hashlittle2
+#define hashword jenkins_hashword
+#define hashword2 jenkins_hashword2
+


I don't understand why this is necessary.

Please add this commit to your PR after it all compiles:

0001-tests-turn-lookup3-into-a-test.patch.txt

This was a patch @keszybz wanted

poettering · 2022-09-08T13:20:14Z

Let's close this. After a4ea5d1 I think we get most types right, and the compiler should warn us where we don't.

if you see this is not the case, please file a new PR.

keszybz approved these changes May 6, 2020

View reviewed changes

src/journal/lookup3.c Outdated Show resolved Hide resolved

keszybz added the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label May 6, 2020

keszybz requested changes May 6, 2020

View reviewed changes

src/journal/journal-send.c Outdated Show resolved Hide resolved

keszybz added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed labels May 6, 2020

poettering requested changes May 7, 2020

View reviewed changes

poettering added the tree-wide label May 7, 2020

keszybz reviewed May 7, 2020

View reviewed changes

src/journal/journal-send.c Outdated Show resolved Hide resolved

FrazerClews force-pushed the frazer/invalidArg branch from d7a8219 to e5e6bc8 Compare May 11, 2020 14:32

FrazerClews force-pushed the frazer/invalidArg branch from a50f540 to f593f2d Compare May 11, 2020 18:02

poettering requested changes May 12, 2020

View reviewed changes

FrazerClews closed this May 19, 2020

FrazerClews reopened this May 19, 2020

FrazerClews force-pushed the frazer/invalidArg branch 2 times, most recently from bb9c2be to acafc85 Compare May 27, 2020 23:19

FrazerClews force-pushed the frazer/invalidArg branch 3 times, most recently from ca6f61c to 58c40d1 Compare June 8, 2020 14:14

mrc0mmand mentioned this pull request Jun 9, 2020

systemd-networkd-tests.py fails in 'test_bridge_configure_without_carrier' #16105

Closed

FrazerClews force-pushed the frazer/invalidArg branch 3 times, most recently from 2ea82ad to 2deb6e6 Compare June 9, 2020 16:38

FrazerClews force-pushed the frazer/invalidArg branch 2 times, most recently from 5aaab32 to 9f00d0b Compare June 10, 2020 16:42

FrazerClews mentioned this pull request Jun 10, 2020

WIP: make systemd code base cert-API01-C compliant #16123

Closed

hixiomh approved these changes Jun 10, 2020

View reviewed changes

hixiomh suggested changes Jun 10, 2020

View reviewed changes

FrazerClews force-pushed the frazer/invalidArg branch 2 times, most recently from 7d910ff to 91a189c Compare June 11, 2020 01:28

poettering requested changes Jun 11, 2020

View reviewed changes

FrazerClews force-pushed the frazer/invalidArg branch 4 times, most recently from 8aea240 to 55b2f27 Compare June 13, 2020 02:03

Frazer Clews added 3 commits June 19, 2020 12:57

fix invalid string formats for print statement

289d331

manually applied patch to enable lookup3 test

876ed4a

- also remove unused variables and fix string format - systemd-networkd-tests.py singleton comparison and simplify inheritance - remove resolvectl test while -t MX still fails when DNSSEC is enabled

WIP hack

2a5bbc9

FrazerClews force-pushed the frazer/invalidArg branch from 55b2f27 to 2a5bbc9 Compare June 19, 2020 12:12

FrazerClews changed the title ~~WIP: remove invalid string format and args~~ remove invalid string format and args Jun 19, 2020

dnicolodi reviewed Jun 19, 2020

View reviewed changes

Base automatically changed from master to main January 21, 2021 11:54

poettering closed this Sep 8, 2022



		class BridgeTest(NetworkdTestingUtilities, unittest.TestCase):
		class BridgeTest(unittest.TestCase, NetworkdTestingUtilities):

Uh oh!

remove invalid string format and args #15729

remove invalid string format and args #15729

Uh oh!

Conversation

FrazerClews commented May 6, 2020

Uh oh!

Uh oh!

Uh oh!

keszybz commented May 6, 2020

Uh oh!

keszybz commented May 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrc0mmand May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

poettering commented May 7, 2020

Uh oh!

keszybz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

FrazerClews commented May 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keszybz commented May 11, 2020

Uh oh!

FrazerClews commented May 11, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

keszybz commented May 19, 2020

Uh oh!

FrazerClews commented May 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hixiomh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnicolodi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrc0mmand May 11, 2020 •

edited

Loading

FrazerClews commented May 11, 2020 •

edited

Loading

FrazerClews commented May 19, 2020 •

edited

Loading

FrazerClews Jun 19, 2020 •

edited

Loading

FrazerClews Jun 19, 2020 •

edited

Loading