Skip to content

https_dns_proxy: logging fix#161

Closed
stangri wants to merge 1 commit intoaarond10:masterfrom
stangri:logging-fix
Closed

https_dns_proxy: logging fix#161
stangri wants to merge 1 commit intoaarond10:masterfrom
stangri:logging-fix

Conversation

@stangri
Copy link
Copy Markdown
Contributor

@stangri stangri commented Oct 16, 2023

This fix is based on @schmaller's patch and @systemcrash testing from this issue: openwrt/packages#19366

Credit goes to @schmaller and @systemcrash.

@stangri
https_dns_proxy: logging fix
88757c6 
This fix is based on @schmaller's patch and @systemcrash testing from
this issue: openwrt/packages#19366

Credit goes to @schmaller and @systemcrash.

Signed-off-by: Stan Grishin <stangri@melmac.net>
@systemcrash
Copy link
Copy Markdown
Contributor

systemcrash commented Oct 17, 2023

This fixes compiler errors, as well as legitimate crashes:

/home/user/openwrt/build_dir/target-mips_24kc_musl/https-dns-proxy-2023-05-25/src/logging.c: In function '_log':
/home/user/openwrt/build_dir/target-mips_24kc_musl/https-dns-proxy-2023-05-25/src/logging.c:81:24: warning: format '%lu' expects argument of type 'long unsigned int', but argument 4 has type 'long long unsigned int' [-Wformat=]
   81 |   fprintf(logf, "%s %8lu.%06lu %s:%d ", SeverityStr[severity],
      |                     ~~~^
      |                        |
      |                        long unsigned int
      |                     %8llu
   82 |           (uint64_t)tv.tv_sec,
      |           ~~~~~~~~~~~~~~~~~~~
      |           |
      |           long long unsigned int
/home/user/openwrt/build_dir/target-mips_24kc_musl/https-dns-proxy-2023-05-25/src/logging.c:81:30: warning: format '%lu' expects argument of type 'long unsigned int', but argument 5 has type 'long long unsigned int' [-Wformat=]
   81 |   fprintf(logf, "%s %8lu.%06lu %s:%d ", SeverityStr[severity],
      |                          ~~~~^
      |                              |
      |                              long unsigned int
      |                          %06llu
   82 |           (uint64_t)tv.tv_sec,
   83 |           (uint64_t)tv.tv_usec, file, line);
      |           ~~~~~~~~~~~~~~~~~~~~
      |           |
      |           long long unsigned int

@baranyaib90
Copy link
Copy Markdown
Contributor

baranyaib90 commented Oct 19, 2023

I agree, that this fix is needed.

@baranyaib90
Copy link
Copy Markdown
Contributor

baranyaib90 commented Oct 24, 2023 •
edited
Loading

Actually this does not work well on Ubuntu 22.04 64bit :S
I would go into the PRIu64 direction.

https_dns_proxy/src/logging.c:82:11: warning: format specifies type 'unsigned long long' but the argument has type 'uint64_t' (aka 'unsigned long') [clang-diagnostic-format]
          (uint64_t)tv.tv_sec,
          ^~~~~~~~~~~~~~~~~~~
https_dns_proxy/src/logging.c:81:27: note: FIX-IT applied suggested code changes
  (void)fprintf(logf, "%s %8llu.%06llu %s:%d ", SeverityStr[severity],
                          ^
https_dns_proxy/src/logging.c:83:11: warning: format specifies type 'unsigned long long' but the argument has type 'uint64_t' (aka 'unsigned long') [clang-diagnostic-format]
          (uint64_t)tv.tv_usec, file, line);
          ^~~~~~~~~~~~~~~~~~~~
https_dns_proxy/src/logging.c:81:33: note: FIX-IT applied suggested code changes
  (void)fprintf(logf, "%s %8llu.%06llu %s:%d ", SeverityStr[severity],
                                ^

Ps.: even clang-tidy shows the warning at Build step
https://github.com/aarond10/https_dns_proxy/actions/runs/6528150677/job/17723933346?pr=161

@stangri
Copy link
Copy Markdown
Contributor Author

stangri commented Oct 24, 2023

@baranyaib90 I've already merged the patch to OpenWrt packages repo, so it's fixed there.

@baranyaib90 baranyaib90 mentioned this pull request Oct 24, 2023
Merged
@baranyaib90
Copy link
Copy Markdown
Contributor

baranyaib90 commented Oct 24, 2023

Patch might fix OpenWRT in your case. I'm unsure, that it actually works for all target (32/64 bit and plenty architectures).
Anyways according to clang-tidy it's incorrect :S Time will tell which works best.

@systemcrash
Copy link
Copy Markdown
Contributor

systemcrash commented Oct 25, 2023

Well, here's the rub: time_t is 32 bit on 32 bit systems+compilers. 64 bit on 64 bit systems.

https://lwn.net/Articles/598408/
https://www.gnu.org/software/libc/manual/html_node/64_002dbit-time-symbol-handling.html

In any case, if it's only this one fprintf then an #ifdef might suffice. What is certain, is that a 64 bit value will be needed in the future.

unsigned long is 32 bits and unsigned long long is equivalent to uint64. The assertion 'uint64_t' (aka 'unsigned long') feels wrong unless the compiler OS is 32bit...

@baranyaib90
Copy link
Copy Markdown
Contributor

baranyaib90 commented Oct 25, 2023

My solution just got merged on master: bd71243
I would recommend to try that one out. I'm using PRIu64 format specifier for the uint64_t type. (So casting time_t to uint64_t is necessary, just the printf format specifier cause trouble.)
My point is, that inside inttypes.h it's handled via define if the code is being compiled on 32 or 64 bit target.
So PRIu64 value is "llu" on 32 bit and "lu" on 64 bit.

I recommend to close this pull request (since this patch causes trouble on 64 bit) and use my hopefully more generic fix.

@systemcrash
Copy link
Copy Markdown
Contributor

systemcrash commented Oct 25, 2023

So PRIu64 value is "llu" on 32 bit and "lu" on 64 bit.

You probably meant vice-versa. But this seems like a promising fix. It just requires testing in situ on our platforms.

@baranyaib90
Copy link
Copy Markdown
Contributor

baranyaib90 commented Oct 25, 2023

I'm pretty sure I didn't swap them. On 32 bit more "l" letters are needed.
https://github.com/lattera/glibc/blob/master/sysdeps/generic/inttypes.h#L43
https://git.musl-libc.org/cgit/musl/tree/include/inttypes.h#n25

@stangri
Copy link
Copy Markdown
Contributor Author

stangri commented Oct 25, 2023

@systemcrash if you want to test the new code, I've updated makefile and patches in stangri/source.openwrt.melmac.ca@2358e71.

@stangri
Copy link
Copy Markdown
Contributor Author

stangri commented Oct 25, 2023

I feel it's OK to close this, I'd like to confirm the @baranyaib90's patch works tho.

@systemcrash
Copy link
Copy Markdown
Contributor

systemcrash commented Oct 25, 2023 via email

@stangri stangri closed this Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@stangri @systemcrash @baranyaib90