You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by br...@apache.org on 2003/02/04 00:51:56 UTC
cvs commit: apr/test testfileinfo.c
brane 2003/02/03 15:51:56
Modified: test testfileinfo.c
Log:
Added a new test to check the behaviour of apr_file_info_get in
combination with buffered writes. At the moment, this test will
probably fail on all plaforms.
Revision Changes Path
1.9 +33 -0 apr/test/testfileinfo.c
Index: testfileinfo.c
===================================================================
RCS file: /home/cvs/apr/test/testfileinfo.c,v
retrieving revision 1.8
retrieving revision 1.9
diff -u -r1.8 -r1.9
--- testfileinfo.c 1 Jan 2003 00:01:56 -0000 1.8
+++ testfileinfo.c 3 Feb 2003 23:51:56 -0000 1.9
@@ -62,6 +62,8 @@
#include "test_apr.h"
#define FILENAME "data/file_datafile.txt"
+#define NEWFILENAME "data/new_datafile.txt"
+#define NEWFILEDATA "This is new text in a new file."
static const struct view_fileinfo
{
@@ -207,6 +209,36 @@
finfo_equal(tc, stat_finfo, finfo);
}
+static void test_buffered_write_size(CuTest *tc)
+{
+ const apr_size_t data_len = strlen(NEWFILEDATA);
+ apr_file_t *thefile;
+ apr_finfo_t finfo;
+ apr_status_t rv;
+ apr_size_t bytes;
+
+ rv = apr_file_open(&thefile, NEWFILENAME,
+ APR_READ | APR_WRITE | APR_CREATE | APR_EXCL
+ | APR_BUFFERED | APR_DELONCLOSE,
+ APR_OS_DEFAULT, p);
+ CuAssertIntEquals(tc, APR_SUCCESS, rv);
+
+ /* A funny thing happened to me the other day: I wrote something
+ * into a buffered file, then asked for its size using
+ * apr_file_info_get; and guess what? The size was 0! That's not a
+ * nice way to behave.
+ */
+ bytes = data_len;
+ rv = apr_file_write(thefile, NEWFILEDATA, &bytes);
+ CuAssertIntEquals(tc, APR_SUCCESS, rv);
+ CuAssertTrue(tc, data_len == bytes);
+
+ rv = apr_file_info_get(&finfo, APR_FINFO_SIZE, thefile);
+ CuAssertIntEquals(tc, APR_SUCCESS, rv);
+ CuAssertTrue(tc, bytes == (apr_size_t) finfo.size);
+ apr_file_close(thefile);
+}
+
CuSuite *testfileinfo(void)
{
CuSuite *suite = CuSuiteNew("File Info");
@@ -214,6 +246,7 @@
SUITE_ADD_TEST(suite, test_info_get);
SUITE_ADD_TEST(suite, test_stat);
SUITE_ADD_TEST(suite, test_stat_eq_finfo);
+ SUITE_ADD_TEST(suite, test_buffered_write_size);
return suite;
}
Re: cvs commit: apr/test testfileinfo.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 11:41 AM 2/11/2003, Branko Äibej wrote:
>Heh. The OS usually has file buffers, too, and yet stat() always tells
>you what the size of the file would be if all write()s were committed,
I really doubt that is a safe assumption across all OS's and file
systems.
>not what the size on disk actually is. If apr_file_write has an internal
>buffer, then apr_file_info_get should obviously be aware of that and
>adjust the results accordingly, without having to flush the buffers
>first -- and becoming orders of magnitude less efficient.
The only safe test I can imagine, without closing the file, is looking at
seek(0, SEEK_END). I believe all platforms implement this correctly
in conjunction with our file buffering, and the OS should perform the
seek correctly even if the inode or directory entry is still out of sync.
Bill
Re: cvs commit: apr/test testfileinfo.c
Posted by Brian Havard <br...@kheldar.apana.org.au>.
On Tue, 11 Feb 2003 23:01:48 +0100, Branko Ä ibej wrote:
>Joe Orton wrote:
>>--- filestat.c 7 Jan 2003 00:52:53 -0000 1.64
>>+++ filestat.c 11 Feb 2003 18:19:12 -0000
>>@@ -135,6 +135,8 @@
>> finfo->pool = thefile->pool;
>> finfo->fname = thefile->fname;
>> fill_out_finfo(finfo, &info, wanted);
>>+ if (thefile->buffered)
>>+ finfo->size += thefile->bufpos;
>> return (wanted & ~finfo->valid) ? APR_INCOMPLETE : APR_SUCCESS;
>> }
>> else {
>>
>>
>
>Hmm, does this patch actually work? I suspect the calculation would be
>slightly more involved -- after all, the buffer might not extend past
>the hard end-of-file at all.
And the buffer might contain read data, not written. This is the patch I'm
considering for the OS/2 code, it passes the File Info test:
Index: filestat.c
===================================================================
RCS file: /home/cvs/apr/file_io/os2/filestat.c,v
retrieving revision 1.34
diff -u -r1.34 filestat.c
--- filestat.c 7 Jan 2003 00:52:51 -0000 1.34
+++ filestat.c 13 Feb 2003 10:20:08 -0000
@@ -141,6 +141,12 @@
if (finfo->filetype == APR_REG) {
if (thefile->isopen) {
+ if (thefile->buffered && thefile->direction == 1) {
+ if (thefile->filePtr + thefile->bufpos > finfo->size) {
+ finfo->size = thefile->filePtr + thefile->bufpos;
+ }
+ }
+
return handle_type(&finfo->filetype, thefile->filedes);
}
} else {
--
______________________________________________________________________________
| Brian Havard | "He is not the messiah! |
| brianh@kheldar.apana.org.au | He's a very naughty boy!" - Life of Brian |
------------------------------------------------------------------------------
Re: cvs commit: apr/test testfileinfo.c
Posted by Branko Čibej <br...@xbc.nu>.
Joe Orton wrote:
>On Tue, Feb 11, 2003 at 06:41:36PM +0100, Branko Čibej wrote:
>
>
>>Joe Orton wrote:
>>
>>
>>
>>>On Sat, Feb 08, 2003 at 02:21:28AM +0100, Branko Čibej wrote:
>>>
>>>
>>>>I expect that change would avoid the problem, yes. (In fact, in
>>>>Subversion, I did an explicit fluxh befode calling apr_file_info_get,
>>>>for this very reason.) I was sort of hoping someone had a better idea
>>>>for a fix, though; forcing a flush before every stat seems to me to
>>>>defeat the whole purpose of buffering.
>>>>
>>>>
>>>>
>>>>
>>>I'm confused by that - buffering means "some writes are deferred", so a
>>>direct consequence of that is that the st_size returned by a stat on the
>>>file may not equal the number of bytes passed to apr_file_write, right?
>>>If you want stat().st_size to always equal the number of bytes passed to
>>>apr_file_write, then it sounds like you don't want to use buffering?
>>>
>>>
>>>
>>>
>>Heh. The OS usually has file buffers, too, and yet stat() always tells
>>you what the size of the file would be if all write()s were committed,
>>not what the size on disk actually is. If apr_file_write has an internal
>>buffer, then apr_file_info_get should obviously be aware of that and
>>adjust the results accordingly, without having to flush the buffers
>>first -- and becoming orders of magnitude less efficient.
>>
>>
>
>Ah, I wondered if that's what you were getting at. That analogy is a
>bit stretched, since if a stat() or fstat() reports st_size = X, you
>will be able to read() X bytes from the file, from any process. If
>apr_file_info_get lies about the st_size, the size returned is only
>valid for access to the file via that particular apr_file_t *, which
>would seem to break the principle of least surprise, at least.
>
That's a good point, but...
>What if the app passes it off to another library, or something?
>
You can't do that (portably) without some other APR magic anyway. And
even ignoring that, the situation is no worse than the frintf + fork
situatiion, for example -- if you pass buffered APR files to non-APR
code, then you have to flush them first.
My point is that apr_file_seek _does_ take buffering into account, so
apr_file_info_get should, too.
>Anyway, this is presumably what you mean: flushing still seems best to
>me, but I don't really care, I just want my builds to stop failing
>everywhere! ;)
>
You did commit the flush fix, didn't you? (/me looks...) Oh, you didn't!
>--- filestat.c 7 Jan 2003 00:52:53 -0000 1.64
>+++ filestat.c 11 Feb 2003 18:19:12 -0000
>@@ -135,6 +135,8 @@
> finfo->pool = thefile->pool;
> finfo->fname = thefile->fname;
> fill_out_finfo(finfo, &info, wanted);
>+ if (thefile->buffered)
>+ finfo->size += thefile->bufpos;
> return (wanted & ~finfo->valid) ? APR_INCOMPLETE : APR_SUCCESS;
> }
> else {
>
>
Hmm, does this patch actually work? I suspect the calculation would be
slightly more involved -- after all, the buffer might not extend past
the hard end-of-file at all.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
Re: cvs commit: apr/test testfileinfo.c
Posted by Joe Orton <jo...@manyfish.co.uk>.
On Tue, Feb 11, 2003 at 06:41:36PM +0100, Branko Čibej wrote:
> Joe Orton wrote:
>
> >On Sat, Feb 08, 2003 at 02:21:28AM +0100, Branko Čibej wrote:
> >>I expect that change would avoid the problem, yes. (In fact, in
> >>Subversion, I did an explicit fluxh befode calling apr_file_info_get,
> >>for this very reason.) I was sort of hoping someone had a better idea
> >>for a fix, though; forcing a flush before every stat seems to me to
> >>defeat the whole purpose of buffering.
> >>
> >>
> >
> >I'm confused by that - buffering means "some writes are deferred", so a
> >direct consequence of that is that the st_size returned by a stat on the
> >file may not equal the number of bytes passed to apr_file_write, right?
> >If you want stat().st_size to always equal the number of bytes passed to
> >apr_file_write, then it sounds like you don't want to use buffering?
> >
> >
> Heh. The OS usually has file buffers, too, and yet stat() always tells
> you what the size of the file would be if all write()s were committed,
> not what the size on disk actually is. If apr_file_write has an internal
> buffer, then apr_file_info_get should obviously be aware of that and
> adjust the results accordingly, without having to flush the buffers
> first -- and becoming orders of magnitude less efficient.
Ah, I wondered if that's what you were getting at. That analogy is a
bit stretched, since if a stat() or fstat() reports st_size = X, you
will be able to read() X bytes from the file, from any process. If
apr_file_info_get lies about the st_size, the size returned is only
valid for access to the file via that particular apr_file_t *, which
would seem to break the principle of least surprise, at least. What if
the app passes it off to another library, or something?
Anyway, this is presumably what you mean: flushing still seems best to
me, but I don't really care, I just want my builds to stop failing
everywhere! ;)
--- filestat.c 7 Jan 2003 00:52:53 -0000 1.64
+++ filestat.c 11 Feb 2003 18:19:12 -0000
@@ -135,6 +135,8 @@
finfo->pool = thefile->pool;
finfo->fname = thefile->fname;
fill_out_finfo(finfo, &info, wanted);
+ if (thefile->buffered)
+ finfo->size += thefile->bufpos;
return (wanted & ~finfo->valid) ? APR_INCOMPLETE : APR_SUCCESS;
}
else {
Re: cvs commit: apr/test testfileinfo.c
Posted by Branko Čibej <br...@xbc.nu>.
Joe Orton wrote:
>On Sat, Feb 08, 2003 at 02:21:28AM +0100, Branko Čibej wrote:
>
>
>>Joe Orton wrote:
>>
>>
>>>It does indeed, is this the kind of fix you were thinking of?
>>>
>>>
>>>
>>I expect that change would avoid the problem, yes. (In fact, in
>>Subversion, I did an explicit fluxh befode calling apr_file_info_get,
>>for this very reason.) I was sort of hoping someone had a better idea
>>for a fix, though; forcing a flush before every stat seems to me to
>>defeat the whole purpose of buffering.
>>
>>
>
>I'm confused by that - buffering means "some writes are deferred", so a
>direct consequence of that is that the st_size returned by a stat on the
>file may not equal the number of bytes passed to apr_file_write, right?
>If you want stat().st_size to always equal the number of bytes passed to
>apr_file_write, then it sounds like you don't want to use buffering?
>
>
Heh. The OS usually has file buffers, too, and yet stat() always tells
you what the size of the file would be if all write()s were committed,
not what the size on disk actually is. If apr_file_write has an internal
buffer, then apr_file_info_get should obviously be aware of that and
adjust the results accordingly, without having to flush the buffers
first -- and becoming orders of magnitude less efficient.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
Re: cvs commit: apr/test testfileinfo.c
Posted by Joe Orton <jo...@manyfish.co.uk>.
On Sat, Feb 08, 2003 at 02:21:28AM +0100, Branko Čibej wrote:
> Joe Orton wrote:
> >It does indeed, is this the kind of fix you were thinking of?
> >
> I expect that change would avoid the problem, yes. (In fact, in
> Subversion, I did an explicit fluxh befode calling apr_file_info_get,
> for this very reason.) I was sort of hoping someone had a better idea
> for a fix, though; forcing a flush before every stat seems to me to
> defeat the whole purpose of buffering.
I'm confused by that - buffering means "some writes are deferred", so a
direct consequence of that is that the st_size returned by a stat on the
file may not equal the number of bytes passed to apr_file_write, right?
If you want stat().st_size to always equal the number of bytes passed to
apr_file_write, then it sounds like you don't want to use buffering?
Regards,
joe
Re: cvs commit: apr/test testfileinfo.c
Posted by Branko Čibej <br...@xbc.nu>.
Joe Orton wrote:
>On Mon, Feb 03, 2003 at 11:51:56PM -0000, Branko �ibej wrote:
>
>
>>brane 2003/02/03 15:51:56
>>
>> Modified: test testfileinfo.c
>> Log:
>> Added a new test to check the behaviour of apr_file_info_get in
>> combination with buffered writes. At the moment, this test will
>> probably fail on all plaforms.
>>
>>
>
>It does indeed, is this the kind of fix you were thinking of?
>
I expect that change would avoid the problem, yes. (In fact, in
Subversion, I did an explicit fluxh befode calling apr_file_info_get,
for this very reason.) I was sort of hoping someone had a better idea
for a fix, though; forcing a flush before every stat seems to me to
defeat the whole purpose of buffering.
>Index: filestat.c
>===================================================================
>RCS file: /home/cvs/apr/file_io/unix/filestat.c,v
>retrieving revision 1.64
>diff -u -r1.64 filestat.c
>--- filestat.c 7 Jan 2003 00:52:53 -0000 1.64
>+++ filestat.c 7 Feb 2003 12:36:01 -0000
>@@ -131,6 +131,13 @@
> {
> struct stat info;
>
>+ if (thefile->buffered) {
>+ apr_status_t rv = apr_file_flush(thefile);
>+
>+ if (rv != APR_SUCCESS)
>+ return rv;
>+ }
>+
> if (fstat(thefile->filedes, &info) == 0) {
> finfo->pool = thefile->pool;
> finfo->fname = thefile->fname;
>
>
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
Re: cvs commit: apr/test testfileinfo.c
Posted by Joe Orton <jo...@manyfish.co.uk>.
On Mon, Feb 03, 2003 at 11:51:56PM -0000, Branko �ibej wrote:
> brane 2003/02/03 15:51:56
>
> Modified: test testfileinfo.c
> Log:
> Added a new test to check the behaviour of apr_file_info_get in
> combination with buffered writes. At the moment, this test will
> probably fail on all plaforms.
It does indeed, is this the kind of fix you were thinking of?
Index: filestat.c
===================================================================
RCS file: /home/cvs/apr/file_io/unix/filestat.c,v
retrieving revision 1.64
diff -u -r1.64 filestat.c
--- filestat.c 7 Jan 2003 00:52:53 -0000 1.64
+++ filestat.c 7 Feb 2003 12:36:01 -0000
@@ -131,6 +131,13 @@
{
struct stat info;
+ if (thefile->buffered) {
+ apr_status_t rv = apr_file_flush(thefile);
+
+ if (rv != APR_SUCCESS)
+ return rv;
+ }
+
if (fstat(thefile->filedes, &info) == 0) {
finfo->pool = thefile->pool;
finfo->fname = thefile->fname;