-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
pidref: record pidfd inode number in PidRef struct #32872
base: main
Are you sure you want to change the base?
Conversation
Important An -rc1 tag has been created and a release is being prepared, so please note that PRs introducing new features and APIs will be held back until the new version has been released. |
src/basic/pidref.c
Outdated
return r != 0; | ||
PidRef *p; | ||
FOREACH_ARGUMENT(p, a, b) { | ||
r = pidref_acquire_pidfd_id(p); |
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.
i'd really prefer if we acquire the ID right-away when we initialize the pidref, and not delay it.
yes, this might mean we do a syscall more initially, but i think that should be ok
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.
So my idea is that we should be able to construct/copy pidrefs freely, and that assumption would break if we now assume the ino is available right away. Also, pidref_equal
is not called that frequently, and thus there's no need to fstat()
each pidfd we acquired.
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.
E.g. 7072777 changed our serialization logic to send both pid and pidfd, and construct a pidref locally on deserialization. And I don't want to needlessly modify that.
@@ -7,6 +7,8 @@ | |||
#include "stdio-util.h" | |||
#include "tests.h" | |||
|
|||
#define PIDREF_NULL_NONCONST (PidRef) { .fd = -EBADF } |
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.
if you acquire the id right-away you can drop this too
we definitely want this, but I think we should really read the id asap, and not delay it until use. seems an unnecessary optimization to me, that more likely creates problem then helps things. |
Also, correct the comment about pidfs (added in kernel 6.9 rather than 6.8). Co-authored-by: Lennart Poettering <lennart@poettering.net>
Besides internal comparisons, the inode number of pidfds might be interesting directly to users, too. In the future this field should also be exposed, so that it can serve as a unique identifier of a process (but only for display, as there's no method to map this back to a pid or pidfd).
Besides internal comparisons, the inode number of pidfds might be interesting directly to users, too. In the future this field should also be exposed, so that it can serve as a unique identifier of a process (but only for display, as there's no method to map this back to a pid or pidfd).
Also, correct the comment about pidfs (added in kernel 6.9 rather than 6.8).