Skip to content

core: fix logic of merging units - #25387

Merged
bluca merged 7 commits into
systemd:mainfrom
yuwata:core-fix-gc-logic
Dec 17, 2022
Merged

core: fix logic of merging units#25387
bluca merged 7 commits into
systemd:mainfrom
yuwata:core-fix-gc-logic

Conversation

@yuwata

@yuwata yuwata commented Nov 15, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Fixes several bugs introduced by 15ed3c3 (v249).

Fixes #24990. Also, hopefully fixes #24577.

@yuwata yuwata linked an issue Nov 15, 2022 that may be closed by this pull request
init: Assertion 'hashmap_isempty(m->jobs)' failed at src/core/manager.c:1474, function manager_clear_jobs_and_units(). Aborting. #24577
Closed
@yuwata yuwata mentioned this pull request Nov 15, 2022
Closed
@yuwata yuwata added this to the v253 milestone Nov 15, 2022
@yuwata
yuwata requested a review from poettering November 15, 2022 14:19
@yuwata yuwata added the ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR label Nov 15, 2022
@yuwata yuwata removed the ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR label Nov 15, 2022
@yuwata yuwata changed the title core: fix unit GC logic core: fix unit garbage collection and merging units logic Nov 15, 2022
@yuwata

yuwata commented Nov 15, 2022

Copy link
Copy Markdown
Member Author

(Hm, still GC logic needs to be updated.)

@yuwata yuwata changed the title core: fix unit garbage collection and merging units logic core: fix logic of merging units Nov 15, 2022
@yuwata

yuwata commented Nov 15, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

The commit for unit GC is dropped. PTAL.

@yuwata yuwata mentioned this pull request Nov 17, 2022
Open
@evverx evverx mentioned this pull request Nov 27, 2022
Closed
keszybz
keszybz requested changes Dec 2, 2022
View reviewed changes
Comment thread src/core/unit.c Outdated
Comment thread test/units/testsuite-26.sh
Comment thread test/units/testsuite-26.sh Outdated
Comment thread test/units/testsuite-26.sh
Comment thread test/units/testsuite-26.sh Outdated
Comment thread src/core/unit.c Outdated
@keszybz keszybz added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Dec 2, 2022
@yuwata
yuwata force-pushed the core-fix-gc-logic branch from f90143e to 2e30309 Compare December 5, 2022 05:31
@yuwata yuwata removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Dec 5, 2022
@yuwata

yuwata commented Dec 5, 2022

Copy link
Copy Markdown
Member Author

@keszybz Thank you for the review. All points, except for the bashism in the test, are addressed. PTAL!

keszybz
keszybz reviewed Dec 5, 2022
View reviewed changes
Comment thread src/core/unit.c
yuwata added 2 commits December 6, 2022 17:49
@yuwata
test: add test case for sysv-generator and invalid dependency
5f882cc
@yuwata
core/unit: merge unit names after merging deps
1d0c81a 
Before:
systemd[1]: issue-24990.service: Dependency Before=n/a dropped, merged into issue-24990.service
After:
systemd[1]: issue-24990.service: Dependency Before=test1.service dropped, merged into issue-24990.service
@yuwata
yuwata force-pushed the core-fix-gc-logic branch from 2e30309 to 4f41ccd Compare December 6, 2022 08:59
@keszybz keszybz added the please-review PR is ready for (re-)review by a maintainer label Dec 8, 2022
keszybz
keszybz reviewed Dec 15, 2022
View reviewed changes

@keszybz keszybz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LaGTM. (almost good)

Comment thread src/core/unit.c Outdated
Comment thread src/core/unit.c Outdated
Comment thread src/core/unit.c Outdated
@keszybz keszybz added good-to-merge/with-minor-suggestions and removed please-review PR is ready for (re-)review by a maintainer labels Dec 15, 2022
@yuwata
core/unit: fix log message
ed99116 
As you can see in the below, the dropped dependency Before=issue-24990.service
is not logged, but the dependency Before=test1.service which is not owned by
the units generated by the TEST-26 is logged.

Before:
systemd[1]: issue-24990.service: Dependency After=test1.service dropped, merged into issue-24990.service
systemd[1]: issue-24990.service: Dependency Before=test1.service dropped, merged into issue-24990.service

After:
systemd[1]: issue-24990.service: Dependency After=test1.service is dropped, as test1.service is merged into issue-24990.service.
systemd[1]: issue-24990.service: Dependency Before=issue-24990.service in test1.service is dropped, as test1.service is merged into issue-24990.service.
@yuwata

yuwata commented Dec 15, 2022

Copy link
Copy Markdown
Member Author

@keszybz Thank you for your review. All comments are addressed. Upgrading the green label.

LaGTM. (almost good)

I like the abbreviation!

@yuwata yuwata added 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 and removed good-to-merge/with-minor-suggestions labels Dec 15, 2022
poettering
poettering reviewed Dec 16, 2022
View reviewed changes
Comment thread src/core/unit.c
/* This is a dependency pointing back to the unit we want to merge with?
* Suppress it (but warn) */
unit_maybe_warn_about_dependency(u, other->id, UNIT_DEPENDENCY_FROM_PTR(dt));
hashmap_remove(other_deps, back);

@poettering poettering Dec 16, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, now you are changing the hashmap you are iterating through. It's something I so far avoided

@poettering

poettering commented Dec 16, 2022

Copy link
Copy Markdown
Member

looks good, but iterating through a hashmap that we modify is problematic, iirc our hashmap implementation is not safe against arbitrary changes during iteration

@poettering poettering 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 Dec 16, 2022
@yuwata

yuwata commented Dec 17, 2022

Copy link
Copy Markdown
Member Author

looks good, but iterating through a hashmap that we modify is problematic, iirc our hashmap implementation is not safe against arbitrary changes during iteration

It is safe. Please see the comment in hashmap_iterate_in_internal_order().

@yuwata yuwata added 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 and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Dec 17, 2022
@bluca
bluca merged commit 896785a into systemd:main Dec 17, 2022
@yuwata
yuwata deleted the core-fix-gc-logic branch December 17, 2022 14:05
@evverx evverx mentioned this pull request Jan 1, 2023
Closed
@keszybz keszybz removed 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 Feb 10, 2023
@keszybz

keszybz commented Feb 10, 2023

Copy link
Copy Markdown
Member

This was already included in v252.4.

@evverx evverx mentioned this pull request Mar 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@keszybz keszybz keszybz requested changes
@poettering poettering poettering left review comments
@bluca bluca bluca approved these changes

Assignees

No one assigned

Labels

pid1

Milestone

v253

4 participants

@yuwata @poettering @keszybz @bluca