tmpfiles: refactor into separate executable #439

aanderse · 2025-05-24T19:34:22Z

this PR is an attempt to resolve #423

i thought i would open this PR early for more concrete discussion and feedback - i don't want to end up too far down a path which you aren't interested in merging

i am proposing the following for this PR:

simply split tmpfiles into its own binary under libexec/
add --create and --remove args to the program so we can run only the desired behaviour (r and R rules will execute when --remove is passed to the program, the rest when --create is)

this sets us up for future PR(s) where more useful functionality can be fleshed out, including the --clean flag for time based cleanup

@troglobit
Yeah a tmpfilesd or similar would be neat for this. Then we could relocate all the current code to that, and call it with an --init flag or something at bootstrap, before we start it as a proper service.

Originally posted by @troglobit in #423

regarding tmpfiles as a service... i'm not sure how useful that actually is - the main reason new tmpfile configs are added to the system is part of package/system management hooks - but of course there is still the cleanup aspect... given that there is no cleanup currently implemented, i don't think that is a strong argument for a service

what do you think? am i heading in a direction you would be willing to merge? thanks again for your time and consideration 🙇‍♂️

src/tmpfiles.c

troglobit

Looks good so far, nice work! 👍

Please elaborate a little more on command line options though. You mention --create, which I was sort of expecting in the call in bootmisc.c. A basic usage text would be sufficient.

aanderse · 2025-05-25T16:29:43Z

i would like to squash the second commit into the first, but i think you probably have strong opinions about the second commit... so i'll let you comment on that first
i noticed there was code duplication in src/runparts.c... at least one function there now exists in the new src/misc.c, so that could be removed in a future PR
i haven't booted into this to test it yet

hopefully this is in the right direction... looking forward to feedback

thanks

troglobit · 2025-05-25T19:30:17Z

i would like to squash the second commit into the first, but i think you probably have strong opinions about the second commit... so i'll let you comment on that first

You're right, I have a lot of opinions about that commit 😄 I'll try to be harsh but fair:

