You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by pq...@apache.org on 2004/12/05 00:40:37 UTC
svn commit: r109832 - /apr/apr/trunk/CHANGES /apr/apr/trunk/file_io/unix/readwrite.c /apr/apr/trunk/test/Makefile.in /apr/apr/trunk/test/data /apr/apr/trunk/test/testfile.c
Author: pquerna
Date: Sat Dec 4 15:40:37 2004
New Revision: 109832
URL: http://svn.apache.org/viewcvs?view=rev&rev=109832
Log:
* test/testfile.c: Add a test for apr_file_writev().
* file_io/unix/readwrite.c: Try to write all iovecs out on platforms without writev.
Modified:
apr/apr/trunk/CHANGES
apr/apr/trunk/file_io/unix/readwrite.c
apr/apr/trunk/test/Makefile.in
apr/apr/trunk/test/data/ (props changed)
apr/apr/trunk/test/testfile.c
Modified: apr/apr/trunk/CHANGES
Url: http://svn.apache.org/viewcvs/apr/apr/trunk/CHANGES?view=diff&rev=109832&p1=apr/apr/trunk/CHANGES&r1=109831&p2=apr/apr/trunk/CHANGES&r2=109832
==============================================================================
--- apr/apr/trunk/CHANGES (original)
+++ apr/apr/trunk/CHANGES Sat Dec 4 15:40:37 2004
@@ -1,5 +1,9 @@
Changes for APR 1.1.0
+ *) apr_file_writev will now at least try to write all iovecs on platforms
+ that do not support writev.
+ [Paul Querna]
+
*) Remove the runtime test for Sendfile versions on FreeBSD. PR 25718.
[Mike Silbersack <silby silby.com>, Paul Querna]
Modified: apr/apr/trunk/file_io/unix/readwrite.c
Url: http://svn.apache.org/viewcvs/apr/apr/trunk/file_io/unix/readwrite.c?view=diff&rev=109832&p1=apr/apr/trunk/file_io/unix/readwrite.c&r1=109831&p2=apr/apr/trunk/file_io/unix/readwrite.c&r2=109832
==============================================================================
--- apr/apr/trunk/file_io/unix/readwrite.c (original)
+++ apr/apr/trunk/file_io/unix/readwrite.c Sat Dec 4 15:40:37 2004
@@ -241,8 +241,17 @@
return APR_SUCCESS;
}
#else
- *nbytes = vec[0].iov_len;
- return apr_file_write(thefile, vec[0].iov_base, nbytes);
+ int i, tbytes;
+ apr_status_t rv = APR_SUCCESS;
+
+ for(i = 0; i < nvec; i++){
+ tbytes = vec[i].iov_len;
+ rv = apr_file_write(thefile, vec[i].iov_base, &tbytes);
+ *nbytes += tbytes;
+ if(rv != APR_SUCCESS)
+ break;
+ }
+ return rv;
#endif
}
Modified: apr/apr/trunk/test/Makefile.in
Url: http://svn.apache.org/viewcvs/apr/apr/trunk/test/Makefile.in?view=diff&rev=109832&p1=apr/apr/trunk/test/Makefile.in&r1=109831&p2=apr/apr/trunk/test/Makefile.in&r2=109832
==============================================================================
--- apr/apr/trunk/test/Makefile.in (original)
+++ apr/apr/trunk/test/Makefile.in Sat Dec 4 15:40:37 2004
@@ -31,7 +31,7 @@
CLEAN_TARGETS = testfile.tmp mod_test.slo proc_child@EXEEXT@ occhild@EXEEXT@ \
readchild@EXEEXT@ tryread@EXEEXT@ sockchild@EXEEXT@ \
globalmutexchild@EXEEXT@ lfstests/large.bin \
- data/testputs.txt data/testbigfprintf.dat
+ data/testputs.txt data/testbigfprintf.dat data/testwritev.txt
CLEAN_SUBDIRS = internal
INCDIR=../include
Modified: apr/apr/trunk/test/testfile.c
Url: http://svn.apache.org/viewcvs/apr/apr/trunk/test/testfile.c?view=diff&rev=109832&p1=apr/apr/trunk/test/testfile.c&r1=109831&p2=apr/apr/trunk/test/testfile.c&r2=109832
==============================================================================
--- apr/apr/trunk/test/testfile.c (original)
+++ apr/apr/trunk/test/testfile.c Sat Dec 4 15:40:37 2004
@@ -553,6 +553,48 @@
file_contents_equal(tc, fname, LINE1 LINE2, strlen(LINE1 LINE2));
}
+static void test_writev(abts_case *tc, void *data)
+{
+ apr_file_t *f;
+ int nbytes;
+ struct iovec vec[5];
+ const char *fname = "data/testwritev.txt";
+
+ APR_ASSERT_SUCCESS(tc, "open file for writing",
+ apr_file_open(&f, fname,
+ APR_WRITE|APR_CREATE|APR_TRUNCATE,
+ APR_OS_DEFAULT, p));
+
+ vec[0].iov_base = LINE1;
+ vec[0].iov_len = strlen(LINE1);
+
+ APR_ASSERT_SUCCESS(tc, "writev of size 1 to file",
+ apr_file_writev(f, vec, 1, &nbytes));
+
+ file_contents_equal(tc, fname, LINE1, strlen(LINE1));
+
+ vec[0].iov_base = LINE1;
+ vec[0].iov_len = strlen(LINE1);
+ vec[1].iov_base = LINE2;
+ vec[1].iov_len = strlen(LINE2);
+ vec[2].iov_base = LINE1;
+ vec[2].iov_len = strlen(LINE1);
+ vec[3].iov_base = LINE1;
+ vec[3].iov_len = strlen(LINE1);
+ vec[4].iov_base = LINE2;
+ vec[4].iov_len = strlen(LINE2);
+
+ APR_ASSERT_SUCCESS(tc, "writev of size 5 to file",
+ apr_file_writev(f, vec, 5, &nbytes));
+
+ APR_ASSERT_SUCCESS(tc, "close for writing",
+ apr_file_close(f));
+
+ file_contents_equal(tc, fname, LINE1 LINE1 LINE2 LINE1 LINE1 LINE2,
+ strlen(LINE1)*4 + strlen(LINE2)*2);
+
+}
+
static void test_truncate(abts_case *tc, void *data)
{
apr_status_t rv;
@@ -648,6 +690,7 @@
abts_run_test(suite, test_ungetc, NULL);
abts_run_test(suite, test_gets, NULL);
abts_run_test(suite, test_puts, NULL);
+ abts_run_test(suite, test_writev, NULL);
abts_run_test(suite, test_bigread, NULL);
abts_run_test(suite, test_mod_neg, NULL);
abts_run_test(suite, test_truncate, NULL);
Re: svn commit: r109832 -
Posted by Joe Orton <jo...@redhat.com>.
On Sun, Dec 05, 2004 at 02:08:31PM -0700, Paul Querna wrote:
> Joe Orton wrote:
> >...and back to the beginning, did you read the original thread on this
> >topic?
>
> No, I did not remember or was not aware of the 'original thread'.
> I was looking at use iovecs in mod_log_config, and in IRC Justin
> mentioned that the !HAVE_WRITEV implementation sucks. That started me
> down the path of trying to fix it.
>
> I assume you are talking about this thread in October?
> http://marc.theaimsgroup.com/?t=109693523200002&r=1&w=2
Yes, indeed.
joe
Re: svn commit: r109832 -
Posted by Paul Querna <ch...@force-elite.com>.
Joe Orton wrote:
> On Sun, Dec 05, 2004 at 10:35:11AM -0700, Paul Querna wrote:
>
>>Jeff Trawick wrote:
>>
>>>On Sun, 05 Dec 2004 00:01:05 -0700, Paul Querna <ch...@force-elite.com>
>>>1) write_full() has the wrong semantics for apr_file_writev();
>>>apr_file_writev() and apr_file_write() are not supposed to block
>>
>>Thanks, I didn't think about the issue of write_full() blocking.
>>r109892 uses apr_file_write(), and adds the check for an under-write
>>as you originally suggested.
>
>
> ...and back to the beginning, did you read the original thread on this
> topic?
No, I did not remember or was not aware of the 'original thread'.
I was looking at use iovecs in mod_log_config, and in IRC Justin
mentioned that the !HAVE_WRITEV implementation sucks. That started me
down the path of trying to fix it.
I assume you are talking about this thread in October?
http://marc.theaimsgroup.com/?t=109693523200002&r=1&w=2
> The fact that the caller must cope with short writes is exactly
> why the original implementation was valid and correct. All subsequent
> versions have been variously buggy - the type of tbytes is still wrong.
> I think this should just be reverted back to the original version.
>
> joe
>
>
Re: svn commit: r109832 -
Posted by Joe Orton <jo...@redhat.com>.
On Sun, Dec 05, 2004 at 10:35:11AM -0700, Paul Querna wrote:
> Jeff Trawick wrote:
> >On Sun, 05 Dec 2004 00:01:05 -0700, Paul Querna <ch...@force-elite.com>
> >1) write_full() has the wrong semantics for apr_file_writev();
> >apr_file_writev() and apr_file_write() are not supposed to block
>
> Thanks, I didn't think about the issue of write_full() blocking.
> r109892 uses apr_file_write(), and adds the check for an under-write
> as you originally suggested.
...and back to the beginning, did you read the original thread on this
topic? The fact that the caller must cope with short writes is exactly
why the original implementation was valid and correct. All subsequent
versions have been variously buggy - the type of tbytes is still wrong.
I think this should just be reverted back to the original version.
joe
Re: svn commit: r109832 -
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
Paul Querna wrote:
>> 2) fix the code formatting
>
>
> Can this please be more specific? I reviewed the style guide at
> http://httpd.apache.org/dev/styleguide.html and I believe all of the
> code I have modified here is within those guidelines.
I believe what he's referring to is the lack of spaces between the for
and the paren and the if and the paren, its inconsistent with the rest
of the codebase.
-garrett
Re: svn commit: r109832 -
Posted by Paul Querna <ch...@force-elite.com>.
Jeff Trawick wrote:
> On Sun, 05 Dec 2004 00:01:05 -0700, Paul Querna <ch...@force-elite.com> wrote:
>
>>Jeff Trawick wrote:
>> > we're hosed here if apr_file_write() can only write part of the data
>>
>>>(tbytes is < vec[i].iov_len and rv is APR_SUCCESS); don't you also
>>>need to bail if tbytes < vec[i].iov_len?
>>
>>Committed r109865 which uses apr_file_write_full() to hopefully write
>>the entire iovec.
>>
>>According to the docs for apr_file_write_full, if it returns in an error
>> condition, you should add the number of bytes written.
>
>
> 1) write_full() has the wrong semantics for apr_file_writev();
> apr_file_writev() and apr_file_write() are not supposed to block
Thanks, I didn't think about the issue of write_full() blocking.
r109892 uses apr_file_write(), and adds the check for an under-write
as you originally suggested.
> 2) fix the code formatting
Can this please be more specific? I reviewed the style guide at
http://httpd.apache.org/dev/styleguide.html and I believe all of the
code I have modified here is within those guidelines.
Thanks,
-Paul
Re: svn commit: r109832 -
Posted by Jeff Trawick <tr...@gmail.com>.
On Sun, 05 Dec 2004 00:01:05 -0700, Paul Querna <ch...@force-elite.com> wrote:
> Jeff Trawick wrote:
> > we're hosed here if apr_file_write() can only write part of the data
> > (tbytes is < vec[i].iov_len and rv is APR_SUCCESS); don't you also
> > need to bail if tbytes < vec[i].iov_len?
>
> Committed r109865 which uses apr_file_write_full() to hopefully write
> the entire iovec.
>
> According to the docs for apr_file_write_full, if it returns in an error
> condition, you should add the number of bytes written.
1) write_full() has the wrong semantics for apr_file_writev();
apr_file_writev() and apr_file_write() are not supposed to block
2) fix the code formatting
Re: svn commit: r109832 -
Posted by Paul Querna <ch...@force-elite.com>.
Jeff Trawick wrote:
> we're hosed here if apr_file_write() can only write part of the data
> (tbytes is < vec[i].iov_len and rv is APR_SUCCESS); don't you also
> need to bail if tbytes < vec[i].iov_len?
Committed r109865 which uses apr_file_write_full() to hopefully write
the entire iovec.
According to the docs for apr_file_write_full, if it returns in an error
condition, you should add the number of bytes written.
-Paul
Re: svn commit: r109832 - /apr/apr/trunk/CHANGES /apr/apr/trunk/file_io/unix/readwrite.c /apr/apr/trunk/test/Makefile.in /apr/apr/trunk/test/data /apr/apr/trunk/test/testfile.c
Posted by Jeff Trawick <tr...@gmail.com>.
On 4 Dec 2004 23:40:37 -0000, pquerna@apache.org <pq...@apache.org> wrote:
> Author: pquerna
> Date: Sat Dec 4 15:40:37 2004
> New Revision: 109832
>
> URL: http://svn.apache.org/viewcvs?view=rev&rev=109832
> Log:
> * test/testfile.c: Add a test for apr_file_writev().
> * file_io/unix/readwrite.c: Try to write all iovecs out on platforms without writev.
>
> Modified:
> apr/apr/trunk/CHANGES
> apr/apr/trunk/file_io/unix/readwrite.c
> apr/apr/trunk/test/Makefile.in
> apr/apr/trunk/test/data/ (props changed)
> apr/apr/trunk/test/testfile.c
>
> Modified: apr/apr/trunk/CHANGES
> Url: http://svn.apache.org/viewcvs/apr/apr/trunk/CHANGES?view=diff&rev=109832&p1=apr/apr/trunk/CHANGES&r1=109831&p2=apr/apr/trunk/CHANGES&r2=109832
> ==============================================================================
> --- apr/apr/trunk/CHANGES (original)
> +++ apr/apr/trunk/CHANGES Sat Dec 4 15:40:37 2004
> @@ -1,5 +1,9 @@
> Changes for APR 1.1.0
>
> + *) apr_file_writev will now at least try to write all iovecs on platforms
> + that do not support writev.
> + [Paul Querna]
> +
> *) Remove the runtime test for Sendfile versions on FreeBSD. PR 25718.
> [Mike Silbersack <silby silby.com>, Paul Querna]
>
> Modified: apr/apr/trunk/file_io/unix/readwrite.c
> Url: http://svn.apache.org/viewcvs/apr/apr/trunk/file_io/unix/readwrite.c?view=diff&rev=109832&p1=apr/apr/trunk/file_io/unix/readwrite.c&r1=109831&p2=apr/apr/trunk/file_io/unix/readwrite.c&r2=109832
> ==============================================================================
> --- apr/apr/trunk/file_io/unix/readwrite.c (original)
> +++ apr/apr/trunk/file_io/unix/readwrite.c Sat Dec 4 15:40:37 2004
> @@ -241,8 +241,17 @@
> return APR_SUCCESS;
> }
> #else
> - *nbytes = vec[0].iov_len;
> - return apr_file_write(thefile, vec[0].iov_base, nbytes);
> + int i, tbytes;
> + apr_status_t rv = APR_SUCCESS;
> +
> + for(i = 0; i < nvec; i++){
what is with the style here?
> + tbytes = vec[i].iov_len;
> + rv = apr_file_write(thefile, vec[i].iov_base, &tbytes);
> + *nbytes += tbytes;
> + if(rv != APR_SUCCESS)
style
we're hosed here if apr_file_write() can only write part of the data
(tbytes is < vec[i].iov_len and rv is APR_SUCCESS); don't you also
need to bail if tbytes < vec[i].iov_len?
> + break;
> + }
> + return rv;
> #endif
> }