-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
malloc_usable_size() tweaks #19653
Merged
Merged
malloc_usable_size() tweaks #19653
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
busctl
journal
journal-remote
network
portable
Anything to do with systemd-portable and portablectl and portables
resolve
systemctl
udev
labels
May 18, 2021
|
yuwata
added
the
ci-fails/needs-rework 🔥
Please rework this, the CI noticed an issue with the PR
label
May 19, 2021
poettering
force-pushed
the
greedy-realloc-more
branch
from
May 19, 2021 13:40
fae427e
to
8b5be96
Compare
poettering
removed
the
ci-fails/needs-rework 🔥
Please rework this, the CI noticed an issue with the PR
label
May 19, 2021
poettering
force-pushed
the
greedy-realloc-more
branch
2 times, most recently
from
May 19, 2021 14:23
9d16390
to
900e170
Compare
yuwata
approved these changes
May 19, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but one minor comment.
It's a wrapper around malloc_usable_size() that is supposed to be compatible with _FORTIFY_SOURCES=1, by taking the __builtin_object_size() data into account, the same way as the _FORTIFY_SOURCES=1 logic does. Fixes: systemd#19203
This is a wrapper around malloc_usable_size() but is typesafe, and divides by the element size. A test it is also added ensuring what it does it does correcly.
…le_size() We recently started making more use of malloc_usable_size() and rely on it (see the string_erase() story). Given that we don't really support sytems where malloc_usable_size() cannot be trusted beyond statistics anyway, let's go fully in and rework GREEDY_REALLOC() on top of it: instead of passing around and maintaining the currenly allocated size everywhere, let's just derive it automatically from malloc_usable_size(). I am mostly after this for the simplicity this brings. It also brings minor efficiency improvements I guess, but things become so much nicer to look at if we can avoid these allocation size variables everywhere. Note that the malloc_usable_size() man page says relying on it wasn't "good programming practice", but I think it does this for reasons that don't apply here: the greedy realloc logic specifically doesn't rely on the returned extra size, beyond the fact that it is equal or larger than what was requested. (This commit was supposed to be a quick patch btw, but apparently we use the greedy realloc stuff quite a bit across the codebase, so this ends up touching *a*lot* of code.)
poettering
force-pushed
the
greedy-realloc-more
branch
from
May 19, 2021 14:45
900e170
to
319a4f4
Compare
fixed the issue @yuwata found. upgrading green label |
poettering
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
May 19, 2021
yuwata
added a commit
to yuwata/systemd
that referenced
this pull request
May 20, 2021
This fixes a conflict between systemd#19555 and systemd#19653.
dakr
pushed a commit
to dakr/systemd
that referenced
this pull request
Jun 14, 2021
This fixes a conflict between systemd#19555 and systemd#19653.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
busctl
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
journal
journal-remote
network
portable
Anything to do with systemd-portable and portablectl and portables
resolve
systemctl
udev
util-lib
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #19203