I do not like moving out all that code from exec.c. Related code should go in the same file, or it will be very difficult to keep track of where it all is.
Why do we have to create misc.c? Why can't the new application link against the same source files as Finit? Sure not all functions are used, but dead code is usually something the linker can clean up.
We already have util.c, helpers.c, giving it the name misc.c just adds to a mess that might need to be untangled in another way. In the example of of moving out sig_unblock() (which I don't like either) one could argue that generic signal code that can be shared should go in a sig-common.c instead. Similarly for the functions from exec.c, could be moved to a exec-common.c

In summary, please explain to me why tmpfiles_SOURCES cannot include exec.c helpers.c sig.c util.c instead.

Also, your commit messages need to tell the story of Why a change is made, reasoning with a future reader, explaining the intent.

* i noticed there was code duplication in `src/runparts.c`... at least one function there now exists in the new `src/misc.c`, so that could be removed in a future PR

OK, nice find.

* i haven't booted into this to test it yet

For future reference, and as a matter of process, please keep PRs as draft until you've verified your changes.

hopefully this is in the right direction... looking forward to feedback

Definitely in the right direction, great work so far!

aanderse · 2025-05-25T21:16:08Z

I do not like moving out all that code from exec.c. Related code should go in the same file, or it will be very difficult to keep track of where it all is.

Why do we have to create misc.c? Why can't the new application link against the same source files as Finit? Sure not all functions are used, but dead code is usually something the linker can clean up.

We already have util.c, helpers.c, giving it the name misc.c just adds to a mess that might need to be untangled in another way. In the example of of moving out sig_unblock() (which I don't like either) one could argue that generic signal code that can be shared should go in a sig-common.c instead. Similarly for the functions from exec.c, could be moved to a exec-common.c

In summary, please explain to me why tmpfiles_SOURCES cannot include exec.c helpers.c sig.c util.c instead.

this was not my original intention - i also wanted to link existing .c files. i guess i misunderstood your prior comments because this seemed off to me too. and you maybe you misunderstood the problems i was having. i will come up with a summary of the problems i had and be very explicit because we aren't understanding each other properly.

Definitely in the right direction, great work so far!

thanks for the feedback - more to come

src/tmpfiles.c

aanderse · 2025-05-30T11:27:35Z

@troglobit i loaded this branch of finit onto my system last night - looks like it is working as expected:

i saw the expected message at boot about creating directories and files
a few test rules i added before reboot were run as expected
manually tested a few different types and executed --create, --remove, as well as --create --remove together

when you get a chance please let me know how this looks... thank you!

troglobit · 2025-05-30T16:15:32Z

[snip]
when you get a chance please let me know how this looks... thank you!

Awesome, I'll have a look over the weekend, cheers!

troglobit

Looks much better, nice work!

You can squash the first two commits, but the last one needs to be split up so you do any refactoring/reshuffle of code in a separate commit from where you add --create and --remove flags. Also, the first line of that commit wraps, try to keep a brief first line of the commit, e.g., "tmpfiles: add --create and --remove flags" is sufficient, the rest of the text can go in the body.

The tmpfiles.d spec has proven useful in systemd, enough so that some developers are starting to ship tmpfiles.d configuration files with their software. By splitting the tmpfiles functionality in finit out into a separate executable we are providing a useful piece of software that package managers can hook into.

The tmpfiles.d spec contains entry types that should be acted upon in different modes of operation: create, clean, remove, and purge - split up create & remove logic to clarify the modes of operation finit supports.

Allow execution of tmpfiles to specify which mode(s) of operation to run in.

> D > Similar to d, but in addition the contents of the directory will be removed when --remove is used.

aanderse · 2025-06-02T23:44:54Z

@troglobit i think the second commit accomplishes what you want to see here

don't hesitate to let me know if there are any changes i can make so you're completely happy with this PR... your code quality is great, so i really don't mind nitpicks from you 🙇‍♂️

troglobit

This looks much better, very nice!

My comments on memory usage could be considered theoretical, but I've learned that it's always best to keep it clean because you never know how code transforms or where it is used. An esoteric use-case is running Finit on uClinux systems (CPUs without MMU), there Linux would not clean up and free memory a process has not freed itself.

src/tmpfiles.c

The existing implementation seemed useful enough to warrant a new helper function. Care was taken to ensure the revised code no longer suffers any memory leaks.

troglobit · 2025-06-07T05:15:48Z

🥇 Great work on this, time to squash some of the commits and mark this as Ready for review 🏁

aanderse · 2025-06-07T09:56:15Z

awesome! thanks for all your time and patience on this!

previously you mentioned that you like commits to be well defined logically small changes, which sounds good - i feel that's what we have here... so please let me know explicitly what you want and i am happy to comply

troglobit · 2025-06-07T18:17:19Z

awesome! thanks for all your time and patience on this!

No problem at all!

previously you mentioned that you like commits to be well defined logically small changes, which sounds good - i feel that's what we have here... so please let me know explicitly what you want and i am happy to comply

Yeah, so logical commits are just that: "whitespace changes only", "drop foo functionality, not used anymore", "add bar to baz, extending xyzzy", just cleanly separating what removes, relocates, and what adds new functionality. Think; "what if I had to debug these changes myself", what would you like to see. Also, there's the art of bisectability, i.e., each change builds and is testable on its own. This latter one I struggle with myself.

The most important thing, apart from commit messages telling why a change is made, is to keep changes logical. So, when you look at a series of patches/commits you should be able to, at some degree, trust the submitter. So when they say, "whitespace changes only", you can skip that commit at the beginning and focus on removals and additions. It's all about making it easier for the reviewer, and yourself a few weeks/months/years down the line ...

aanderse · 2025-06-08T19:55:50Z

i have been running this for a bit on my system and i think it is ready for a final review

troglobit

This looks great, and the tests pass, so time to merge!

Awesome job on this, @aanderse ! 💯 🥇

aanderse · 2025-06-09T10:05:59Z

thanks again!

aanderse mentioned this pull request May 24, 2025

tmpfiles: initctl command or native cleanup-age time #423

Closed

aanderse commented May 24, 2025

View reviewed changes

src/tmpfiles.c Outdated Show resolved Hide resolved

aanderse commented May 24, 2025

View reviewed changes

src/tmpfiles.c Outdated Show resolved Hide resolved

troglobit reviewed May 25, 2025

View reviewed changes

aanderse force-pushed the tmpfiles branch 2 times, most recently from ef667b7 to cdff3e4 Compare May 25, 2025 16:20

aanderse marked this pull request as ready for review May 25, 2025 16:33

aanderse marked this pull request as draft May 25, 2025 21:10

troglobit added the enhancement label May 25, 2025

troglobit added this to the 4.13 milestone May 25, 2025

aanderse force-pushed the tmpfiles branch from cdff3e4 to 7553b01 Compare May 25, 2025 22:24

aanderse commented May 25, 2025

View reviewed changes

src/tmpfiles.c Show resolved Hide resolved

aanderse force-pushed the tmpfiles branch from 7553b01 to b7e2ab8 Compare May 29, 2025 22:17

troglobit requested changes Jun 1, 2025

View reviewed changes

aanderse added 4 commits June 2, 2025 19:27

tmpfiles: split up create & remove logic

ff95ebe

The tmpfiles.d spec contains entry types that should be acted upon in different modes of operation: create, clean, remove, and purge - split up create & remove logic to clarify the modes of operation finit supports.

tmpfiles: add --create and --remove flags

a571012

Allow execution of tmpfiles to specify which mode(s) of operation to run in.

tmpfiles: implement remove logic for 'D'

2957246

> D > Similar to d, but in addition the contents of the directory will be removed when --remove is used.

aanderse force-pushed the tmpfiles branch from b7e2ab8 to 2957246 Compare June 2, 2025 23:32

troglobit requested changes Jun 5, 2025

View reviewed changes

src/tmpfiles.c Outdated Show resolved Hide resolved

src/tmpfiles.c Show resolved Hide resolved

tmpfiles: refactor code into new is_dir_empty function

5993b4b

The existing implementation seemed useful enough to warrant a new helper function. Care was taken to ensure the revised code no longer suffers any memory leaks.

aanderse force-pushed the tmpfiles branch from aa95f0b to 5993b4b Compare June 6, 2025 00:53

aanderse marked this pull request as ready for review June 8, 2025 19:55

troglobit approved these changes Jun 9, 2025

View reviewed changes

troglobit merged commit 8be7a46 into finit-project:master Jun 9, 2025
2 checks passed

aanderse deleted the tmpfiles branch June 9, 2025 10:05

Uh oh!

tmpfiles: refactor into separate executable #439

tmpfiles: refactor into separate executable #439

Uh oh!

Conversation

aanderse commented May 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

troglobit left a comment

Choose a reason for hiding this comment

Uh oh!

aanderse commented May 25, 2025

Uh oh!

troglobit commented May 25, 2025

Uh oh!

aanderse commented May 25, 2025

Uh oh!

Uh oh!

aanderse commented May 30, 2025

Uh oh!

troglobit commented May 30, 2025

Uh oh!

troglobit left a comment

Choose a reason for hiding this comment

Uh oh!

aanderse commented Jun 2, 2025

Uh oh!

troglobit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

troglobit commented Jun 7, 2025

Uh oh!

aanderse commented Jun 7, 2025

Uh oh!

troglobit commented Jun 7, 2025

Uh oh!

aanderse commented Jun 8, 2025

Uh oh!

troglobit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aanderse commented Jun 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aanderse commented May 24, 2025 •

edited

Loading