You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Joe Orton <jo...@orton.demon.co.uk> on 2000/07/05 11:32:21 UTC
[PATCH] mod_dav: APRize fs/repos.c
This APRizes dav/fs/repos.c... issues:
* I couldn't find a 'rename()' equivalent in APR
* sdbm really needs APRizing too
* dav_new_error uses errno, I think this needs to be
changed to take an ap_status_t instead?
* mod_dav.c needs checking for the size_t->ap_ssize_t change
in the stream functions.
There's some memory corruption or something going on in dav/main/props.c
at the moment which I'm trying to track down, it's causing random
segfaults.
joe
(ps. Are inlined or attached diffs preffered?)
Index: fs/repos.c
===================================================================
RCS file: /home/joe/lib/cvsroot/apache2/src/modules/dav/fs/repos.c,v
retrieving revision 1.8
diff -u -p -r1.8 repos.c
--- repos.c 2000/07/03 22:50:51 1.8
+++ repos.c 2000/07/05 09:17:56
@@ -59,12 +59,8 @@
#include <string.h>
/* ### this stuff is temporary... need APR */
-#include <sys/stat.h>
-#include <fcntl.h>
-#include <unistd.h>
-#ifndef O_BINARY
-#define O_BINARY (0)
-#endif
+#include <unistd.h> /* ### still have rename */
+#include "apr_file_io.h"
#include "httpd.h"
#include "http_log.h"
@@ -181,7 +177,7 @@ static const dav_fs_liveprop_name dav_fs
/* define the dav_stream structure for our use */
struct dav_stream {
ap_pool_t *p;
- int fd;
+ ap_file_t *f;
const char *pathname; /* we may need to remove it at close time */
};
@@ -260,19 +256,19 @@ static void dav_format_time(int style, a
tms.tm_hour, tms.tm_min, tms.tm_sec);
}
-static int dav_sync_write(int fd, const char *buf, ssize_t bufsize)
+static ap_status_t dav_sync_write(ap_file_t *f, const char *buf, ap_ssize_t bufsize)
{
- ssize_t amt;
-
+ ap_status_t status;
+ ap_ssize_t amt;
+
do {
- amt = write(fd, buf, bufsize);
- if (amt > 0) {
- bufsize -= amt;
- buf += amt;
- }
- } while (amt > 0 && bufsize > 0);
+ amt = bufsize;
+ status = ap_write(f, buf, &amt);
+ bufsize -= amt;
+ buf += amt;
+ } while (status == APR_SUCCESS && bufsize > 0);
- return amt < 0 ? -1 : 0;
+ return status;
}
static dav_error * dav_fs_copymove_file(
@@ -283,24 +279,25 @@ static dav_error * dav_fs_copymove_file(
dav_buffer *pbuf)
{
dav_buffer work_buf = { 0 };
- int fdi;
- int fdo;
+ ap_file_t *inf;
+ ap_file_t *outf;
if (pbuf == NULL)
pbuf = &work_buf;
dav_set_bufsize(p, pbuf, DAV_FS_COPY_BLOCKSIZE);
- if ((fdi = open(src, O_RDONLY | O_BINARY)) == -1) {
+ if ((ap_open(&inf, src, APR_READ | APR_BINARY, APR_OS_DEFAULT, p))
+ != APR_SUCCESS) {
/* ### use something besides 500? */
return dav_new_error(p, HTTP_INTERNAL_SERVER_ERROR, 0,
"Could not open file for reading");
}
/* ### do we need to deal with the umask? */
- if ((fdo = open(dst, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
- DAV_FS_MODE_FILE)) == -1) {
- close(fdi);
+ if ((ap_open(&outf, dst, APR_WRITE | APR_CREATE | APR_TRUNCATE | APR_BINARY,
+ APR_OS_DEFAULT, p)) != APR_SUCCESS) {
+ ap_close(inf);
/* ### use something besides 500? */
return dav_new_error(p, HTTP_INTERNAL_SERVER_ERROR, 0,
@@ -308,13 +305,18 @@ static dav_error * dav_fs_copymove_file(
}
while (1) {
- ssize_t len = read(fdi, pbuf->buf, DAV_FS_COPY_BLOCKSIZE);
-
- if (len == -1) {
- close(fdi);
- close(fdo);
+ ap_ssize_t len = DAV_FS_COPY_BLOCKSIZE;
+ ap_status_t status;
- if (remove(dst) != 0) {
+ status = ap_read(inf, pbuf->buf, &len);
+ if (status == APR_EOF) {
+ break;
+ }
+ else if (status != APR_SUCCESS) {
+ ap_close(inf);
+ ap_close(outf);
+
+ if (ap_remove_file(dst, p) != APR_SUCCESS) {
/* ### ACK! Inconsistent state... */
/* ### use something besides 500? */
@@ -328,14 +330,12 @@ static dav_error * dav_fs_copymove_file(
return dav_new_error(p, HTTP_INTERNAL_SERVER_ERROR, 0,
"Could not read input file");
}
- if (len == 0)
- break;
- if (dav_sync_write(fdo, pbuf->buf, len) != 0) {
+ if (dav_sync_write(outf, pbuf->buf, len) != APR_SUCCESS) {
int save_errno = errno;
- close(fdi);
- close(fdo);
+ ap_close(inf);
+ ap_close(outf);
if (remove(dst) != 0) {
/* ### ACK! Inconsistent state... */
@@ -359,8 +359,8 @@ static dav_error * dav_fs_copymove_file(
}
}
- close(fdi);
- close(fdo);
+ ap_close(inf);
+ ap_close(outf);
if (is_move && remove(src) != 0) {
dav_error *err;
@@ -416,7 +416,7 @@ static dav_error * dav_fs_copymove_state
/* ### do we need to deal with the umask? */
/* ensure that it exists */
- if (mkdir(dst, DAV_FS_MODE_DIR) != 0) {
+ if (ap_make_dir(dst, APR_OS_DEFAULT, p) != 0) {
if (errno != EEXIST) {
/* ### use something besides 500? */
return dav_new_error(p, HTTP_INTERNAL_SERVER_ERROR, 0,
@@ -759,36 +759,33 @@ static dav_error * dav_fs_open_stream(co
{
ap_pool_t *p = resource->info->pool;
dav_stream *ds = ap_palloc(p, sizeof(*ds));
- int flags;
+ ap_int32_t flags;
switch (mode) {
case DAV_MODE_READ:
case DAV_MODE_READ_SEEKABLE:
default:
- flags = O_RDONLY;
+ flags = APR_READ; /* ### | APR_BINARY? */
break;
case DAV_MODE_WRITE_TRUNC:
- flags = O_WRONLY | O_CREAT | O_TRUNC | O_BINARY;
+ flags = APR_WRITE | APR_CREATE | APR_TRUNCATE | APR_BINARY;
break;
case DAV_MODE_WRITE_SEEKABLE:
- flags = O_WRONLY | O_CREAT | O_BINARY;
+ flags = APR_WRITE | APR_CREATE | APR_BINARY;
break;
}
ds->p = p;
ds->pathname = resource->info->pathname;
- ds->fd = open(ds->pathname, flags, DAV_FS_MODE_FILE);
- if (ds->fd == -1) {
+ if (ap_open(&ds->f, ds->pathname, flags, APR_OS_DEFAULT,
+ ds->p) != APR_SUCCESS) {
/* ### use something besides 500? */
return dav_new_error(p, HTTP_INTERNAL_SERVER_ERROR, 0,
"An error occurred while opening a resource.");
}
- /* ### need to fix this */
-#if 0
- ap_note_cleanups_for_fd(p, ds->fd);
-#endif
+ /* (APR registers cleanups for the fd with the pool) */
*stream = ds;
return NULL;
@@ -796,14 +793,10 @@ static dav_error * dav_fs_open_stream(co
static dav_error * dav_fs_close_stream(dav_stream *stream, int commit)
{
- /* ### need to fix this */
-#if 0
- ap_kill_cleanups_for_fd(stream->p, stream->fd);
-#endif
- close(stream->fd);
+ ap_close(stream->f);
if (!commit) {
- if (remove(stream->pathname) != 0) {
+ if (ap_remove_file(stream->pathname, stream->p) != 0) {
/* ### use a better description? */
return dav_new_error(stream->p, HTTP_INTERNAL_SERVER_ERROR, 0,
"There was a problem removing (rolling "
@@ -816,26 +809,24 @@ static dav_error * dav_fs_close_stream(d
}
static dav_error * dav_fs_read_stream(dav_stream *stream,
- void *buf, size_t *bufsize)
+ void *buf, ap_ssize_t *bufsize)
{
- ssize_t amt;
-
- amt = read(stream->fd, buf, *bufsize);
- if (amt == -1) {
+ if (ap_read(stream->f, buf, bufsize) != APR_SUCCESS) {
/* ### use something besides 500? */
return dav_new_error(stream->p, HTTP_INTERNAL_SERVER_ERROR, 0,
"An error occurred while reading from a "
"resource.");
}
- *bufsize = (size_t)amt;
return NULL;
}
static dav_error * dav_fs_write_stream(dav_stream *stream,
- const void *buf, size_t bufsize)
+ const void *buf, ap_ssize_t bufsize)
{
- if (dav_sync_write(stream->fd, buf, bufsize) != 0) {
- if (errno == ENOSPC) {
+ ap_status_t status;
+
+ if ((status = dav_sync_write(stream->f, buf, bufsize)) != APR_SUCCESS) {
+ if (status == APR_ENOSPC) {
return dav_new_error(stream->p, HTTP_INSUFFICIENT_STORAGE, 0,
"There is not enough storage to write to "
"this resource.");
@@ -849,9 +840,11 @@ static dav_error * dav_fs_write_stream(d
return NULL;
}
-static dav_error * dav_fs_seek_stream(dav_stream *stream, off_t abs_pos)
+static dav_error * dav_fs_seek_stream(dav_stream *stream, ap_off_t abs_pos)
{
- if (lseek(stream->fd, abs_pos, SEEK_SET) == (off_t)-1) {
+ if (ap_seek(stream->f, APR_SET, &abs_pos) != APR_SUCCESS) {
+ /* ### should check whether ap_seek set abs_pos was set to the
+ * correct position? */
/* ### use something besides 500? */
return dav_new_error(stream->p, HTTP_INTERNAL_SERVER_ERROR, 0,
"Could not seek to specified position in the "
@@ -906,13 +899,15 @@ static void dav_fs_free_file(void *free_
static dav_error * dav_fs_create_collection(ap_pool_t *p, dav_resource *resource)
{
dav_resource_private *ctx = resource->info;
-
- if (mkdir(ctx->pathname, DAV_FS_MODE_DIR) != 0) {
- if (errno == ENOSPC)
- return dav_new_error(p, HTTP_INSUFFICIENT_STORAGE, 0,
- "There is not enough storage to create "
- "this collection.");
+ ap_status_t status;
+ status = ap_make_dir(ctx->pathname, APR_OS_DEFAULT, p);
+ if (status == ENOSPC) {
+ return dav_new_error(p, HTTP_INSUFFICIENT_STORAGE, 0,
+ "There is not enough storage to create "
+ "this collection.");
+ }
+ else if (status != APR_SUCCESS) {
/* ### refine this error message? */
return dav_new_error(p, HTTP_FORBIDDEN, 0,
"Unable to create collection.");
@@ -941,7 +936,8 @@ static dav_error * dav_fs_copymove_walke
}
else {
/* copy/move of a collection. Create the new, target collection */
- if (mkdir(dstinfo->pathname, DAV_FS_MODE_DIR) != 0) {
+ if (ap_make_dir(dstinfo->pathname, APR_OS_DEFAULT, ctx->pool)
+ != APR_SUCCESS) {
/* ### assume it was a permissions problem */
/* ### need a description here */
err = dav_new_error(ctx->pool, HTTP_FORBIDDEN, 0, NULL);
@@ -949,8 +945,9 @@ static dav_error * dav_fs_copymove_walke
}
}
else {
- err = dav_fs_copymove_file(ctx->is_move, ctx->pool, srcinfo->pathname,
- dstinfo->pathname, &ctx->work_buf);
+ err = dav_fs_copymove_file(ctx->is_move, ctx->pool,
+ srcinfo->pathname, dstinfo->pathname,
+ &ctx->work_buf);
/* ### push a higher-level description? */
}
@@ -1131,6 +1128,7 @@ static dav_error * dav_fs_move_resource(
/* no multistatus response */
*response = NULL;
+ /* ### APR has no rename? */
if (rename(srcinfo->pathname, dstinfo->pathname) != 0) {
/* ### should have a better error than this. */
return dav_new_error(srcinfo->pool, HTTP_INTERNAL_SERVER_ERROR, 0,
@@ -1277,8 +1275,7 @@ static dav_error * dav_fs_walker(dav_fs_
dav_error *err = NULL;
dav_walker_ctx *wctx = fsctx->wctx;
int isdir = wctx->resource->collection;
- DIR *dirp;
- struct dirent *ep;
+ ap_dir_t *dirp;
/* ensure the context is prepared properly, then call the func */
err = (*wctx->func)(wctx,
@@ -1316,16 +1313,19 @@ static dav_error * dav_fs_walker(dav_fs_
fsctx->res2.collection = 0;
/* open and scan the directory */
- if ((dirp = opendir(fsctx->path1.buf)) == NULL) {
+ if ((ap_opendir(&dirp, fsctx->path1.buf, wctx->pool)) != APR_SUCCESS) {
/* ### need a better error */
return dav_new_error(wctx->pool, HTTP_NOT_FOUND, 0, NULL);
}
- while ((ep = readdir(dirp)) != NULL) {
- size_t len = strlen(ep->d_name);
+ while ((ap_readdir(dirp)) == APR_SUCCESS) {
+ char *name;
+ size_t len;
+
+ ap_get_dir_filename(&name,dirp);
+ len = strlen(name);
/* avoid recursing into our current, parent, or state directories */
- if (ep->d_name[0] == '.'
- && (len == 1 || (ep->d_name[1] == '.' && len == 2))) {
+ if (name[0] == '.' && (len == 1 || (name[1] == '.' && len == 2))) {
continue;
}
@@ -1334,19 +1334,18 @@ static dav_error * dav_fs_walker(dav_fs_
/* ### example: .htaccess is normally configured to fail auth */
/* stuff in the state directory is never authorized! */
- if (!strcmp(ep->d_name, DAV_FS_STATE_DIR)) {
+ if (!strcmp(name, DAV_FS_STATE_DIR)) {
continue;
}
}
/* skip the state dir unless a HIDDEN is performed */
if (!(wctx->walk_type & DAV_WALKTYPE_HIDDEN)
- && !strcmp(ep->d_name, DAV_FS_STATE_DIR)) {
+ && !strcmp(name, DAV_FS_STATE_DIR)) {
continue;
}
/* append this file onto the path buffer (copy null term) */
- dav_buffer_place_mem(wctx->pool,
- &fsctx->path1, ep->d_name, len + 1, 0);
+ dav_buffer_place_mem(wctx->pool, &fsctx->path1, name, len + 1, 0);
if (ap_lstat(&fsctx->info1.finfo, fsctx->path1.buf, wctx->pool) != 0) {
/* woah! where'd it go? */
@@ -1357,12 +1356,11 @@ static dav_error * dav_fs_walker(dav_fs_
/* copy the file to the URI, too. NOTE: we will pad an extra byte
for the trailing slash later. */
- dav_buffer_place_mem(wctx->pool, &wctx->uri, ep->d_name, len + 1, 1);
+ dav_buffer_place_mem(wctx->pool, &wctx->uri, name, len + 1, 1);
/* if there is a secondary path, then do that, too */
if (fsctx->path2.buf != NULL) {
- dav_buffer_place_mem(wctx->pool, &fsctx->path2,
- ep->d_name, len + 1, 0);
+ dav_buffer_place_mem(wctx->pool, &fsctx->path2, name, len + 1, 0);
}
/* set up the (internal) pathnames for the two resources */
@@ -1418,7 +1416,7 @@ static dav_error * dav_fs_walker(dav_fs_
}
/* ### check the return value of this? */
- closedir(dirp);
+ ap_closedir(dirp);
if (err != NULL)
return err;
Index: fs/repos.h
===================================================================
RCS file: /home/joe/lib/cvsroot/apache2/src/modules/dav/fs/repos.h,v
retrieving revision 1.3
diff -u -p -r1.3 repos.h
--- repos.h 2000/06/28 11:23:45 1.3
+++ repos.h 2000/07/05 08:56:39
@@ -59,28 +59,22 @@
#ifndef _DAV_FS_REPOS_H_
#define _DAV_FS_REPOS_H_
+#include "apr_file_io.h"
+
/* the subdirectory to hold all DAV-related information for a directory */
#define DAV_FS_STATE_DIR ".DAV"
#define DAV_FS_STATE_FILE_FOR_DIR ".state_for_dir"
#define DAV_FS_LOCK_NULL_FILE ".locknull"
+/* ### this is still needed for sdbm_open()...
+ * sdbm should be APR-ized really. */
#ifndef WIN32
-#define DAV_FS_MODE_DIR (S_IRWXU | S_IRWXG)
#define DAV_FS_MODE_FILE (S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP)
-#define DAV_FS_MODE_XUSR (S_IXUSR)
#else /* WIN32 */
-#define DAV_FS_MODE_DIR (_S_IREAD | _S_IWRITE)
#define DAV_FS_MODE_FILE (_S_IREAD | _S_IWRITE)
-#define DAV_FS_MODE_XUSR (_S_IEXEC)
-
-#include <limits.h>
-
-typedef int ssize_t;
-
-#define mkdir(p,m) _mkdir(p)
#endif /* WIN32 */
Index: main/mod_dav.h
===================================================================
RCS file: /home/joe/lib/cvsroot/apache2/src/modules/dav/main/mod_dav.h,v
retrieving revision 1.6
diff -u -p -r1.6 mod_dav.h
--- mod_dav.h 2000/07/03 22:50:59 1.6
+++ mod_dav.h 2000/07/04 14:32:54
@@ -1459,7 +1459,7 @@ struct dav_hooks_repository
** on each call, until the EOF condition is met.
*/
dav_error * (*read_stream)(dav_stream *stream,
- void *buf, size_t *bufsize);
+ void *buf, ap_ssize_t *bufsize);
/*
** Write data to the stream.
@@ -1467,7 +1467,7 @@ struct dav_hooks_repository
** All of the bytes must be written, or an error should be returned.
*/
dav_error * (*write_stream)(dav_stream *stream,
- const void *buf, size_t bufsize);
+ const void *buf, ap_ssize_t bufsize);
/*
** Seek to an absolute position in the stream. This is used to support
Re: [PATCH] mod_dav: APRize fs/repos.c
Posted by Greg Stein <gs...@lyra.org>.
On Thu, Jul 06, 2000 at 02:39:44AM +0100, Joe Orton wrote:
> Patch attempt two attached, which differs from the previous in
> initializing ap_file_t's to 0.
>
> Incremental to that, below is a forward-port of the "maintain executable
> bit" fix from mod_dav 1.0.
This patch is excellent, as always :-). I basically dropped it right in,
with a few extra mods noted below.
I've got the original and incremental patches applied, but locus is barfing
right now. I can't get the stuff checked in.
Note: I didn't pick up the ap_ssize_t change in the stream API. Those
functiosn will never returned signed data, so there isn't any reason to use
those variables types. (and yes: APR doesn't either, so its type probably
ought to change also)
I also APR-ized lock.c and added ap_rename_file().
Just waiting for locus...
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [PATCH] mod_dav: APRize fs/repos.c
Posted by Joe Orton <jo...@orton.demon.co.uk>.
Patch attempt two attached, which differs from the previous in
initializing ap_file_t's to 0.
Incremental to that, below is a forward-port of the "maintain executable
bit" fix from mod_dav 1.0.
joe
--- repos.c Thu Jul 6 02:26:07 2000
+++ repos.prm.c Thu Jul 6 02:21:49 2000
@@ -276,15 +276,30 @@
ap_pool_t * p,
const char *src,
const char *dst,
+ const ap_finfo_t *src_finfo,
+ const ap_finfo_t *dst_finfo,
dav_buffer *pbuf)
{
dav_buffer work_buf = { 0 };
ap_file_t *inf = NULL;
ap_file_t *outf = NULL;
+ ap_fileperms_t perms;
if (pbuf == NULL)
pbuf = &work_buf;
+ perms = src_finfo->protection;
+
+ /* chmod() the destination if the source is executable, and the
+ * destination already exists. */
+ if ((perms & APR_UEXECUTE) && (dst_finfo != NULL) &&
+ (dst_finfo->filetype != APR_NOFILE)) {
+ if (ap_setfileperms(dst, perms) == -1) {
+ return dav_new_error(p, HTTP_INTERNAL_SERVER_ERROR, 0,
+ "Could not set permissions on destination");
+ }
+ }
+
dav_set_bufsize(p, pbuf, DAV_FS_COPY_BLOCKSIZE);
if ((ap_open(&inf, src, APR_READ | APR_BINARY, APR_OS_DEFAULT, p))
@@ -296,7 +311,7 @@
/* ### do we need to deal with the umask? */
if ((ap_open(&outf, dst, APR_WRITE | APR_CREATE | APR_TRUNCATE | APR_BINARY,
- APR_OS_DEFAULT, p)) != APR_SUCCESS) {
+ perms, p)) != APR_SUCCESS) {
ap_close(inf);
/* ### use something besides 500? */
@@ -454,8 +469,11 @@
}
else
{
- /* gotta copy (and delete) */
- return dav_fs_copymove_file(is_move, p, src, dst, pbuf);
+ /* gotta copy (and delete).
+ * We don't have the finfo of the destination *file*,
+ * only the destination *directory*, so pass NULL. */
+ return dav_fs_copymove_file(is_move, p, src, dst,
+ &src_finfo, NULL, pbuf);
}
return NULL;
@@ -947,6 +965,7 @@
else {
err = dav_fs_copymove_file(ctx->is_move, ctx->pool,
srcinfo->pathname, dstinfo->pathname,
+ &srcinfo->finfo, &dstinfo->finfo,
&ctx->work_buf);
/* ### push a higher-level description? */
}
@@ -1024,6 +1043,7 @@
/* not a collection */
if ((err = dav_fs_copymove_file(is_move, src->info->pool,
src->info->pathname, dst->info->pathname,
+ &src->info->finfo, &dst->info->finfo,
&work_buf)) != NULL) {
/* ### push a higher-level description? */
return err;