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

Rainer Orth
2018-09-19T09:04:44Z

Hi Joel,

> 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.

I've done some more digging myself: here's what I found:

* The MAX_PROC_NAME_SIZE part (done slightly differently) was originally
done by Stefan Teleman when he imported gdb 7.6 into the userland repo
(gdb.procfs.c.patch).

* The change to create_procinfo originated with April Chin when
importing gdb 7.12.1 later (001-fix-proc-name-size.patch).

* I had to make minor adjustments for master to account for my removal
of !NEW_PROC_API

So I'm going to attribute the patch to all three of us ;-)

>> @@ -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.

Indeed, and Solaris procfs doesn't care if the <pid> part contains
additional leading zeros or not.

> 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.

True: I removed support for all other previous users when getting rid of
!NEW_PROC_API, IRIX and Tru64 UNIX support that had long been obsoleted.