Skip to content

Conversation

alejandro-colomar
Copy link
Collaborator

@alejandro-colomar alejandro-colomar commented Jun 4, 2025

Print it with "%d". Also, we need to adapt the size of the allocation. To avoid making mistakes, just make it PATH_MAX, and let the compiler optimize that if it's smart enough.

This is a cherry-pick from

I cherry-picked it, because I want to have it in soon, because of #1222 (comment). I'm blocking a PR from @sthibaul due to this, and I'd like to solve that.


Revisions:

v2
  • Acked-by: @lslebodn
  • Split in two commits.
  • Add missing include.
$ git rd 
-:  -------- > 1:  479e7cfd lib/get_pid.c: Use PATH_MAX instead of a magic number
1:  1c08d6a8 ! 2:  7536a0ae lib/get_pid.c: pid_t is a signed integer
    @@ Metadata
      ## Commit message ##
         lib/get_pid.c: pid_t is a signed integer
     
    -    Print it with "%d".  Also, we need to adapt the size of the allocation.
    -    To avoid making mistakes, just make it PATH_MAX, and let the compiler
    -    optimize that if it's smart enough.
    +    Print it with "%d".
     
    +    Acked-by: Lukas Slebodnik <lslebodn@fedoraproject.org>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## lib/get_pid.c ##
    -@@ lib/get_pid.c: int get_pidfd_from_fd(const char *pidfdstr)
    - int open_pidfd(const char *pidstr)
    - {
    -   int    proc_dir_fd;
    --  char   proc_dir_name[32];
    -+  char   proc_dir_name[PATH_MAX];
    -   pid_t  target;
    - 
    +@@ lib/get_pid.c: int open_pidfd(const char *pidstr)
        if (get_pid(pidstr, &target) == -1)
                return -ENOENT;
      
    --  /* max string length is 6 + 10 + 1 + 1 = 18, allocate 32 bytes */
     -  if (SNPRINTF(proc_dir_name, "/proc/%u/", target) == -1) {
     -          fprintf(stderr, "snprintf of proc path failed for %u: %s\n",
     +  if (SNPRINTF(proc_dir_name, "/proc/%d/", target) == -1) {
$ git diff gh/pid..pid 
diff --git a/lib/get_pid.c b/lib/get_pid.c
index 27e0899d..c687eab0 100644
--- a/lib/get_pid.c
+++ b/lib/get_pid.c
@@ -5,15 +5,14 @@
 
 #include <config.h>
 
-#ident "$Id$"
-
-#include "prototypes.h"
-#include "defines.h"
+#include <limits.h>
+#include <fcntl.h>
 #include <sys/types.h>
 #include <sys/stat.h>
-#include <fcntl.h>
 
 #include "atoi/getnum.h"
+#include "defines.h"
+#include "prototypes.h"
 #include "string/sprintf/snprintf.h"
 
 

@alejandro-colomar alejandro-colomar marked this pull request as ready for review June 4, 2025 11:08
@lslebodn
Copy link

lslebodn commented Jun 6, 2025

One should ideally do just one change in the commit. 2 commits would be much better.

Otherwise LGTM

@alejandro-colomar
Copy link
Collaborator Author

One should ideally do just one change in the commit. 2 commits would be much better.

Otherwise LGTM

Hmmm, yeah, I can split it in two.

Acked-by: Lukas Slebodnik <lslebodn@fedoraproject.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Print it with "%d".

Acked-by: Lukas Slebodnik <lslebodn@fedoraproject.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
@alejandro-colomar
Copy link
Collaborator Author

@lslebodn I have taken your comment as an Acked-by. And have split the PR into two commits.

Thanks!

@hallyn hallyn merged commit 983a866 into shadow-maint:master Jun 7, 2025
10 checks passed
@alejandro-colomar alejandro-colomar deleted the pid branch June 7, 2025 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants