pidref: record pidfd inode number in PidRef struct #32872
Conversation
github-actions
bot
added
util-lib
please-review
|
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. |
DaanDeMeyer
added
ci-fails/needs-rework 🔥
github-actions
bot
added
please-review
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.
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.
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.
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.
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. |
poettering
added
reviewed/needs-rework 🔨
github-actions
bot
added
please-review
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).