From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk1-x72e.google.com (mail-qk1-x72e.google.com [IPv6:2607:f8b0:4864:20::72e]) by sourceware.org (Postfix) with ESMTPS id E35A5385E454 for ; Tue, 4 May 2021 16:37:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org E35A5385E454 Received: by mail-qk1-x72e.google.com with SMTP id i17so9173195qki.3 for ; Tue, 04 May 2021 09:37:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=6gQJ+BhJvoxjYguOFXKNdyG3S6Dxkt7cubo6W5kihvw=; b=XNNVH5i3D07/jNCeJKbTP/Ts4ptbsY38ZTg2fXo6G6pOdQu2xdVn1TtwvR5xpNDNfr x11rhbhnSy1mrCx0znaH2ZaSy5DLxykzWp8VDtxSgAz2dfNmzqB2cwGSvDhoLDXisn0A u8k9dzhbrGBhOys+pxZS23BgkVtjaLCoFRx0YTp598Nm+RK/BMEK26QFeCD102nwI7rB E/KBoF/0aAbn76Cx6pyGOjVwG9W8hABXBu/f8UGUoVA3krAzsQqr+QKYYQem/SmikOud zcuPwx7oTXidPoBrXJzHu8KiTJvCg04Sheawp+hhOaG7ymMjHZSrRKjW02prI/8wEuEc hE9A== X-Gm-Message-State: AOAM532oErrpgLm65B9GV/MtvPHMRFkqdm5eyx/5JHK4n8RPPKxwdTOC Wcaqekt2/dIoEhr8hLVc5DetyecK6yvjFw== X-Google-Smtp-Source: ABdhPJxfI1GI2IuJ8npM6Mr80cZIX5uhf0SQn/wgqf/cfPJVqnormWxCo7C6f1x6PC9Q3KO4MNB/MA== X-Received: by 2002:a37:9982:: with SMTP id b124mr26144816qke.365.1620146252338; Tue, 04 May 2021 09:37:32 -0700 (PDT) Received: from [192.168.1.4] ([177.194.37.86]) by smtp.gmail.com with ESMTPSA id y84sm7618776qkb.134.2021.05.04.09.37.31 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 04 May 2021 09:37:32 -0700 (PDT) Subject: Re: [PATCH 3/3] linux: implement ttyname as a wrapper around ttyname_r. To: libc-alpha@sourceware.org, =?UTF-8?Q?=c3=89rico_Nogueira?= References: <20210504015152.31064-1-ericonr@disroot.org> <20210504015152.31064-3-ericonr@disroot.org> From: Adhemerval Zanella Message-ID: Date: Tue, 4 May 2021 13:37:27 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: <20210504015152.31064-3-ericonr@disroot.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-12.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 04 May 2021 16:37:34 -0000 On 03/05/2021 22:51, Érico Nogueira via Libc-alpha wrote: > Big win in binary size and avoids duplicating the logic in multiple > places. > > On x86_64, dropped from 1883206 to 1881790, a 1416 byte decrease. > > Also changed logic to track if ttyname_buf has been allocated by > checking if it's NULL instead of tracking buflen as an additional > variable. > --- > > I was going to simply change the function to use __fd_to_filename, like > the 2/3 patch, but this felt like a better improvement. However, I > haven't studied the implications deeply or verified if the ttyname() > logic behaved noticeably (or subtly) different than what ttyname_r() > did. > > sysdeps/unix/sysv/linux/ttyname.c | 161 ++---------------------------- > 1 file changed, 9 insertions(+), 152 deletions(-) > > diff --git a/sysdeps/unix/sysv/linux/ttyname.c b/sysdeps/unix/sysv/linux/ttyname.c > index d561ac12f2..a2bb81c78b 100644 > --- a/sysdeps/unix/sysv/linux/ttyname.c > +++ b/sysdeps/unix/sysv/linux/ttyname.c > @@ -17,17 +17,9 @@ > > #include > #include > -#include > -#include > -#include > -#include > #include > -#include > -#include > #include > > -#include <_itoa.h> > - > #include "ttyname.h" > > #if 0 > @@ -35,170 +27,35 @@ > char *__ttyname; > #endif Maybe remove this dead code as well. > > -static char *getttyname (const char *dev, const struct stat64 *mytty, > - int save, int *dostat); > - > -libc_freeres_ptr (static char *getttyname_name); > - > -static char * > -attribute_compat_text_section > -getttyname (const char *dev, const struct stat64 *mytty, int save, int *dostat) > -{ > - static size_t namelen; > - struct stat64 st; > - DIR *dirstream; > - struct dirent64 *d; > - size_t devlen = strlen (dev) + 1; > - > - dirstream = __opendir (dev); > - if (dirstream == NULL) > - { > - *dostat = -1; > - return NULL; > - } > - > - /* Prepare for the loop. If we already have a buffer copy the directory > - name we look at into it. */ > - if (devlen < namelen) > - *((char *) __mempcpy (getttyname_name, dev, devlen - 1)) = '/'; > - > - while ((d = __readdir64 (dirstream)) != NULL) > - if ((d->d_fileno == mytty->st_ino || *dostat) > - && strcmp (d->d_name, "stdin") > - && strcmp (d->d_name, "stdout") > - && strcmp (d->d_name, "stderr")) > - { > - size_t dlen = _D_ALLOC_NAMLEN (d); > - if (devlen + dlen > namelen) > - { > - free (getttyname_name); > - namelen = 2 * (devlen + dlen); /* Big enough. */ > - getttyname_name = malloc (namelen); > - if (! getttyname_name) > - { > - *dostat = -1; > - /* Perhaps it helps to free the directory stream buffer. */ > - (void) __closedir (dirstream); > - return NULL; > - } > - *((char *) __mempcpy (getttyname_name, dev, devlen - 1)) = '/'; > - } > - memcpy (&getttyname_name[devlen], d->d_name, dlen); > - if (__stat64 (getttyname_name, &st) == 0 > - && is_mytty (mytty, &st)) > - { > - (void) __closedir (dirstream); > -#if 0 > - __ttyname = getttyname_name; > -#endif > - __set_errno (save); > - return getttyname_name; > - } > - } > - > - (void) __closedir (dirstream); > - __set_errno (save); > - return NULL; > -} > - > - > +#define TTYNAME_BUFLEN (PATH_MAX-1) > /* Static buffer in `ttyname'. */ > libc_freeres_ptr (static char *ttyname_buf); Although not really related to this change, since you are touching it I think it should use a libc_freeres_fn instead: | static char *ttyname_buf; | | libc_freeres_fn (free_mem) | { | free (ttyname_buf); | } > > - > /* Return the pathname of the terminal FD is open on, or NULL on errors. > The returned storage is good only until the next call to this function. */ > char * > ttyname (int fd) > { > - static size_t buflen; > - char procname[30]; > - struct stat64 st, st1; > - int dostat = 0; > - int doispty = 0; > - char *name; > - int save = errno; > - struct termios term; > - > /* isatty check, tcgetattr is used because it sets the correct > - errno (EBADF resp. ENOTTY) on error. */ > + errno (EBADF resp. ENOTTY) on error. Fast error path to avoid the allocation */ Double space after period and line too long. > + struct termios term; > if (__glibc_unlikely (__tcgetattr (fd, &term) < 0)) > return NULL; > > - if (__fstat64 (fd, &st) < 0) > - return NULL; > - > - /* We try using the /proc filesystem. */ > - *_fitoa_word (fd, __stpcpy (procname, "/proc/self/fd/"), 10, 0) = '\0'; > - > - if (buflen == 0) > + if (ttyname_buf == NULL) > { > - buflen = 4095; > - ttyname_buf = (char *) malloc (buflen + 1); > + ttyname_buf = malloc (TTYNAME_BUFLEN + 1); > if (ttyname_buf == NULL) > { > - buflen = 0; > return NULL; > } > } > Ok. > - ssize_t len = __readlink (procname, ttyname_buf, buflen); > - if (__glibc_likely (len != -1)) > - { > - if ((size_t) len >= buflen) > - return NULL; > - > -#define UNREACHABLE_LEN strlen ("(unreachable)") > - if (len > UNREACHABLE_LEN > - && memcmp (ttyname_buf, "(unreachable)", UNREACHABLE_LEN) == 0) > - { > - memmove (ttyname_buf, ttyname_buf + UNREACHABLE_LEN, > - len - UNREACHABLE_LEN); > - len -= UNREACHABLE_LEN; > - } > - > - /* readlink need not terminate the string. */ > - ttyname_buf[len] = '\0'; > - > - /* Verify readlink result, fall back on iterating through devices. */ > - if (ttyname_buf[0] == '/' > - && __stat64 (ttyname_buf, &st1) == 0 > - && is_mytty (&st, &st1)) > - return ttyname_buf; > - > - doispty = 1; > - } > - > - if (__stat64 ("/dev/pts", &st1) == 0 && S_ISDIR (st1.st_mode)) > - { > - name = getttyname ("/dev/pts", &st, save, &dostat); > - } > - else > - { > - __set_errno (save); > - name = NULL; > - } > - > - if (!name && dostat != -1) > + int result = ttyname_r (fd, ttyname_buf, TTYNAME_BUFLEN); > + if (result != 0) Why do you need to pass the allocated buffer size minus 1? The ttyname_r should handle it, it already pass buflen - 1 on readlink for instance. > { > - name = getttyname ("/dev", &st, save, &dostat); > - } > - > - if (!name && dostat != -1) > - { > - dostat = 1; > - name = getttyname ("/dev", &st, save, &dostat); > - } > - > - if (!name && doispty && is_pty (&st)) > - { > - /* We failed to figure out the TTY's name, but we can at least > - signal that we did verify that it really is a PTY slave. > - This happens when we have inherited the file descriptor from > - a different mount namespace. */ > - __set_errno (ENODEV); > + __set_errno (result); > return NULL; > } > - > - return name; > + return ttyname_buf; > } > With this change ttyname now returns ERANGE instead of ENAMETOOLONG if readlink returns a link larger than the specified buffer, but I think it should be ok.