Re: [PATCH] Fix /proc pathname sizes on Solaris

[Joel Brobecker <brobecker@adacore.com>]

Hi Rainer,

On Mon, Sep 17, 2018 at 04:11:35PM +0200, Rainer Orth wrote:
> I'm slowly working my way through the gdb patches from the
> solaris-userland repo
> 
> 	https://github.com/oracle/solaris-userland/tree/master/components/gdb/patches
> 
> Some of them are pretty obvious and should be able to go in (such as
> this one and the next), while others are either incomplete
> (e.g. 008-syscalls.patch, which adds XML descriptions of the Solaris
> syscalls, but lacks their registration) or inappropriate in their
> current form (unnecessarily intrusive).
> 
> This one (001-fix-proc-name-size.patch) should be obvious given the
> patches' comment:
> 
> # In Solaris, PID_MAX is 999999 (6 digit pid).
> # In Solaris, lwpid_t is an unsigned int, so theoretically the lwp id
> # could be 10 digits.
> 
> Two questions about procedure here:
> 
> * AFAIK Oracle has a corporate copyright assignment on file, so the
>   patches should be covered.  Even if that were not the case, this one
>   and the next are certainly below the 15-line limit for non-trivial
>   patches.

I checked, and indeed, Oracle has a copyright assignment.

> * Given the code isn't mine, how should we handle attribution?  I
>   suspect the engineer who committed the patch to github is the author,
>   but don't know for certain.  Should I attribute it to her in the
>   ChangeLog?

Can you ask the user in question if they are the author? If not,
can they help figuring out who it is? Ideally, we would want the
name and email of the author of  the patch -- not sure what we should
be doing if we don't have that info.

> Tested on i386-pc-solaris2.11.  Ok for master?
> 
> 	Rainer
> 
> -- 
> -----------------------------------------------------------------------------
> Rainer Orth, Center for Biotechnology, Bielefeld University
> 
> 
> 2018-06-27  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>
> 
> 	* procfs.c (MAX_PROC_NAME_SIZE): Allow for 6-digit PID_MAX and
> 	uint_t lwpid_t.
> 	(create_procinfo): Print pids in /proc without leading zeros.

OK for me.

> # HG changeset patch
> # Parent  e6140f0a7128422be8a7e2a148da8de516d676d8
> Fix /proc pathname sizes on Solaris
> 
> diff --git a/gdb/procfs.c b/gdb/procfs.c
> --- a/gdb/procfs.c
> +++ b/gdb/procfs.c
> @@ -233,7 +233,7 @@ enum { READ_WATCHFLAG  = WA_READ,
>  #define AS_PROC_NAME_FMT     "/proc/%d/as"
>  #define MAP_PROC_NAME_FMT    "/proc/%d/map"
>  #define STATUS_PROC_NAME_FMT "/proc/%d/status"
> -#define MAX_PROC_NAME_SIZE sizeof("/proc/99999/lwp/8096/lstatus")
> +#define MAX_PROC_NAME_SIZE sizeof("/proc/999999/lwp/0123456789/lwpstatus")
>  
>  typedef struct procinfo {
>    struct procinfo *next;
> @@ -483,7 +483,7 @@ create_procinfo (int pid, int tid)
>      }
>    else
>      {
> -      sprintf (pi->pathname, "/proc/%05d/lwp/%d", pid, tid);
> +      sprintf (pi->pathname, "/proc/%d/lwp/%d", pid, tid);

I am wondering how this ever worked for processes whose pid had
fewer than 5 digits. I was initially concerned that this patch
introduced a change of behavior that would create an incompatibility.
But looking at Solaris 2.8 and 2.11 systems, I see processes with
3 or 4 digits PIDs, and the path in /proc doesn't have leading zeroes.

I also checked whether the file might be used on platforms other than
Solaris (see configure.nat), and this does not appear to be the case.

>        pi->next = parent->thread_list;
>        parent->thread_list = pi;
>      }


-- 
Joel