openwrt / procd

[MIRROR] OpenWrt service / process manager
https://git.openwrt.org/?p=project/procd.git;
4 stars 16 forks source link

trace: use standard POSIX header for basename() #6

Closed guidosarducci closed 7 months ago

guidosarducci commented 8 months ago

The musl libc only implements POSIX basename() but provided a GNU header kludge in , which is removed in musl 1.2.5 [1]. Use the standard header to avoid compilation errors like:

trace/trace.c: In function 'main':
trace/trace.c:435:64: error: implicit declaration of function 'basename'; did you mean 'rename'? [-Werror=implicit-function-declaration]
  435 | if (asprintf(&json, "/tmp/%s.%u.json", basename(*argv), child) < 0)
      |                                        ^~~~~~~~
      |                                        rename
cc1: all warnings being treated as errors

Link 1: https://git.musl-libc.org/cgit/musl/log/?qt=grep&q=basename

@blogic This change is related to https://github.com/openwrt/openwrt/pull/14802 and was tested therein.

guidosarducci commented 8 months ago

@dangowrt Would appreciate if you have a chance to look at this. Thanks...

Ansuel commented 8 months ago

@guidosarducci i searched a bit on the differences and there are a few but considering the following case maybe it's ok...

the main difference is that posix variant can modify the passed arg (while GNU one can't)

But I assume in the following context it's not a problem since argc is not used in the code after basename is called.

dangowrt commented 8 months ago

Code was written under the assumption that basename() modifies the argument.

guidosarducci commented 8 months ago

@guidosarducci i searched a bit on the differences and there are a few but considering the following case maybe it's ok...

the main difference is that posix variant can modify the passed arg (while GNU one can't)

But I assume in the following context it's not a problem since argc is not used in the code after basename is called.

Code was written under the assumption that basename() modifies the argument.

Right, I concluded the same thing while reviewing, so thought this should be OK. Thanks for looking.

guidosarducci commented 8 months ago

@dangowrt @Ansuel Are there any changes you'd like to see or is this OK as is?

guidosarducci commented 7 months ago

I see that CI checks were just added for procd are are now flagging errors, however the failure looks to be internal and unrelated to this PR. This is a trivial change, has been tested and reviewed, and is needed before a toolchain update to musl 1.2.5.

At the moment I have multiple PRs blocked by unrelated CI failures or shortcomings, sitting for several weeks total, with no response from members as to workarounds or fixes. See also https://github.com/openwrt/ubox/pull/4 for example.

Any suggestions on how best to proceed @Ansuel @dangowrt @aparcar @ynezz ?

Ansuel commented 7 months ago

Yes Sorry i will merge your pr to move things up. The error were already there and some are present but in the context of usage not really security treat.

Il Sab 30 Mar 2024, 08:53 guidosarducci @.***> ha scritto:

I see that CI checks were just added for procd are are now flagging errors, however the failure looks to be internal and unrelated to this PR. This is a trivial change, has been tested and reviewed, and is needed before a toolchain update to musl 1.2.5.

At the moment I have multiple PRs blocked by unrelated CI failures or shortcomings, sitting for several weeks total, with no response from members as to workarounds or fixes. See also openwrt/ubox#4 https://github.com/openwrt/ubox/pull/4 for example.

Any suggestions on how best to proceed @Ansuel https://github.com/Ansuel @dangowrt https://github.com/dangowrt @aparcar https://github.com/aparcar @ynezz https://github.com/ynezz ?

— Reply to this email directly, view it on GitHub https://github.com/openwrt/procd/pull/6#issuecomment-2027958193, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE2ZMQWTQG2XPM7B2WMRQ5DY2ZVOFAVCNFSM6AAAAABEIRCAU6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRXHE2TQMJZGM . You are receiving this because you were mentioned.Message ID: @.***>

ynezz commented 7 months ago

I see that CI checks were just added for procd are are now flagging errors

Indeed, GH CI is really strange, since it seems, that one can't test addition of the CI bits, thus due to that broken pull request was merged, still it should be either reverted or GH actions disabled until this is fixed.

however the failure looks to be internal and unrelated to this PR.

Correct, sorry about that.

dangowrt commented 7 months ago

Please make your branch writable for maintainers so we can rebase it before merging. Or rebase on top of current procd git HEAD yourself and force-push.

guidosarducci commented 7 months ago

Please make your branch writable for maintainers so we can rebase it before merging. Or rebase on top of current procd git HEAD yourself and force-push.

@dangowrt Thanks, I've done both yesterday.

robimarko commented 7 months ago

Thanks! Rebased on top of master and merged!

guidosarducci commented 7 months ago

Thanks, Robert.