From patchwork Mon Mar 17 11:33:01 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Siddhesh Poyarekar X-Patchwork-Id: 330902 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 ED1392C00AC for ; Mon, 17 Mar 2014 22:32:33 +1100 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:subject:message-id:references :mime-version:content-type:in-reply-to; q=dns; s=default; b=EKj9 NPYC3GlIp/+z6cuzmToK7bZxQv7YsO9lAzthV5KCpanPzA7CB6ZYJlny7sIzQK3I YI8KGgu1yRZm6xOyGiBgZFyN1SQX6X5whsiOKbxS2Zif/Mh/dyUdoBUrykkn2DhA 7AX70BEUpOauBNLxs9vvI+V4lA1Y4P5ON/lcz28= 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:date:from:to:cc:subject:message-id:references :mime-version:content-type:in-reply-to; s=default; bh=HkhcquF+nN Gldfy9GxDdmHWhBhA=; b=qRKvlqEyTxcYnhPGo36xHO8B03NgoJaI9pmnZTaKni lLcz1YAPMmkAp0CPoin0ZhqEgNdygHmqUPolikaN6tLYYhK6enhhop41/PFz2+po hWvlNOMsSNDZr99ait7VnxCQ++ct/Gzep6xrxJpXqXrTsmqezC81G6PJexrlZqUv g= Received: (qmail 16264 invoked by alias); 17 Mar 2014 11:32:27 -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 16253 invoked by uid 89); 17 Mar 2014 11:32:26 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.8 required=5.0 tests=AWL, BAYES_00, SPF_HELO_PASS, SPF_PASS, T_RP_MATCHES_RCVD, UNSUBSCRIBE_BODY autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Date: Mon, 17 Mar 2014 17:03:01 +0530 From: Siddhesh Poyarekar To: "Carlos O'Donell" Cc: libc-alpha@sourceware.org, dalias@aerifal.cx Subject: Re: [PATCH v2] Fix offset caching for streams and use it for ftell (BZ #16680) Message-ID: <20140317113301.GL1850@spoyarek.pnq.redhat.com> References: <20140311124615.GA2182@spoyarek.pnq.redhat.com> <5326AFAA.7000809@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <5326AFAA.7000809@redhat.com> User-Agent: Mutt/1.5.22.1-rc1 (2013-10-16) On Mon, Mar 17, 2014 at 04:17:46AM -0400, Carlos O'Donell wrote: > Right, so I was thinking along the same lines. If you didn't switch > from write to read then there is no reason why the offset can't > be the end of the file after rewind. However, that subtle change > is a change in our implementation and I agree that it would be best > if we retained the old behaviour. > > Does a `write; rewind; read' work now with the patch? Was the current > patch causing this to fail also or did the `read' start reading at > offset 0? That's not a concern since the read would fail for a file opened in 'a' mode. Only ftell behaviour was relevant here. For 'a+' it reads at offset 0 and that is correct. On 17 March 2014 16:02, Rich Felker wrote: >> Does this reinstitution of lseek cause any problems for the use of ftell >> on inactive streams? For example is it really correct to have _IO_file_open >> seek to the end of the fully flushed file in append mode? What about >> other users of the fd that might expect the underlying offset to remain >> the same? > > It's perfectly correct for fopen to perform the seek. For fdopen I'm > not so clear; I think it's wrong for fdopen to change the position > except in the case where O_APPEND wasn't set and fdopen adds it (in > this case the implementation can do whatever it wants, no?). I had missed the detail about fdopen not modifying the file descriptor if O_APPEND is already set, thanks for pointing out. I've attached a patch that applies on top of the earlier patch to ensure that fdopen seeks to the end only when O_APPEND is set by fdopen and not when O_APPEND is already set. I have also added a test case to verify this behaviour. None of the old behaviour is affected by this. Siddhesh * libio/iofdopen.c (_IO_new_fdopen): Seek to end only if setting O_APPEND. * libio/tst-ftell-active-handler.c (do_append_test): Add a test case. diff --git a/libio/iofdopen.c b/libio/iofdopen.c index 843a4fa..77a61f1 100644 --- a/libio/iofdopen.c +++ b/libio/iofdopen.c @@ -58,6 +58,7 @@ _IO_new_fdopen (fd, mode) int fd_flags; int i; int use_mmap = 0; + bool do_seek = false; switch (*mode) { @@ -128,6 +129,7 @@ _IO_new_fdopen (fd, mode) */ if ((posix_mode & O_APPEND) && !(fd_flags & O_APPEND)) { + do_seek = true; #ifdef F_SETFL if (_IO_fcntl (fd, F_SETFL, fd_flags | O_APPEND) == -1) #endif @@ -169,8 +171,8 @@ _IO_new_fdopen (fd, mode) /* For append mode, set the file offset to the end of the file. Don't update the offset cache though, since the file handle is not active. */ - if ((read_write & (_IO_IS_APPENDING | _IO_NO_READS)) - == (_IO_IS_APPENDING | _IO_NO_READS)) + if (do_seek && ((read_write & (_IO_IS_APPENDING | _IO_NO_READS)) + == (_IO_IS_APPENDING | _IO_NO_READS))) { _IO_off64_t new_pos = _IO_SYSSEEK (&new_f->fp.file, 0, _IO_seek_end); if (new_pos == _IO_pos_BAD && errno != ESPIPE) diff --git a/libio/tst-ftell-active-handler.c b/libio/tst-ftell-active-handler.c index 40ca58c..e9dc7b3 100644 --- a/libio/tst-ftell-active-handler.c +++ b/libio/tst-ftell-active-handler.c @@ -414,6 +414,61 @@ do_append_test (const char *filename) } } + /* For fdopen in 'a' mode, the file descriptor should not change if the file + is already open with the O_APPEND flag set. */ + fd = open (filename, O_WRONLY | O_APPEND, 0); + if (fd == -1) + { + printf ("open(O_APPEND) failed: %m\n"); + return 1; + } + + off_t seek_ret = lseek (fd, file_len - 1, SEEK_SET); + if (seek_ret == -1) + { + printf ("lseek[O_APPEND][0] failed: %m\n"); + ret |= 1; + } + + fp = fdopen (fd, "a"); + if (fp == NULL) + { + printf ("fdopen(O_APPEND) failed: %m\n"); + close (fd); + return 1; + } + + off_t new_seek_ret = lseek (fd, 0, SEEK_CUR); + if (seek_ret == -1) + { + printf ("lseek[O_APPEND][1] failed: %m\n"); + ret |= 1; + } + + printf ("\tappend: fdopen (file, \"a\"): O_APPEND: "); + + if (seek_ret != new_seek_ret) + { + printf ("incorrectly modified file offset to %ld, should be %ld", + new_seek_ret, seek_ret); + ret |= 1; + } + else + printf ("retained current file offset %ld", seek_ret); + + new_seek_ret = ftello (fp); + + if (seek_ret != new_seek_ret) + { + printf (", ftello reported incorrect offset %ld, should be %ld\n", + new_seek_ret, seek_ret); + ret |= 1; + } + else + printf (", ftello reported correct offset %ld\n", seek_ret); + + fclose (fp); + return ret; }