httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jeff Trawick \(httpd\)" <traw...@ibm.net>
Subject [PATCH] Re: APR file i/o questions
Date Fri, 16 Jun 2000 13:30:13 GMT
Differences between this one and what Brian posted earlier today:

1) this fixes a problem in where ap_open() where a lock is created for
   non-buffered files
2) this fixes problems setting rv correctly in the ap_read() buffered 
   path
3) since ap_read() works as expected, it is possible to make ap_getc()
   and ap_fgets() even smaller
4) ap_fgets() no longer cares about '\r'

ap_fgets() and ap_getc() are copied in their entirety at the end as
it is hard to piece together the little remaining code from the diff.

This doesn't attempt to change the lock design.

This does add more paths where you don't get both bytes read and
non-zero status, status being saved for the next call.  There were
such paths already and this is a familiar paradigm, so I don't have
any guilt with this.

Unix didn't have the ap_fgets() optimization that Greg S. wanted to
keep in the Win32 implementation and I didn't attempt to implement it
for Unix.

Does anybody want to speak up with specific objections or improvements
before I commit?

Thanks,

Jeff

Index: file_io/unix/open.c
===================================================================
RCS file: /cvs/apache/apache-2.0/src/lib/apr/file_io/unix/open.c,v
retrieving revision 1.54
diff -u -r1.54 open.c
--- open.c	2000/06/15 17:39:16	1.54
+++ open.c	2000/06/16 13:12:37
@@ -82,6 +82,9 @@
 ap_status_t ap_open(ap_file_t **new, const char *fname, ap_int32_t flag,  ap_fileperms_t
perm, ap_pool_t *cont)
 {
     int oflags = 0;
+#if APR_HAS_THREADS
+    ap_status_t rv;
+#endif
 
     if ((*new) == NULL) {
         (*new) = (ap_file_t *)ap_pcalloc(cont, sizeof(ap_file_t));
@@ -90,10 +93,6 @@
     (*new)->cntxt = cont;
     (*new)->oflags = oflags;
     (*new)->filedes = -1;
-    (*new)->buffer = NULL;
-#if APR_HAS_THREADS
-    ap_create_lock(&((*new)->thlock), APR_MUTEX, APR_INTRAPROCESS, NULL, cont);
-#endif
 
     if ((flag & APR_READ) && (flag & APR_WRITE)) {
         oflags = O_RDWR;
@@ -111,6 +110,20 @@
     (*new)->fname = ap_pstrdup(cont, fname);
 
     (*new)->buffered = (flag & APR_BUFFERED) > 0;
+
+    if ((*new)->buffered) {
+        (*new)->buffer = ap_palloc(cont, APR_FILE_BUFSIZE);
+#if APR_HAS_THREADS
+        rv = ap_create_lock(&((*new)->thlock), APR_MUTEX, APR_INTRAPROCESS, 
+                            NULL, cont);
+        if (rv) {
+            return rv;
+        }
+#endif
+    }
+    else {
+        (*new)->buffer = NULL;
+    }
 
     if (flag & APR_CREATE) {
         oflags |= O_CREAT; 
Index: file_io/unix/readwrite.c
===================================================================
RCS file: /cvs/apache/apache-2.0/src/lib/apr/file_io/unix/readwrite.c,v
retrieving revision 1.54
diff -u -r1.54 readwrite.c
--- readwrite.c	2000/06/14 02:58:34	1.54
+++ readwrite.c	2000/06/16 13:12:37
@@ -98,6 +98,9 @@
 }
 #endif
 
+/* problems: 
+ * 1) ungetchar not used for buffered files
+ */
 ap_status_t ap_read(ap_file_t *thefile, void *buf, ap_ssize_t *nbytes)
 {
     ap_ssize_t rv;
@@ -130,8 +133,13 @@
                 thefile->dataRead = read(thefile->filedes, thefile->buffer, APR_FILE_BUFSIZE);
                 if (thefile->dataRead == 0) {
                     thefile->eof_hit = TRUE;
+                    rv = APR_EOF;
                     break;
                 }
+                else if (thefile->dataRead == -1) {
+                    rv = errno;
+                    break;
+                }
                 thefile->filePtr += thefile->dataRead;
                 thefile->bufpos = 0;
             }
@@ -143,7 +151,10 @@
             size -= blocksize;
         }
 
-        *nbytes = rv == 0 ? pos - (char *)buf : 0;
+        *nbytes = pos - (char *)buf;
+        if (*nbytes) {
+            rv = 0;
+        }
 #if APR_HAS_THREADS
         ap_unlock(thefile->thlock);
 #endif
@@ -301,22 +312,9 @@
 
 ap_status_t ap_getc(char *ch, ap_file_t *thefile)
 {
-    ssize_t rv;
-    
-    if (thefile->ungetchar != -1) {
-        *ch = (char) thefile->ungetchar;
-        thefile->ungetchar = -1;
-        return APR_SUCCESS;
-    }
-    rv = read(thefile->filedes, ch, 1); 
-    if (rv == 0) {
-        thefile->eof_hit = TRUE;
-        return APR_EOF;
-    }
-    else if (rv != 1) {
-        return errno;
-    }
-    return APR_SUCCESS; 
+    ap_ssize_t nbytes = 1;
+
+    return ap_read(thefile, ch, &nbytes);
 }
 
 ap_status_t ap_puts(char *str, ap_file_t *thefile)
@@ -356,59 +354,34 @@
 
 ap_status_t ap_fgets(char *str, int len, ap_file_t *thefile)
 {
-    ssize_t rv;
-    int i, used_unget = FALSE, beg_idx;
-
-    if (len <= 1) {  /* as per fgets() */
+    ap_status_t rv = APR_SUCCESS; /* get rid of gcc warning */
+    ap_ssize_t nbytes;
+    char *final = str + len - 1;
+
+    if (len <= 1) {  
+        /* sort of like fgets(), which returns NULL and stores no bytes 
+         */
         return APR_SUCCESS;
     }
 
-    if (thefile->ungetchar != -1) {
-        str[0] = thefile->ungetchar;
-	used_unget = TRUE;
-	beg_idx = 1;
-	if (str[0] == '\n' || str[0] == '\r') {
-	    thefile->ungetchar = -1;
-	    str[1] = '\0';
-	    return APR_SUCCESS;
-	}
-    } 
-    else {
-        beg_idx = 0;
-    }
-    
-    for (i = beg_idx; i < len; i++) {
-        rv = read(thefile->filedes, &str[i], 1); 
-        if (rv == 0) {
-            thefile->eof_hit = TRUE;
-	    if (used_unget) {
-                thefile->filedes = -1;
-            }
-	    str[i] = '\0';
-            return APR_EOF;
-        }
-        else if (rv != 1) {
-            return errno;
+    while (str < final) { /* leave room for trailing '\0' */
+        nbytes = 1;
+        rv = ap_read(thefile, str, &nbytes);
+        if (rv != APR_SUCCESS) {
+            break;
         }
-        if (str[i] == '\n' || str[i] == '\r') {
+        if (*str == '\n') {
+            ++str;
             break;
         }
+        ++str;
     }
-    if (i < len-1) {
-        str[i+1] = '\0';
-    }
-    return APR_SUCCESS; 
-}
-
-#if 0 /* not currently used */
-static int printf_flush(ap_vformatter_buff_t *vbuff)
-{
-    /* I would love to print this stuff out to the file, but I will
-     * get that working later.  :)  For now, just return.
+    /* We must store a terminating '\0' if we've stored any chars. We can
+     * get away with storing it if we hit an error first. 
      */
-    return -1;
+    *str = '\0'; 
+    return rv;
 }
-#endif
 
 APR_EXPORT(int) ap_fprintf(ap_file_t *fptr, const char *format, ...)
 {
Index: test/testfile.c
===================================================================
RCS file: /cvs/apache/apache-2.0/src/lib/apr/test/testfile.c,v
retrieving revision 1.13
diff -u -r1.13 testfile.c
--- testfile.c	2000/05/24 22:32:44	1.13
+++ testfile.c	2000/06/16 13:12:39
@@ -52,6 +52,7 @@
  * <http://www.apache.org/>.
  */
 
+#include <assert.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include "apr_file_io.h"
@@ -65,6 +66,7 @@
 
 int test_filedel(ap_pool_t *);
 int testdirs(ap_pool_t *);
+static void test_read(ap_pool_t *);
 
 int main()
 {
@@ -239,6 +241,7 @@
 
     testdirs(context); 
     test_filedel(context);
+    test_read(context);
 
     return 1;
 }
@@ -382,4 +385,201 @@
     
     return 1; 
 }    
+
+#define TESTREAD_BLKSIZE 1024
+#define APR_BUFFERSIZE   4096 /* This should match APR's buffer size. */
+
+static void create_testread(ap_pool_t *p, const char *fname)
+{
+    ap_file_t *f = NULL;
+    ap_status_t rv;
+    char buf[TESTREAD_BLKSIZE];
+    ap_ssize_t nbytes;
+
+    /* Create a test file with known content.
+     */
+    rv = ap_open(&f, fname, APR_CREATE | APR_WRITE | APR_TRUNCATE, APR_UREAD | APR_UWRITE,
p);
+    if (rv) {
+        fprintf(stderr, "ap_open()->%d/%s\n",
+                rv, ap_strerror(rv, buf, sizeof buf));
+        exit(1);
+    }
+    nbytes = 4;
+    rv = ap_write(f, "abc\n", &nbytes);
+    assert(!rv && nbytes == 4);
+    memset(buf, 'a', sizeof buf);
+    nbytes = sizeof buf;
+    rv = ap_write(f, buf, &nbytes);
+    assert(!rv && nbytes == sizeof buf);
+    nbytes = 2;
+    rv = ap_write(f, "\n\n", &nbytes);
+    assert(!rv && nbytes == 2);
+    rv = ap_close(f);
+    assert(!rv);
+}
+
+static char read_one(ap_file_t *f, int expected)
+{
+  char bytes[3];
+  ap_status_t rv;
+  static int counter = 0;
+  ap_ssize_t nbytes;
+
+  counter += 1;
+
+  bytes[0] = bytes[2] = 0x01;
+  if (counter % 2) {
+      rv = ap_getc(bytes + 1, f);
+  }
+  else {
+      nbytes = 1;
+      rv = ap_read(f, bytes + 1, &nbytes);
+      assert(nbytes == 1);
+  }
+  assert(!rv);
+  assert(bytes[0] == 0x01 && bytes[2] == 0x01);
+  if (expected != -1) {
+      assert(bytes[1] == expected);
+  }
+  return bytes[1];
+}
+
+static void test_read_guts(ap_pool_t *p, const char *fname, ap_int32_t extra_flags)
+{
+    ap_file_t *f = NULL;
+    ap_status_t rv;
+    ap_ssize_t nbytes;
+    char buf[1024];
+    int i;
+
+    rv = ap_open(&f, fname, APR_READ | extra_flags, 0, p);
+    assert(!rv);
+    read_one(f, 'a');
+    read_one(f, 'b');
+    if (extra_flags & APR_BUFFERED) {
+        fprintf(stdout, 
+                "\n\t\tskipping ap_ungetc() for APR_BUFFERED... "
+                "doesn't work yet...\n\t\t\t\t ");
+    }
+    else {
+        rv = ap_ungetc('z', f);
+        assert(!rv);
+        rv = ap_ungetc('a', f);
+        assert(!rv); /* we just overwrote the previously-un-got char */
+        read_one(f, 'a');
+    }
+    read_one(f, 'c');
+    read_one(f, '\n');
+    for (i = 0; i < TESTREAD_BLKSIZE; i++) {
+        read_one(f, 'a');
+    }
+    read_one(f, '\n');
+    read_one(f, '\n');
+    rv = ap_getc(buf, f);
+    assert(rv == APR_EOF);
+    rv = ap_close(f);
+    assert(!rv);
+
+    f = NULL;
+    rv = ap_open(&f, fname, APR_READ | extra_flags, 0, p);
+    assert(!rv);
+    rv = ap_fgets(buf, 10, f);
+    assert(!rv);
+    assert(!strcmp(buf, "abc\n"));
+    /* read first 800 of TESTREAD_BLKSIZE 'a's 
+     */
+    rv = ap_fgets(buf, 801, f);
+    assert(!rv);
+    assert(strlen(buf) == 800);
+    for (i = 0; i < 800; i++) {
+        assert(buf[i] == 'a');
+    }
+    /* read rest of the 'a's and the first newline
+     */
+    rv = ap_fgets(buf, sizeof buf, f);
+    assert(!rv);
+    assert(strlen(buf) == TESTREAD_BLKSIZE - 800 + 1);
+    for (i = 0; i < TESTREAD_BLKSIZE - 800; i++) {
+        assert(buf[i] == 'a');
+    }
+    assert(buf[TESTREAD_BLKSIZE - 800] == '\n');
+    /* read the last newline
+     */
+    rv = ap_fgets(buf, sizeof buf, f);
+    assert(!rv);
+    assert(!strcmp(buf, "\n"));
+    /* get APR_EOF
+     */
+    rv = ap_fgets(buf, sizeof buf, f);
+    assert(rv == APR_EOF);
+    /* get APR_EOF with ap_getc
+     */
+    rv = ap_getc(buf, f);
+    assert(rv == APR_EOF);
+    /* get APR_EOF with ap_read
+     */
+    nbytes = sizeof buf;
+    rv = ap_read(f, buf, &nbytes);
+    assert(rv == APR_EOF);
+    rv = ap_close(f);
+    assert(!rv);
+}
+
+static void test_bigread(ap_pool_t *p, const char *fname, ap_int32_t extra_flags)
+{
+    ap_file_t *f = NULL;
+    ap_status_t rv;
+    char buf[APR_BUFFERSIZE * 2];
+    ap_ssize_t nbytes;
+
+    /* Create a test file with known content.
+     */
+    rv = ap_open(&f, fname, APR_CREATE | APR_WRITE | APR_TRUNCATE, APR_UREAD | APR_UWRITE,
p);
+    if (rv) {
+        fprintf(stderr, "ap_open()->%d/%s\n",
+                rv, ap_strerror(rv, buf, sizeof buf));
+        exit(1);
+    }
+    nbytes = APR_BUFFERSIZE;
+    memset(buf, 0xFE, nbytes);
+    rv = ap_write(f, buf, &nbytes);
+    assert(!rv && nbytes == APR_BUFFERSIZE);
+    rv = ap_close(f);
+    assert(!rv);
+
+    f = NULL;
+    rv = ap_open(&f, fname, APR_READ | extra_flags, 0, p);
+    assert(!rv);
+    nbytes = sizeof buf;
+    rv = ap_read(f, buf, &nbytes);
+    assert(!rv);
+    assert(nbytes == APR_BUFFERSIZE);
+    rv = ap_close(f);
+    assert(!rv);
+}
+
+static void test_read(ap_pool_t *p)
+{
+    const char *fname = "testread.dat";
+    ap_status_t rv;
+
+    fprintf(stdout, "Testing file read functions.\n");
+
+    create_testread(p, fname);
+    fprintf(stdout, "\tBuffered file tests......");
+    test_read_guts(p, fname, APR_BUFFERED);
+    fprintf(stdout, "OK\n");
+    fprintf(stdout, "\tUnbuffered file tests....");
+    test_read_guts(p, fname, 0);
+    fprintf(stdout, "OK\n");
+    fprintf(stdout, "\tMore buffered file tests......");
+    test_bigread(p, fname, APR_BUFFERED);
+    fprintf(stdout, "OK\n");
+    fprintf(stdout, "\tMore unbuffered file tests......");
+    test_bigread(p, fname, 0);
+    fprintf(stdout, "OK\n");
+    rv = ap_remove_file(fname, p);
+    assert(!rv);
+    fprintf(stdout, "\tAll read tests...........OK\n");
+}
 
ap_getc()

ap_status_t ap_getc(char *ch, ap_file_t *thefile)
{
    ap_ssize_t nbytes = 1;

    return ap_read(thefile, ch, &nbytes);
}

ap_fgets()

ap_status_t ap_fgets(char *str, int len, ap_file_t *thefile)
{
    ap_status_t rv = APR_SUCCESS; /* get rid of gcc warning */
    ap_ssize_t nbytes;
    char *final = str + len - 1;

    if (len <= 1) {
        /* sort of like fgets(), which returns NULL and stores no
        bytes
         */
        return APR_SUCCESS;
    }

    while (str < final) { /* leave room for trailing '\0' */
        nbytes = 1;
        rv = ap_read(thefile, str, &nbytes);
        if (rv != APR_SUCCESS) {
            break;
        }
        if (*str == '\n') {
            ++str;
            break;
        }
        ++str;
    }
    /* We must store a terminating '\0' if we've stored any chars. We
        can
     * get away with storing it if we hit an error first.
     */
    *str = '\0';
    return rv;
}

Mime
View raw message