From patchwork Wed May 13 23:35:52 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Roland McGrath X-Patchwork-Id: 472149 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 834B114016A for ; Thu, 14 May 2015 09:36:07 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=sourceware.org header.i=@sourceware.org header.b=oYC8Dlow; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:mime-version:content-type :content-transfer-encoding:from:to:subject:message-id:date; q= dns; s=default; b=eXTU4I1EgMC9xP5RDfMv2IeQdXKw9SRA4Vde6ilkFudpHa cBQCnv3rKEnx3yqs4SxBilcmmmLInp0ZnPQWdaVjh3sHbS1yAwOZPqbjFJaNmpIb qFJb3thME8fydIYUMU84P0Xxs2PFikld72FlCwt/Qk2Fir1ml4lw9TfMXKRqM= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:mime-version:content-type :content-transfer-encoding:from:to:subject:message-id:date; s= default; bh=BXNr9brkOg/yKpcGl5zudSmyA+g=; b=oYC8DlowJfBbbpE1IyOO ZdEIvSwpiEAtGXC4tM97HKsjKGJyWSl1FEdqfSFgfIrHGLX9/ygoH5QB4MBGibiG ETsRI+7JOPAgincvQc2qfYOPupk4zoJfW/ZStizsqbHzPC85SOHcY3+J9v6PtKLb Uz2W+ajDnU9YNv3mj464al0= Received: (qmail 27377 invoked by alias); 13 May 2015 23:35:59 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 27366 invoked by uid 89); 13 May 2015 23:35:57 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.7 required=5.0 tests=AWL, BAYES_40, KAM_LAZY_DOMAIN_SECURITY autolearn=no version=3.3.2 X-HELO: topped-with-meat.com MIME-Version: 1.0 From: Roland McGrath To: "GNU C. Library" Subject: [PATCH roland/opendir] Refactor opendir. Message-Id: <20150513233552.3BAAB2C3A9B@topped-with-meat.com> Date: Wed, 13 May 2015 16:35:52 -0700 (PDT) X-CMAE-Score: 0 X-CMAE-Analysis: v=2.1 cv=SvUDtp+0 c=1 sm=1 tr=0 a=WkljmVdYkabdwxfqvArNOQ==:117 a=14OXPxybAAAA:8 a=kj9zAlcOel0A:10 a=hOe2yjtxAAAA:8 a=MtU9jjGLKbNy1b0kBKMA:9 a=CjuIK1q_8ugA:10 I've refactored the opendir implementation, with a few effects. The main one (and my motivation) is that opendir no longer calls __opendirat, and so opendir uses only open while only __opendirat uses openat. The other things were incidental but they fix hidden bugs. 1. If O_DIRECTORY is not known to work, then the old code in __opendirat called stat (really __xstat64) on the name without regard to the fd, so for cases other that AT_FDCWD it was stat'ing the wrong thing. I've changed it to use fstatat (really __fxstatat64). GNU/Linux configurations assume O_DIRECTORY works and so don't use this path, while GNU/Hurd doesn't use this code at all. So this did not affect existing configurations (other than NaCl, which doesn't have openat anyway). 2. When O_DIRECTORY does work, then the old code bypassed the fstat (really __fxstat64) call to do the S_ISDIR check. But this also meant that it didn't sample st_blksize, so ignored st_blksize in deciding what buffer size to use. This was a regression from past versions (I didn't look to see when the change was made), and is now fixed. Tested x86_64-linux-gnu (and arm-nacl). If there are no objections, I'll commit this on Monday. Thanks, Roland * sysdeps/posix/opendir.c: Include . (invalid_name): New function, broken out of ... (__opendirat): ... here. Call it. (need_isdir_precheck): New function, broken out of ... (__opendirat): ... here. Call it. Use __fxstatat64, not __xstatat64. (opendir_oflags): New function, broken out of ... (__opendirat): ... here. Call it. (opendir_tail): New function, broken out of ... (__opendirat): ... here. Call it. (__opendir): Call invalid_name, need_isdir_precheck, __xstat64, and opendir_tail, rather than punting to __opendirat. (__opendirat): Conditionalize function definition on [IS_IN (libc)]. diff --git a/sysdeps/posix/opendir.c b/sysdeps/posix/opendir.c index 451c56e..6509f5c 100644 --- a/sysdeps/posix/opendir.c +++ b/sysdeps/posix/opendir.c @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -81,83 +82,122 @@ tryopen_o_directory (void) #endif -DIR * -internal_function -__opendirat (int dfd, const char *name) +static bool +invalid_name (const char *name) { - struct stat64 statbuf; - struct stat64 *statp = NULL; - - if (__builtin_expect (name[0], '\1') == '\0') + if (__glibc_unlikely (name[0] == '\0')) { /* POSIX.1-1990 says an empty name gets ENOENT; but `open' might like it fine. */ __set_errno (ENOENT); - return NULL; + return true; } + return false; +} + +static bool +need_isdir_precheck (void) +{ #ifdef O_DIRECTORY /* Test whether O_DIRECTORY works. */ if (o_directory_works == 0) tryopen_o_directory (); /* We can skip the expensive `stat' call if O_DIRECTORY works. */ - if (o_directory_works < 0) + return o_directory_works > 0; #endif - { - /* We first have to check whether the name is for a directory. We - cannot do this after the open() call since the open/close operation - performed on, say, a tape device might have undesirable effects. */ - if (__builtin_expect (__xstat64 (_STAT_VER, name, &statbuf), 0) < 0) - return NULL; - if (__glibc_unlikely (! S_ISDIR (statbuf.st_mode))) - { - __set_errno (ENOTDIR); - return NULL; - } - } + return true; +} + +static int +opendir_oflags (void) +{ int flags = O_RDONLY|O_NDELAY|EXTRA_FLAGS|O_LARGEFILE; #ifdef O_CLOEXEC flags |= O_CLOEXEC; #endif - int fd; -#if IS_IN (rtld) - assert (dfd == AT_FDCWD); - fd = open_not_cancel_2 (name, flags); -#else - fd = openat_not_cancel_3 (dfd, name, flags); -#endif - if (__builtin_expect (fd, 0) < 0) + return flags; +} + + +static DIR * +opendir_tail (int fd) +{ + if (__glibc_unlikely (fd < 0)) return NULL; -#ifdef O_DIRECTORY - if (o_directory_works <= 0) -#endif + /* Now make sure this really is a directory and nothing changed since the + `stat' call. The S_ISDIR check is superfluous if O_DIRECTORY works, + but it's cheap and we need the stat call for st_blksize anyway. */ + struct stat64 statbuf; + if (__glibc_unlikely (__fxstat64 (_STAT_VER, fd, &statbuf) < 0)) + goto lose; + if (__glibc_unlikely (! S_ISDIR (statbuf.st_mode))) { - /* Now make sure this really is a directory and nothing changed since - the `stat' call. */ - if (__builtin_expect (__fxstat64 (_STAT_VER, fd, &statbuf), 0) < 0) - goto lose; + __set_errno (ENOTDIR); + lose: + close_not_cancel_no_status (fd); + return NULL; + } + + return __alloc_dir (fd, true, 0, &statbuf); +} + + +#if IS_IN (libc) +DIR * +internal_function +__opendirat (int dfd, const char *name) +{ + if (__glibc_unlikely (invalid_name (name))) + return NULL; + + if (need_isdir_precheck ()) + { + /* We first have to check whether the name is for a directory. We + cannot do this after the open() call since the open/close operation + performed on, say, a tape device might have undesirable effects. */ + struct stat64 statbuf; + if (__glibc_unlikely (__fxstatat64 (_STAT_VER, dfd, name, + &statbuf, 0) < 0)) + return NULL; if (__glibc_unlikely (! S_ISDIR (statbuf.st_mode))) { __set_errno (ENOTDIR); - lose: - close_not_cancel_no_status (fd); return NULL; } - statp = &statbuf; } - return __alloc_dir (fd, true, 0, statp); + return opendir_tail (openat_not_cancel_3 (dfd, name, opendir_oflags ())); } +#endif /* Open a directory stream on NAME. */ DIR * __opendir (const char *name) { - return __opendirat (AT_FDCWD, name); + if (__glibc_unlikely (invalid_name (name))) + return NULL; + + if (need_isdir_precheck ()) + { + /* We first have to check whether the name is for a directory. We + cannot do this after the open() call since the open/close operation + performed on, say, a tape device might have undesirable effects. */ + struct stat64 statbuf; + if (__glibc_unlikely (__xstat64 (_STAT_VER, name, &statbuf) < 0)) + return NULL; + if (__glibc_unlikely (! S_ISDIR (statbuf.st_mode))) + { + __set_errno (ENOTDIR); + return NULL; + } + } + + return opendir_tail (open_not_cancel_2 (name, opendir_oflags ())); } weak_alias (__opendir, opendir)