Message ID | yddlg80nrhk.fsf@CeBiTec.Uni-Bielefeld.DE |
---|---|
State | New, archived |
Headers |
Received: (qmail 68063 invoked by alias); 17 Sep 2018 14:11:43 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 68047 invoked by uid 89); 17 Sep 2018 14:11:43 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-19.8 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY autolearn=ham version=3.3.2 spammy=unnecessarily, sprintf, procfsc, procfs.c X-HELO: smtp.CeBiTec.Uni-Bielefeld.DE Received: from smtp.CeBiTec.Uni-Bielefeld.DE (HELO smtp.CeBiTec.Uni-Bielefeld.DE) (129.70.160.84) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 17 Sep 2018 14:11:41 +0000 Received: from localhost (localhost.CeBiTec.Uni-Bielefeld.DE [127.0.0.1]) by smtp.CeBiTec.Uni-Bielefeld.DE (Postfix) with ESMTP id D6D6CCD6 for <gdb-patches@sourceware.org>; Mon, 17 Sep 2018 16:11:38 +0200 (CEST) Received: from smtp.CeBiTec.Uni-Bielefeld.DE ([127.0.0.1]) by localhost (malfoy.CeBiTec.Uni-Bielefeld.DE [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 5-D6m-FIRQeh for <gdb-patches@sourceware.org>; Mon, 17 Sep 2018 16:11:36 +0200 (CEST) Received: from lokon.CeBiTec.Uni-Bielefeld.DE (lokon.CeBiTec.Uni-Bielefeld.DE [129.70.161.152]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by smtp.CeBiTec.Uni-Bielefeld.DE (Postfix) with ESMTPS id 66ABBCD5 for <gdb-patches@sourceware.org>; Mon, 17 Sep 2018 16:11:36 +0200 (CEST) Received: (from ro@localhost) by lokon.CeBiTec.Uni-Bielefeld.DE (8.15.2+Sun/8.15.2/Submit) id w8HEBaCV018660; Mon, 17 Sep 2018 16:11:36 +0200 (MEST) From: Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> To: gdb-patches@sourceware.org Subject: [PATCH] Fix /proc pathname sizes on Solaris Date: Mon, 17 Sep 2018 16:11:35 +0200 Message-ID: <yddlg80nrhk.fsf@CeBiTec.Uni-Bielefeld.DE> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (usg-unix-v) MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-IsSubscribed: yes |
Commit Message
Rainer Orth
Sept. 17, 2018, 2:11 p.m. UTC
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. * 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? Tested on i386-pc-solaris2.11. Ok for master? Rainer
Comments
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; > }
Hi Joel, >> * 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'll try, but she's left Oracle since (mail just bounced). Maybe I can figure it out nonetheless through different contacts. Rainer
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. Rainer
>>>>> "Rainer" == Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes:
Rainer> - sprintf (pi->pathname, "/proc/%05d/lwp/%d", pid, tid);
Rainer> + sprintf (pi->pathname, "/proc/%d/lwp/%d", pid, tid);
FWIW gdb has xsnprintf which will check the length. Or I suppose it
wouldn't be hard to just change this to be a std::string and not worry
about lengths.
Tom
> 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 ;-) Sounds good :). > >> @@ -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. Excellent. An extra confirmation is always nice.
Hi Tom, >>>>>> "Rainer" == Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> writes: > > Rainer> - sprintf (pi->pathname, "/proc/%05d/lwp/%d", pid, tid); > Rainer> + sprintf (pi->pathname, "/proc/%d/lwp/%d", pid, tid); > > FWIW gdb has xsnprintf which will check the length. Or I suppose it > wouldn't be hard to just change this to be a std::string and not worry > about lengths. ah, good point. I'd forgotten about that here, but it's already fixed in a largish procfs.c cleanup patch which gets rids of tons of ARI warnings among others. Currently retesting... Rainer
On 09/17/2018 03:11 PM, Rainer Orth wrote: > * AFAIK Oracle has a corporate copyright assignment on file, so the > patches should be covered. It is my understanding that an assignment alone is not sufficient. The patches should be willfully contributed somehow too. The most common form is by the author or representative contributing or disclaiming it on the list of course. > Even if that were not the case, this one > and the next are certainly below the 15-line limit for non-trivial > patches. That certainly helps. Thanks, Pedro Alves
Hi Pedro, > On 09/17/2018 03:11 PM, Rainer Orth wrote: >> * AFAIK Oracle has a corporate copyright assignment on file, so the >> patches should be covered. > > It is my understanding that an assignment alone is not sufficient. > The patches should be willfully contributed somehow too. The most > common form is by the author or representative contributing or > disclaiming it on the list of course. I wasn't aware of that. In case any of the remaining patches from the solaris-userland repo prove fit for submission, I'll check how this can be done, given that the authors are no longer with Oracle. >> Even if that were not the case, this one >> and the next are certainly below the 15-line limit for non-trivial >> patches. > > That certainly helps. On top of that, both patches clearly document the intent already: $ head 001-fix-proc-name-size.patch # # Fix the size of the pathname for /proc files. # 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. # Patch will be submitted upstream. $ head 007-solib-svr4.patch # Patch required for Solaris. # Will contribute upstream if possible. Rainer
# 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); pi->next = parent->thread_list; parent->thread_list = pi; }