From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/10476 Path: news.gmane.org!.POSTED!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] add pthread_setname_np Date: Fri, 16 Sep 2016 11:00:38 -0400 Message-ID: <20160916150038.GM15995@brightrain.aerifal.cx> References: <20160916002351.GA5913@nyan> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: blaine.gmane.org 1474038072 17767 195.159.176.226 (16 Sep 2016 15:01:12 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Fri, 16 Sep 2016 15:01:12 +0000 (UTC) User-Agent: Mutt/1.5.21 (2010-09-15) To: musl@lists.openwall.com Original-X-From: musl-return-10489-gllmg-musl=m.gmane.org@lists.openwall.com Fri Sep 16 17:01:08 2016 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.84_2) (envelope-from ) id 1bkudH-0002bh-SR for gllmg-musl@m.gmane.org; Fri, 16 Sep 2016 17:00:51 +0200 Original-Received: (qmail 30241 invoked by uid 550); 16 Sep 2016 15:00:52 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 30220 invoked from network); 16 Sep 2016 15:00:51 -0000 Content-Disposition: inline In-Reply-To: Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:10476 Archived-At: On Fri, Sep 16, 2016 at 12:03:16PM +0300, Alexander Monakov wrote: > Hi, > > On Thu, 15 Sep 2016, Felix Janda wrote: > > --- /dev/null > > +++ b/src/thread/pthread_setname_np.c > > @@ -0,0 +1,25 @@ > > +#include > > +#include > > +#include > > +#include > > + > > +#include "pthread_impl.h" > > This should bring in the declaration of pthread_setname_np from pthread.h by > #defin'ing _GNU_SOURCE prior to inclusion. > (today, this rule is not enforced with warnings and thus not always followed in > musl, for example the recent pthread_tryjoin_np patch missed that as well) Yep. > > +int pthread_setname_np(pthread_t thread, const char *name) > > +{ > > + int fd, cs, status = 0; > > + char f[sizeof "/proc/self/task//comm" + 3*sizeof(int)]; > > + size_t len; > > + > > + if ((len = strnlen(name, 16)) > 15) return ERANGE; > > + > > + if (thread == pthread_self()) > > This likely should use the static inline function __pthread_self()? My preference when implementing nonstandard extension/junk functions that are not performance-critical is to write them using nothing but public APIs so that they're musl-agnostic and thereby maintenance-free. Certainly saving a few bytes/cycles for the function call here is not worthwhile so I'd stick with pthread_self(). > > + return prctl(PR_SET_NAME, (unsigned long)name, 0, 0, 0) ? errno : 0; > > First, prctl is declared as a variadic function, so zeros should be passed with > the right type (0ul); passing fewer than 5 arguments will cause musl's > implementation of prctl to invoke UB, since it always retrieves 4 variadic arguments. > > Second, while I don't have a strong opinion on whether musl wants to use prctl > here (my previous comment in this thread was based on the wrong idea that prctl > would be sufficient in all cases - sorry about that), I think some tuning would > be nice in case the decision is to use prctl. So, either > > return -__syscall(SYS_prctl, PR_SET_NAME, name); > > (not sure if Rich would like it), or See above. This would be smaller but if we (or someone else using the code) wanted to change the conventions of how __syscall is used, this would require them to dig info nonstandard functions (which don't _need_ to be using __syscall) in addition to all the standard ones (which might need to do this for namespace reasons, etc.). So I'd prefer just using the public interface. > if (thread == pthread_self()) { > status = prctl(...); > } else { > ... > fd = status = open(f, O_WRONLY); > if (fd >= 0) { > status = write(fd, name, len); > close(fd); > } > pthread_setcancelstate(cs, 0); > } > return status ? errno : 0; > > > + snprintf(f, sizeof f, "/proc/self/task/%d/comm", thread->tid); > > + pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs); > > + if ((fd = open(f, O_WRONLY)) < 0 || write(fd, name, len) < 0) status = errno; > > + if (fd >= 0) close(fd); > > + pthread_setcancelstate(cs, 0); > > + return status; > > +} I don't have a strong opinion on how this part is structured. Rich