From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 112825 invoked by alias); 20 Oct 2016 13:58:13 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 112816 invoked by uid 89); 20 Oct 2016 13:58:12 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=no version=3.3.2 spammy=imply, Hx-languages-length:2588 X-HELO: mail-vk0-f49.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=Ts518ke7J9LLHMk1C4GEX73V2RxTup0dkYSbyqnXPSw=; b=hP5EycSC1hLvePoDw3bvkAwRI8jZZwRdtN5tKQiwqgmhZc2VGYZfe2wmFv3SGWolgg SV+aYjjzM09UbtBOcyOvTgBgBWM72xEzgaI4CdCvVtcRLWhzw0AOZjz70H+Yx/+56jnR 4rMyhMuFVHWOIxwVNl2wd5bL3RExTs+jj12jwKeYYQXGcKbBmyui0gvkNnVnhSYpWBy5 TQ7zRPvxFyvIdwmkhpyWKn56K75kj/xeI6Z6haV1abC/8BIHvEli5LTp2O3NBozHHO77 cxiZ8XaXhonXObPhHVB9zeI4SrD07Z8gry/W+plaH/gZi6o4SpZNzcw6X8M30igkGc15 NtoA== X-Gm-Message-State: AA6/9Rl0+e2EG3lVFot8pvM3JDMHv9+6Ewjqwv9eg+IaNvyfgeq8Lu8kTBvksKmn2buSPL5L X-Received: by 10.31.191.14 with SMTP id p14mr1131671vkf.40.1476971880309; Thu, 20 Oct 2016 06:58:00 -0700 (PDT) Subject: Re: [PATCH v2] Linux: consolidate rename() To: libc-alpha@sourceware.org References: <1476924756-31448-1-git-send-email-ynorov@caviumnetworks.com> From: Adhemerval Zanella Message-ID: <0b76f92f-0c56-b239-3820-ff38a707c382@linaro.org> Date: Thu, 20 Oct 2016 13:58:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-SW-Source: 2016-10/txt/msg00336.txt.bz2 On 20/10/2016 11:13, Joseph Myers wrote: > On Thu, 20 Oct 2016, Yury Norov wrote: > >> Tested on arm64/lp64 which supports renameat and renameat2, >> and aarch64/ilp32 which supports renameat2 only. > > There's clearly something very wrong with your test environment that you > need to resolve before submitting any more glibc patches at all. If you'd > tested this patch you should have seen linknamespace tests failing because > rename is in standards that do not include renameat, and the localplt test > failing because you're calling an exported function that isn't using the > libc_hidden_proto/libc_hidden_def machinery (even non-exported functions > have to use that machinery when called within the same library, but > failure to do so is less visible from test results). > > Just leave rename.c in sysdeps/unix/sysv/linux/generic and add a case for > it to use the renameat2 syscall and you avoid all these issues; on > platforms with the rename syscall, there is no benefit from going via > other syscalls, and calling renameat from rename rather than using > syscalls introduces unnecessary complications. (But you must still test > by running the full glibc testsuite and actually investigating any > failures seen.) > I think the idea is to remove the 'generic' folder and let the sysdeps/unix/sysv/linux/ be the generic Linux implementation (which makes more sense IMHO and avoid confusion in which folder to imply). Yury, as Joseph is pointing out you need to run full make check from GLIBC to check not only that syscall is running but also if linkspace is being clean and inside PLT is what to expected. After make check shows no regression you can move forward to functionality tests like LTP. Also, I would recommend to run a sanity make check on x86 as well. Now, related to patch I think a simpler implementation for both rename and renameat would be just: * sysdeps/unix/sysv/linux/rename.c: int rename (const char *old, const char *new) { #ifdef __NR_rename return INLINE_SYSCALL_CALL (rename, old, new); #else return INLINE_SYSCALL_CALL (renameat, AT_FDCWD, old, AT_FDCWD, new); #endif } * sysdeps/unix/sysv/linux/renameat.c int renameat (int oldfd, const char *old, int newfd, const char *new) { #ifdef __NR_renameat return INLINE_SYSCALL_CALL (renameat, oldfd, old, newfd, new); #else return INLINE_SYSCALL_CALL (renameat2, oldfd, old, newfd, new, 0); #endif } I think we can drop the #error on renameat since at least __NR_renameat should be defined in all supported kernels (it was added on 2.6.16 afaik).