From patchwork Tue Oct 4 15:07:08 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella Netto X-Patchwork-Id: 77268 Delivered-To: patch@linaro.org Received: by 10.140.106.72 with SMTP id d66csp2271584qgf; Tue, 4 Oct 2016 08:08:14 -0700 (PDT) X-Received: by 10.98.27.138 with SMTP id b132mr6671805pfb.111.1475593693940; Tue, 04 Oct 2016 08:08:13 -0700 (PDT) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id xy2si3738322pab.242.2016.10.04.08.08.13 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 04 Oct 2016 08:08:13 -0700 (PDT) Received-SPF: pass (google.com: domain of libc-alpha-return-73683-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; dkim=pass header.i=@sourceware.org; spf=pass (google.com: domain of libc-alpha-return-73683-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=libc-alpha-return-73683-patch=linaro.org@sourceware.org; dmarc=fail (p=NONE dis=NONE) header.from=linaro.org DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:subject:to:references:from:message-id:date :mime-version:in-reply-to:content-type :content-transfer-encoding; q=dns; s=default; b=bnYdb9FywU9qH6RT CjSTPjf0n4yaBKzmiX/q06XatuQ7lCSB56F+COMD+ogfkWvoyKMQGz+ZX82FUNnu xcFv54p+ebxWai+2KMbhH0zXngk+JtY2rk/ijZlyFiTQ/UdsWI3qdtEyBOZzD0Mp 3AFK8P2gbdCqwuDfEDFsz6Ay9vQ= 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:subject:to:references:from:message-id:date :mime-version:in-reply-to:content-type :content-transfer-encoding; s=default; bh=5RzJm0CKJ9mqzD2e9/wjxo U9gmI=; b=iTjfe09kv6x/5iB+kOmgq8SO/7VoneQCQwxVPkmyTeli0juaistzAJ yD+NC4/3VGxxspJrQ8q9N/ncKXqNIzrPp6Czy2ChhBMyNwmZHT6ReI+STD5m+Ky6 dCsYBrnBdlUh4g5kn1IZ/+pGOv4xMtoLwBkQZHycmVhEYRWVsluG0= Received: (qmail 14259 invoked by alias); 4 Oct 2016 15:07: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 14232 invoked by uid 89); 4 Oct 2016 15:07:58 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=memcmp, 2007-2016, 20072016, llu X-HELO: mail-yw0-f178.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=XhDDs9ig004wgFTT8j5t7CO9lYT4vlbwqmUGOno40wU=; b=j5EFhmw3zxXNI56+LIlM/3kIe26hb1xDxc5MRQrjmFNV9ajnGdTUqQgI+rqRrUjcPU Wr8skAseJGq3DQIO7qO8nr+j7+MaQA65AcgP2MvOfruH6wyFtVswdp0j7yv8PFtG0oQo mFjIktI7bHeIp1FkmtPScoagDfklVZ8MltfSnY2cIg6qn0AKRj60Vs2LJsz3gMumoxBs KFhF6MY+p3j+sipAmITRSNpvWKKppAB/AuxosSHO5YZGb7djKkE53VTpNmt1HkAfY9zM yxJfpqbfhaNqDavG4AA61FNU0noLWQaHeofFk/Sck26NP5bv/VwDyl3YY3W2F9e7cQh+ 3fBw== X-Gm-Message-State: AA6/9RnqPwZ8Q+Y/tRUcDKI0D2fcrITiweSZFv5FuzuKXfEYHPA4yPRSSDnuUDtcY/4TADj7 X-Received: by 10.13.210.67 with SMTP id u64mr3289001ywd.135.1475593632057; Tue, 04 Oct 2016 08:07:12 -0700 (PDT) Subject: Re: [PATCH v2 1/3] Consolidate fallocate{64} implementations To: Siddhesh Poyarekar , libc-alpha@sourceware.org References: <1475021701-22246-1-git-send-email-adhemerval.zanella@linaro.com> From: Adhemerval Zanella Message-ID: Date: Tue, 4 Oct 2016 12:07:08 -0300 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: Hi Siddhesh, Thanks for the review, comments below: On 29/09/2016 21:51, Siddhesh Poyarekar wrote: >> diff --git a/sysdeps/unix/sysv/linux/tst-fallocate-common.c b/sysdeps/unix/sysv/linux/tst-fallocate-common.c >> new file mode 100644 >> index 0000000..5ef66be >> --- /dev/null >> +++ b/sysdeps/unix/sysv/linux/tst-fallocate-common.c >> + >> +void >> +do_prepare (void) >> +{ >> + temp_fd = create_temp_file ("tst-fallocate.", &temp_filename); >> + if (temp_fd == -1) >> + { >> + printf ("cannot create temporary file: %m\n"); >> + exit (1); >> + } >> +} >> + >> +#define FAIL(str) \ >> + do { printf ("error: %s (line %d)\n", str, __LINE__); return 1; } while (0) > > Suggest: Use statement expressions since we use it widely in glibc. It > might be a nice low hanging fruit project to fix all our tests to use > FAIL defined like this. Ack, I changed to a statement expression. > >> + >> +static int >> +do_test_with_offset (off_t offset) >> +{ >> + int ret; >> + struct stat finfo; >> + char bwrite[1024] = { 0xf0 }; >> + char bread[1024]; > > Define a macro BLOCK_SIZE to 1024 and use that everywhere. Ack. >> + >> + /* It tries to fallocate 1024 bytes from 'offset' and then write 1024 bytes. >> + After both operation rewind the file descriptor and read 1024 bytes >> + and check if both buffer have the same contents. */ >> + ret = fallocate (temp_fd, 0, offset, 1024); >> + if (ret == -1) >> + FAIL ("fallocate failed"); >> + >> + ret = fstat (temp_fd, &finfo); >> + if (ret == -1) >> + FAIL ("fstat failed"); >> + >> + if (finfo.st_size < (offset + 1024)) >> + FAIL ("size of first fallocate less than expected (1024)"); > > Suggest: Print offset + 1024 instead of just 1024. Ack. >> diff --git a/sysdeps/unix/sysv/linux/tst-fallocate64.c b/sysdeps/unix/sysv/linux/tst-fallocate64.c >> new file mode 100644 >> index 0000000..78bcb8c >> --- /dev/null >> +++ b/sysdeps/unix/sysv/linux/tst-fallocate64.c >> + >> +static int >> +do_test (void) >> +{ >> + ssize_t ret; >> + >> + ret = do_test_with_offset (0); >> + if (ret == -1) >> + return 1; > > This will fail since you return 1 on failure from do_test_with_offset, > not -1. Ack, I fixed it. > >> + >> + off_t base_offset = UINT32_MAX + 2048LL; > > This will overflow on 32-bit. Not really since we explicit build the test with _FILE_OFFSET_BITS=64 so off_t will be 64-bit regardless. > >> + ret = do_test_with_offset (base_offset); >> + if (ret == -1) >> + return 1; > > Again do_test_with_offset returns 1, not -1. Ack. Below it is a version with the test fixed based on your comments: diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile index 9a0423e..f8f778b 100644 --- a/sysdeps/unix/sysv/linux/Makefile +++ b/sysdeps/unix/sysv/linux/Makefile @@ -42,7 +42,8 @@ sysdep_headers += sys/mount.h sys/acct.h sys/sysctl.h \ bits/socket_type.h bits/syscall.h bits/sysctl.h \ bits/mman-linux.h -tests += tst-clone tst-clone2 tst-fanotify tst-personality tst-quota +tests += tst-clone tst-clone2 tst-fanotify tst-personality tst-quota \ + tst-fallocate tst-fallocate64 # Generate the list of SYS_* macros for the system calls (__NR_* macros). diff --git a/sysdeps/unix/sysv/linux/fallocate.c b/sysdeps/unix/sysv/linux/fallocate.c index 6a58a5f..a38e9a6 100644 --- a/sysdeps/unix/sysv/linux/fallocate.c +++ b/sysdeps/unix/sysv/linux/fallocate.c @@ -19,17 +19,12 @@ #include #include - +#ifndef __OFF_T_MATCHES_OFF64_T /* Reserve storage for the data of the file associated with FD. */ int fallocate (int fd, int mode, __off_t offset, __off_t len) { -#ifdef __NR_fallocate return SYSCALL_CANCEL (fallocate, fd, mode, - __LONG_LONG_PAIR (offset >> 31, offset), - __LONG_LONG_PAIR (len >> 31, len)); -#else - __set_errno (ENOSYS); - return -1; -#endif + SYSCALL_LL (offset), SYSCALL_LL (len)); } +#endif diff --git a/sysdeps/unix/sysv/linux/fallocate64.c b/sysdeps/unix/sysv/linux/fallocate64.c index 8e76d6f..d466095 100644 --- a/sysdeps/unix/sysv/linux/fallocate64.c +++ b/sysdeps/unix/sysv/linux/fallocate64.c @@ -24,14 +24,10 @@ int fallocate64 (int fd, int mode, __off64_t offset, __off64_t len) { -#ifdef __NR_fallocate return SYSCALL_CANCEL (fallocate, fd, mode, - __LONG_LONG_PAIR ((long int) (offset >> 32), - (long int) offset), - __LONG_LONG_PAIR ((long int) (len >> 32), - (long int) len)); -#else - __set_errno (ENOSYS); - return -1; -#endif + SYSCALL_LL64 (offset), SYSCALL_LL64 (len)); } + +#ifdef __OFF_T_MATCHES_OFF64_T +weak_alias (fallocate64, fallocate) +#endif diff --git a/sysdeps/unix/sysv/linux/mips/mips64/n32/fallocate.c b/sysdeps/unix/sysv/linux/mips/mips64/n32/fallocate.c deleted file mode 100644 index 4e432f4..0000000 --- a/sysdeps/unix/sysv/linux/mips/mips64/n32/fallocate.c +++ /dev/null @@ -1,33 +0,0 @@ -/* Copyright (C) 2007-2016 Free Software Foundation, Inc. - This file is part of the GNU C Library. - - The GNU C Library is free software; you can redistribute it and/or - modify it under the terms of the GNU Lesser General Public - License as published by the Free Software Foundation; either - version 2.1 of the License, or (at your option) any later version. - - The GNU C Library is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - Lesser General Public License for more details. - - You should have received a copy of the GNU Lesser General Public - License along with the GNU C Library. If not, see - . */ - -#include -#include -#include - - -/* Reserve storage for the data of the file associated with FD. */ -int -fallocate (int fd, int mode, __off_t offset, __off_t len) -{ -#ifdef __NR_fallocate - return SYSCALL_CANCEL (fallocate, fd, mode, offset, len); -#else - __set_errno (ENOSYS); - return -1; -#endif -} diff --git a/sysdeps/unix/sysv/linux/mips/mips64/n32/fallocate64.c b/sysdeps/unix/sysv/linux/mips/mips64/n32/fallocate64.c deleted file mode 100644 index 95c9eab..0000000 --- a/sysdeps/unix/sysv/linux/mips/mips64/n32/fallocate64.c +++ /dev/null @@ -1,33 +0,0 @@ -/* Copyright (C) 2007-2016 Free Software Foundation, Inc. - This file is part of the GNU C Library. - - The GNU C Library is free software; you can redistribute it and/or - modify it under the terms of the GNU Lesser General Public - License as published by the Free Software Foundation; either - version 2.1 of the License, or (at your option) any later version. - - The GNU C Library is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - Lesser General Public License for more details. - - You should have received a copy of the GNU Lesser General Public - License along with the GNU C Library. If not, see - . */ - -#include -#include -#include - - -/* Reserve storage for the data of the file associated with FD. */ -int -fallocate64 (int fd, int mode, __off64_t offset, __off64_t len) -{ -#ifdef __NR_fallocate - return SYSCALL_CANCEL (fallocate, fd, mode, offset, len); -#else - __set_errno (ENOSYS); - return -1; -#endif -} diff --git a/sysdeps/unix/sysv/linux/mips/mips64/n64/fallocate.c b/sysdeps/unix/sysv/linux/mips/mips64/n64/fallocate.c deleted file mode 100644 index d3b7218..0000000 --- a/sysdeps/unix/sysv/linux/mips/mips64/n64/fallocate.c +++ /dev/null @@ -1 +0,0 @@ -#include diff --git a/sysdeps/unix/sysv/linux/mips/mips64/n64/fallocate64.c b/sysdeps/unix/sysv/linux/mips/mips64/n64/fallocate64.c deleted file mode 100644 index fb2b681..0000000 --- a/sysdeps/unix/sysv/linux/mips/mips64/n64/fallocate64.c +++ /dev/null @@ -1 +0,0 @@ -/* fallocate64 is in fallocate.c */ diff --git a/sysdeps/unix/sysv/linux/tst-fallocate-common.c b/sysdeps/unix/sysv/linux/tst-fallocate-common.c new file mode 100644 index 0000000..76f31ff --- /dev/null +++ b/sysdeps/unix/sysv/linux/tst-fallocate-common.c @@ -0,0 +1,98 @@ +/* Basic fallocate test (no specific flags is checked). + Copyright (C) 2016 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + . */ + +#include +#include +#include +#include + +static void do_prepare (void); +#define PREPARE(argc, argv) do_prepare () +static int do_test (void); +#define TEST_FUNCTION do_test () + +#define TIMEOUT 20 /* sec. */ + +#define XSTR(s) STR(S) +#define STR(s) #s + +#include + +static char *temp_filename; +static int temp_fd; + +void +do_prepare (void) +{ + temp_fd = create_temp_file ("tst-fallocate.", &temp_filename); + if (temp_fd == -1) + { + printf ("cannot create temporary file: %m\n"); + exit (1); + } +} + +#define FAIL(...) \ + ({ \ + printf ("error: line %d: ", __LINE__); \ + printf (__VA_ARGS__); \ + printf ("\n"); \ + return 1; \ + }) + +static int +do_test_with_offset (off_t offset) +{ + int ret; + struct stat finfo; +#define BLK_SIZE 1024 + char bwrite[BLK_SIZE] = { 0xf0 }; + char bread[BLK_SIZE]; + + /* It tries to fallocate 1024 bytes from 'offset' and then write 1024 bytes. + After both operation rewind the file descriptor and read 1024 bytes + and check if both buffer have the same contents. */ + ret = fallocate (temp_fd, 0, offset, BLK_SIZE); + if (ret == -1) + FAIL ("fallocate failed"); + + ret = fstat (temp_fd, &finfo); + if (ret == -1) + FAIL ("fstat failed"); + + if (finfo.st_size < (offset + BLK_SIZE)) + FAIL ("size of first fallocate less than expected (%llu)", + (long long unsigned int)offset + BLK_SIZE); + + if (lseek (temp_fd, offset, SEEK_SET) == (off_t) -1) + FAIL ("fseek (0, SEEK_SET) failed"); + + if (write (temp_fd, bwrite, BLK_SIZE) != BLK_SIZE) + FAIL ("fail trying to write " XSTR (BLK_SIZE) " bytes"); + + if (lseek (temp_fd, offset, SEEK_SET) == (off_t) -1) + FAIL ("fseek (0, SEEK_SET) failed"); + + if (read (temp_fd, bread, BLK_SIZE) != BLK_SIZE) + FAIL ("fail trying to read " XSTR (BLK_SIZE) " bytes"); + + if (memcmp (bwrite, bread, BLK_SIZE) != 0) + FAIL ("buffer writted different than buffer readed"); + + return 0; +} diff --git a/sysdeps/unix/sysv/linux/tst-fallocate.c b/sysdeps/unix/sysv/linux/tst-fallocate.c new file mode 100644 index 0000000..ec3867c --- /dev/null +++ b/sysdeps/unix/sysv/linux/tst-fallocate.c @@ -0,0 +1,25 @@ +/* Basic fallocate test (no specific flags is checked). + Copyright (C) 2016 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + . */ + +#include "tst-fallocate-common.c" + +static int +do_test (void) +{ + return do_test_with_offset (0); +} diff --git a/sysdeps/unix/sysv/linux/tst-fallocate64.c b/sysdeps/unix/sysv/linux/tst-fallocate64.c new file mode 100644 index 0000000..3c0d370 --- /dev/null +++ b/sysdeps/unix/sysv/linux/tst-fallocate64.c @@ -0,0 +1,54 @@ +/* Basic fallocate64 test (no specific flags is checked). + Copyright (C) 2016 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + . */ + +#define _FILE_OFFSET_BITS 64 +#include "tst-fallocate-common.c" + +static int +do_test (void) +{ + ssize_t ret; + + ret = do_test_with_offset (0); + if (ret == 1) + return 1; + + off_t base_offset = UINT32_MAX + 2048LL; + ret = do_test_with_offset (base_offset); + if (ret == 1) + return 1; + + struct stat st; + if (fstat (temp_fd, &st) == -1) + { + printf ("error: fstat on temporary file failed: %m"); + return 1; + } + + /* The file size should >= base_offset plus bytes written. */ + off_t expected_value = base_offset + ret; + if (st.st_size < expected_value) + { + printf ("error: file size less than expected (%jd > %jd)\n", + (intmax_t) expected_value, (intmax_t) st.st_size); + return 1; + } + + return 0; +} + diff --git a/sysdeps/unix/sysv/linux/wordsize-64/fallocate.c b/sysdeps/unix/sysv/linux/wordsize-64/fallocate.c deleted file mode 100644 index c1dd0b4..0000000 --- a/sysdeps/unix/sysv/linux/wordsize-64/fallocate.c +++ /dev/null @@ -1,34 +0,0 @@ -/* Copyright (C) 2007-2016 Free Software Foundation, Inc. - This file is part of the GNU C Library. - - The GNU C Library is free software; you can redistribute it and/or - modify it under the terms of the GNU Lesser General Public - License as published by the Free Software Foundation; either - version 2.1 of the License, or (at your option) any later version. - - The GNU C Library is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - Lesser General Public License for more details. - - You should have received a copy of the GNU Lesser General Public - License along with the GNU C Library; if not, see - . */ - -#include -#include -#include - - -/* Reserve storage for the data of the file associated with FD. */ -int -fallocate (int fd, int mode, __off_t offset, __off_t len) -{ -#ifdef __NR_fallocate - return SYSCALL_CANCEL (fallocate, fd, mode, offset, len); -#else - __set_errno (ENOSYS); - return -1; -#endif -} -strong_alias (fallocate, fallocate64) diff --git a/sysdeps/unix/sysv/linux/wordsize-64/fallocate64.c b/sysdeps/unix/sysv/linux/wordsize-64/fallocate64.c deleted file mode 100644 index df33112..0000000 --- a/sysdeps/unix/sysv/linux/wordsize-64/fallocate64.c +++ /dev/null @@ -1 +0,0 @@ -/* fallocate64 is in posix_fallocate.c */