You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by sf...@apache.org on 2009/11/09 14:14:08 UTC
svn commit: r834049 - in /httpd/httpd/trunk: CHANGES modules/dav/fs/lock.c
modules/dav/fs/repos.c
Author: sf
Date: Mon Nov 9 13:14:07 2009
New Revision: 834049
URL: http://svn.apache.org/viewvc?rev=834049&view=rev
Log:
Make PUT with DAV_MODE_WRITE_TRUNC create a temporary file first and, when the
transfer has been completed successfully, move it over the old file.
Since this would break inode keyed locking, switch to filename keyed locking
exclusively.
PR: 39815
Submitted by: Paul Querna, Stefan Fritsch
Modified:
httpd/httpd/trunk/CHANGES
httpd/httpd/trunk/modules/dav/fs/lock.c
httpd/httpd/trunk/modules/dav/fs/repos.c
Modified: httpd/httpd/trunk/CHANGES
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=834049&r1=834048&r2=834049&view=diff
==============================================================================
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Mon Nov 9 13:14:07 2009
@@ -10,6 +10,13 @@
mod_proxy_ftp: NULL pointer dereference on error paths.
[Stefan Fritsch <sf fritsch.de>, Joe Orton]
+ *) mod_dav_fs: Make PUT create files atomically and no longer destroy the
+ old file if the transfer aborted. PR 39815. [Paul Querna, Stefan Fritsch]
+
+ *) mod_dav_fs: Remove inode keyed locking as this conflicts with atomically
+ creating files. This is a format cange of the DavLockDB. The old
+ DavLockDB must be deleted on upgrade. [Stefan Fritsch]
+
*) mod_log_config: Make ${cookie}C correctly match whole cookie names
instead of substrings. PR 28037. [Dan Franklin <dan dan-franklin.com>,
Stefan Fritsch]
Modified: httpd/httpd/trunk/modules/dav/fs/lock.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/dav/fs/lock.c?rev=834049&r1=834048&r2=834049&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/dav/fs/lock.c (original)
+++ httpd/httpd/trunk/modules/dav/fs/lock.c Mon Nov 9 13:14:07 2009
@@ -48,9 +48,7 @@
**
** KEY
**
-** The database is keyed by a key_type unsigned char (DAV_TYPE_INODE or
-** DAV_TYPE_FNAME) followed by inode and device number if possible,
-** otherwise full path (in the case of Win32 or lock-null resources).
+** The database is keyed by the full path.
**
** VALUE
**
@@ -82,9 +80,6 @@
#define DAV_LOCK_DIRECT 1
#define DAV_LOCK_INDIRECT 2
-#define DAV_TYPE_INODE 10
-#define DAV_TYPE_FNAME 11
-
/* ack. forward declare. */
static dav_error * dav_fs_remove_locknull_member(apr_pool_t *p,
@@ -372,68 +367,26 @@
}
/*
-** dav_fs_build_fname_key
-**
-** Given a pathname, build a DAV_TYPE_FNAME lock database key.
+** dav_fs_build_key: Given a resource, return a apr_datum_t key
+** to look up lock information for this file.
*/
-static apr_datum_t dav_fs_build_fname_key(apr_pool_t *p, const char *pathname)
+static apr_datum_t dav_fs_build_key(apr_pool_t *p,
+ const dav_resource *resource)
{
+ const char *pathname = dav_fs_pathname(resource);
apr_datum_t key;
/* ### does this allocation have a proper lifetime? need to check */
/* ### can we use a buffer for this? */
- /* size is TYPE + pathname + null */
- key.dsize = strlen(pathname) + 2;
- key.dptr = apr_palloc(p, key.dsize);
- *key.dptr = DAV_TYPE_FNAME;
- memcpy(key.dptr + 1, pathname, key.dsize - 1);
+ key.dsize = strlen(pathname) + 1;
+ key.dptr = apr_pstrmemdup(p, pathname, key.dsize - 1);
if (key.dptr[key.dsize - 2] == '/')
key.dptr[--key.dsize - 1] = '\0';
return key;
}
/*
-** dav_fs_build_key: Given a resource, return a apr_datum_t key
-** to look up lock information for this file.
-**
-** (inode/dev not supported or file is lock-null):
-** apr_datum_t->dvalue = full path
-**
-** (inode/dev supported and file exists ):
-** apr_datum_t->dvalue = inode, dev
-*/
-static apr_datum_t dav_fs_build_key(apr_pool_t *p,
- const dav_resource *resource)
-{
- const char *file = dav_fs_pathname(resource);
- apr_datum_t key;
- apr_finfo_t finfo;
- apr_status_t rv;
-
- /* ### use lstat() ?? */
- /*
- * XXX: What for platforms with no IDENT (dev/inode)?
- */
- rv = apr_stat(&finfo, file, APR_FINFO_IDENT, p);
- if ((rv == APR_SUCCESS || rv == APR_INCOMPLETE)
- && ((finfo.valid & APR_FINFO_IDENT) == APR_FINFO_IDENT))
- {
- /* ### can we use a buffer for this? */
- key.dsize = 1 + sizeof(finfo.inode) + sizeof(finfo.device);
- key.dptr = apr_palloc(p, key.dsize);
- *key.dptr = DAV_TYPE_INODE;
- memcpy(key.dptr + 1, &finfo.inode, sizeof(finfo.inode));
- memcpy(key.dptr + 1 + sizeof(finfo.inode), &finfo.device,
- sizeof(finfo.device));
-
- return key;
- }
-
- return dav_fs_build_fname_key(p, file);
-}
-
-/*
** dav_fs_lock_expired: return 1 (true) if the given timeout is in the past
** or present (the lock has expired), or 0 (false) if in the future
** (the lock has not yet expired).
@@ -626,15 +579,14 @@
need_save = DAV_TRUE;
/* Remove timed-out locknull fm .locknull list */
- if (*key.dptr == DAV_TYPE_FNAME) {
- const char *fname = key.dptr + 1;
+ {
apr_finfo_t finfo;
apr_status_t rv;
/* if we don't see the file, then it's a locknull */
- rv = apr_stat(&finfo, fname, APR_FINFO_MIN | APR_FINFO_LINK, p);
+ rv = apr_stat(&finfo, key.dptr, APR_FINFO_MIN | APR_FINFO_LINK, p);
if (rv != APR_SUCCESS && rv != APR_INCOMPLETE) {
- if ((err = dav_fs_remove_locknull_member(p, fname, &buf)) != NULL) {
+ if ((err = dav_fs_remove_locknull_member(p, key.dptr, &buf)) != NULL) {
/* ### push a higher-level description? */
return err;
}
@@ -989,13 +941,8 @@
/*
** dav_fs_remove_locknull_state: Given a request, check to see if r->filename
-** is/was a lock-null resource. If so, return it to an existant state.
-**
-** ### this function is broken... it doesn't check!
-**
-** In this implementation, this involves two things:
-** (a) remove it from the list in the appropriate .DAV/locknull file
-** (b) on *nix, convert the key from a filename to an inode.
+** is/was a lock-null resource. If so, return it to an existant state, i.e.
+** remove it from the list in the appropriate .DAV/locknull file.
*/
static dav_error * dav_fs_remove_locknull_state(
dav_lockdb *lockdb,
@@ -1011,35 +958,6 @@
return err;
}
- {
- dav_lock_discovery *ld;
- dav_lock_indirect *id;
- apr_datum_t key;
-
- /*
- ** Fetch the lock(s) that made the resource lock-null. Remove
- ** them under the filename key. Obtain the new inode key, and
- ** save the same lock information under it.
- */
- key = dav_fs_build_fname_key(p, pathname);
- if ((err = dav_fs_load_lock_record(lockdb, key, DAV_CREATE_LIST,
- &ld, &id)) != NULL) {
- /* ### insert a higher-level error description */
- return err;
- }
-
- if ((err = dav_fs_save_lock_record(lockdb, key, NULL, NULL)) != NULL) {
- /* ### insert a higher-level error description */
- return err;
- }
-
- key = dav_fs_build_key(p, resource);
- if ((err = dav_fs_save_lock_record(lockdb, key, ld, id)) != NULL) {
- /* ### insert a higher-level error description */
- return err;
- }
- }
-
return NULL;
}
Modified: httpd/httpd/trunk/modules/dav/fs/repos.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/dav/fs/repos.c?rev=834049&r1=834048&r2=834049&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/dav/fs/repos.c (original)
+++ httpd/httpd/trunk/modules/dav/fs/repos.c Mon Nov 9 13:14:07 2009
@@ -140,6 +140,11 @@
*/
#define DAV_PROPID_FS_executable 1
+/*
+ * prefix for temporary files
+ */
+#define DAV_FS_TMP_PREFIX ".davfs."
+
static const dav_liveprop_spec dav_fs_props[] =
{
/* standard DAV properties */
@@ -192,6 +197,7 @@
apr_pool_t *p;
apr_file_t *f;
const char *pathname; /* we may need to remove it at close time */
+ const char *temppath;
};
/* returns an appropriate HTTP status code given an APR status code for a
@@ -852,6 +858,14 @@
&& ctx2->pathname[len1] == '/');
}
+static apr_status_t tmpfile_cleanup(void *data) {
+ dav_stream *ds = data;
+ if (ds->temppath) {
+ apr_file_remove(ds->temppath, ds->p);
+ }
+ return APR_SUCCESS;
+}
+
static dav_error * dav_fs_open_stream(const dav_resource *resource,
dav_stream_mode mode,
dav_stream **stream)
@@ -876,7 +890,19 @@
ds->p = p;
ds->pathname = resource->info->pathname;
- rv = apr_file_open(&ds->f, ds->pathname, flags, APR_OS_DEFAULT, ds->p);
+ ds->temppath = NULL;
+
+ if (mode == DAV_MODE_WRITE_TRUNC) {
+ ds->temppath = apr_pstrcat(p, ap_make_dirstr_parent(p, ds->pathname),
+ DAV_FS_TMP_PREFIX "XXXXXX", NULL);
+ rv = apr_file_mktemp(&ds->f, ds->temppath, flags, ds->p);
+ apr_pool_cleanup_register(p, ds, tmpfile_cleanup,
+ apr_pool_cleanup_null);
+ }
+ else {
+ rv = apr_file_open(&ds->f, ds->pathname, flags, APR_OS_DEFAULT, ds->p);
+ }
+
if (rv != APR_SUCCESS) {
return dav_new_error(p, MAP_IO2HTTP(rv), 0,
"An error occurred while opening a resource.");
@@ -890,16 +916,32 @@
static dav_error * dav_fs_close_stream(dav_stream *stream, int commit)
{
+ apr_status_t rv;
+
apr_file_close(stream->f);
if (!commit) {
- if (apr_file_remove(stream->pathname, stream->p) != APR_SUCCESS) {
- /* ### use a better description? */
- return dav_new_error(stream->p, HTTP_INTERNAL_SERVER_ERROR, 0,
- "There was a problem removing (rolling "
- "back) the resource "
- "when it was being closed.");
+ if (stream->temppath) {
+ apr_pool_cleanup_run(stream->p, stream, tmpfile_cleanup);
+ }
+ else {
+ if (apr_file_remove(stream->pathname, stream->p) != APR_SUCCESS) {
+ /* ### use a better description? */
+ return dav_new_error(stream->p, HTTP_INTERNAL_SERVER_ERROR, 0,
+ "There was a problem removing (rolling "
+ "back) the resource "
+ "when it was being closed.");
+ }
+ }
+ }
+ else if (stream->temppath) {
+ rv = apr_file_rename(stream->temppath, stream->pathname, stream->p);
+ if (rv) {
+ return dav_new_error(stream->p, HTTP_INTERNAL_SERVER_ERROR, rv,
+ "There was a problem writing the file "
+ "atomically after writes.");
}
+ apr_pool_cleanup_kill(stream->p, stream, tmpfile_cleanup);
}
return NULL;
@@ -1451,14 +1493,18 @@
/* ### need to authorize each file */
/* ### example: .htaccess is normally configured to fail auth */
- /* stuff in the state directory is never authorized! */
- if (!strcmp(dirent.name, DAV_FS_STATE_DIR)) {
+ /* stuff in the state directory and temp files are never authorized! */
+ if (!strcmp(dirent.name, DAV_FS_STATE_DIR) ||
+ !strncmp(dirent.name, DAV_FS_TMP_PREFIX,
+ strlen(DAV_FS_TMP_PREFIX))) {
continue;
}
}
- /* skip the state dir unless a HIDDEN is performed */
+ /* skip the state dir and temp files unless a HIDDEN is performed */
if (!(params->walk_type & DAV_WALKTYPE_HIDDEN)
- && !strcmp(dirent.name, DAV_FS_STATE_DIR)) {
+ && (!strcmp(dirent.name, DAV_FS_STATE_DIR) ||
+ !strncmp(dirent.name, DAV_FS_TMP_PREFIX,
+ strlen(DAV_FS_TMP_PREFIX)))) {
continue;
}
Re: svn commit: r834049 - in /httpd/httpd/trunk: CHANGES modules/dav/fs/lock.c modules/dav/fs/repos.c
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Monday 09 November 2009, Greg Stein wrote:
> On Mon, Nov 9, 2009 at 14:46, Stefan Fritsch <sf...@sfritsch.de> wrote:
> > On Monday 09 November 2009, Greg Stein wrote:
> >> >> Why did you go with a format change of the DAVLockDB? It is
> >> >> quite possible that people will miss that step during an
> >> >> upgrade. You could just leave DAV_TYPE_FNAME in there.
> >> >
> >> > That wouldn't help because it would still break with
> >> > DAV_TYPE_INODE locks existing in the DAVLockDB. Or am I
> >> > missing something?
> >>
> >> Heh. Yeah. I realized that right after I hit the Send button :-P
> >>
> >> Tho: mod_dav could error if it sees an unrecognized type, rather
> >> than simply misinterpreting the data and silently unlocking all
> >> nodes.
> >
> > What do you want to do exactly? Check the db at httpd startup and
> > abort if it contains old format entries? I don't think it is
> > possible to convert the entries, at least not without traversing
> > the whole dav tree.
>
> Oh dear, no. I was just thinking of dropping a log like, "cannot
> open DAVLockDB at /some/path; it has old-style entries."
>
> That would effectively shut off DAV and leave a reason for why it
> happened. Quite enough for a sysadmin to figure out how to fix it.
> If the sysadmin missed the "erase your db" step, then there will
> be a silent failure (locks will be missed).
> > In any case, I wonder if this is worth the effort. It definitely
> > isn't
>
> Not suggest any more work than to revert the removal of the TYPE
> byte from the lock record. Then add a simple check/error if the
> type is not FNAME.
We would have to search through all the keys in the DB. This would
mean that we have to read the whole lockdb on every server restart.
This is quite a step from the current behaviour of only opening the
lockDB when there actually is a lock request. I don't think this is
what we want.
Not removing the TYPE byte would however mean that locknull locks are
not lost and, more importantly, it would keep the db format unchanged
on platforms that don't have inodes anyway. Therefore I agree that
this is a good idea.
> > for 2.2 -> 2.4 upgrades. And if we backport the changes to 2.2.x,
> > I would still primarily see it as responsibility of the
> > distributors to warn the user/remove the old db in postinst/etc.
> >
> > And for those people who compile it themselves and run a system
> > critical enough that they cannot affort to loose the locks during
> > an httpd upgrade, those people should really read the changelog.
>
> Sure, but I'd rather make it a bit easier for them. We don't simply
> change directives on people without trying to do something
> intelligent for when we see the old format. We never just say
> "well, we'll silently misinterpret that. them's the breaks."
Re: svn commit: r834049 - in /httpd/httpd/trunk: CHANGES
modules/dav/fs/lock.c modules/dav/fs/repos.c
Posted by Greg Stein <gs...@gmail.com>.
On Mon, Nov 9, 2009 at 15:21, Greg Stein <gs...@gmail.com> wrote:
> On Mon, Nov 9, 2009 at 14:46, Stefan Fritsch <sf...@sfritsch.de> wrote:
>> On Monday 09 November 2009, Greg Stein wrote:
>>> >> Why did you go with a format change of the DAVLockDB? It is
>>> >> quite possible that people will miss that step during an
>>> >> upgrade. You could just leave DAV_TYPE_FNAME in there.
>>> >
>>> > That wouldn't help because it would still break with
>>> > DAV_TYPE_INODE locks existing in the DAVLockDB. Or am I missing
>>> > something?
>>>
>>> Heh. Yeah. I realized that right after I hit the Send button :-P
>>>
>>> Tho: mod_dav could error if it sees an unrecognized type, rather
>>> than simply misinterpreting the data and silently unlocking all
>>> nodes.
>>
>> What do you want to do exactly? Check the db at httpd startup and
>> abort if it contains old format entries? I don't think it is possible
>> to convert the entries, at least not without traversing the whole dav
>> tree.
>
> Oh dear, no. I was just thinking of dropping a log like, "cannot open
> DAVLockDB at /some/path; it has old-style entries."
>
> That would effectively shut off DAV and leave a reason for why it
> happened. Quite enough for a sysadmin to figure out how to fix it.
To clarify this comment I made:
> If
> the sysadmin missed the "erase your db" step, then there will be a
> silent failure (locks will be missed).
I mean: using the *current* code, as checked-in, there will be a
silent failure to recognize old/outstanding locks.
I think it would be best for us to at least say "woah. you didn't read
the instructions. go RTFM." The admin can then decide to blast the
lockdb, or avoid the upgrade. Their choice.
Cheers,
-g
>
>> In any case, I wonder if this is worth the effort. It definitely isn't
>
> Not suggest any more work than to revert the removal of the TYPE byte
> from the lock record. Then add a simple check/error if the type is not
> FNAME.
>
>> for 2.2 -> 2.4 upgrades. And if we backport the changes to 2.2.x, I
>> would still primarily see it as responsibility of the distributors to
>> warn the user/remove the old db in postinst/etc.
>>
>> And for those people who compile it themselves and run a system
>> critical enough that they cannot affort to loose the locks during an
>> httpd upgrade, those people should really read the changelog.
>
> Sure, but I'd rather make it a bit easier for them. We don't simply
> change directives on people without trying to do something intelligent
> for when we see the old format. We never just say "well, we'll
> silently misinterpret that. them's the breaks."
>
> Cheers,
> -g
>
Re: svn commit: r834049 - in /httpd/httpd/trunk: CHANGES
modules/dav/fs/lock.c modules/dav/fs/repos.c
Posted by Greg Stein <gs...@gmail.com>.
On Mon, Nov 9, 2009 at 14:46, Stefan Fritsch <sf...@sfritsch.de> wrote:
> On Monday 09 November 2009, Greg Stein wrote:
>> >> Why did you go with a format change of the DAVLockDB? It is
>> >> quite possible that people will miss that step during an
>> >> upgrade. You could just leave DAV_TYPE_FNAME in there.
>> >
>> > That wouldn't help because it would still break with
>> > DAV_TYPE_INODE locks existing in the DAVLockDB. Or am I missing
>> > something?
>>
>> Heh. Yeah. I realized that right after I hit the Send button :-P
>>
>> Tho: mod_dav could error if it sees an unrecognized type, rather
>> than simply misinterpreting the data and silently unlocking all
>> nodes.
>
> What do you want to do exactly? Check the db at httpd startup and
> abort if it contains old format entries? I don't think it is possible
> to convert the entries, at least not without traversing the whole dav
> tree.
Oh dear, no. I was just thinking of dropping a log like, "cannot open
DAVLockDB at /some/path; it has old-style entries."
That would effectively shut off DAV and leave a reason for why it
happened. Quite enough for a sysadmin to figure out how to fix it. If
the sysadmin missed the "erase your db" step, then there will be a
silent failure (locks will be missed).
> In any case, I wonder if this is worth the effort. It definitely isn't
Not suggest any more work than to revert the removal of the TYPE byte
from the lock record. Then add a simple check/error if the type is not
FNAME.
> for 2.2 -> 2.4 upgrades. And if we backport the changes to 2.2.x, I
> would still primarily see it as responsibility of the distributors to
> warn the user/remove the old db in postinst/etc.
>
> And for those people who compile it themselves and run a system
> critical enough that they cannot affort to loose the locks during an
> httpd upgrade, those people should really read the changelog.
Sure, but I'd rather make it a bit easier for them. We don't simply
change directives on people without trying to do something intelligent
for when we see the old format. We never just say "well, we'll
silently misinterpret that. them's the breaks."
Cheers,
-g
Re: svn commit: r834049 - in /httpd/httpd/trunk: CHANGES modules/dav/fs/lock.c modules/dav/fs/repos.c
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Monday 09 November 2009, Greg Stein wrote:
> >> Why did you go with a format change of the DAVLockDB? It is
> >> quite possible that people will miss that step during an
> >> upgrade. You could just leave DAV_TYPE_FNAME in there.
> >
> > That wouldn't help because it would still break with
> > DAV_TYPE_INODE locks existing in the DAVLockDB. Or am I missing
> > something?
>
> Heh. Yeah. I realized that right after I hit the Send button :-P
>
> Tho: mod_dav could error if it sees an unrecognized type, rather
> than simply misinterpreting the data and silently unlocking all
> nodes.
What do you want to do exactly? Check the db at httpd startup and
abort if it contains old format entries? I don't think it is possible
to convert the entries, at least not without traversing the whole dav
tree.
In any case, I wonder if this is worth the effort. It definitely isn't
for 2.2 -> 2.4 upgrades. And if we backport the changes to 2.2.x, I
would still primarily see it as responsibility of the distributors to
warn the user/remove the old db in postinst/etc.
And for those people who compile it themselves and run a system
critical enough that they cannot affort to loose the locks during an
httpd upgrade, those people should really read the changelog.
Cheers,
Stefan
Re: svn commit: r834049 - in /httpd/httpd/trunk: CHANGES
modules/dav/fs/lock.c modules/dav/fs/repos.c
Posted by Greg Stein <gs...@gmail.com>.
On Mon, Nov 9, 2009 at 08:42, Stefan Fritsch <sf...@sfritsch.de> wrote:
> On Monday 09 November 2009, Greg Stein wrote:
>> On Mon, Nov 9, 2009 at 08:14, <sf...@apache.org> wrote:
>> > Author: sf
>> > Date: Mon Nov 9 13:14:07 2009
>> > New Revision: 834049
>> >
>> > URL: http://svn.apache.org/viewvc?rev=834049&view=rev
>> > Log:
>> > Make PUT with DAV_MODE_WRITE_TRUNC create a temporary file first
>> > and, when the transfer has been completed successfully, move it
>> > over the old file.
>> >
>> > Since this would break inode keyed locking, switch to filename
>> > keyed locking exclusively.
>> >
>> > PR: 39815
>> > Submitted by: Paul Querna, Stefan Fritsch
>> >
>> > Modified:
>> > httpd/httpd/trunk/CHANGES
>> > httpd/httpd/trunk/modules/dav/fs/lock.c
>> > httpd/httpd/trunk/modules/dav/fs/repos.c
>> >
>> > Modified: httpd/httpd/trunk/CHANGES
>> > URL:
>> > http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=834049
>> >&r1=834048&r2=834049&view=diff
>> > =================================================================
>> >============= --- httpd/httpd/trunk/CHANGES [utf-8] (original)
>> > +++ httpd/httpd/trunk/CHANGES [utf-8] Mon Nov 9 13:14:07 2009
>> > @@ -10,6 +10,13 @@
>> > mod_proxy_ftp: NULL pointer dereference on error paths.
>> > [Stefan Fritsch <sf fritsch.de>, Joe Orton]
>> >
>> > + *) mod_dav_fs: Make PUT create files atomically and no longer
>> > destroy the + old file if the transfer aborted. PR 39815.
>> > [Paul Querna, Stefan Fritsch] +
>> > + *) mod_dav_fs: Remove inode keyed locking as this conflicts
>> > with atomically + creating files. This is a format cange of
>> > the DavLockDB. The old + DavLockDB must be deleted on
>> > upgrade. [Stefan Fritsch] +
>> > *) mod_log_config: Make ${cookie}C correctly match whole cookie
>> > names instead of substrings. PR 28037. [Dan Franklin <dan
>> > dan-franklin.com>, Stefan Fritsch]
>> >...
>>
>> Why did you go with a format change of the DAVLockDB? It is quite
>> possible that people will miss that step during an upgrade. You
>> could just leave DAV_TYPE_FNAME in there.
>
> That wouldn't help because it would still break with DAV_TYPE_INODE
> locks existing in the DAVLockDB. Or am I missing something?
Heh. Yeah. I realized that right *after* I hit the Send button :-P
Tho: mod_dav could error if it sees an unrecognized type, rather than
simply misinterpreting the data and silently unlocking all nodes.
Cheers,
-g
Re: svn commit: r834049 - in /httpd/httpd/trunk: CHANGES modules/dav/fs/lock.c modules/dav/fs/repos.c
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Monday 09 November 2009, Greg Stein wrote:
> On Mon, Nov 9, 2009 at 08:14, <sf...@apache.org> wrote:
> > Author: sf
> > Date: Mon Nov 9 13:14:07 2009
> > New Revision: 834049
> >
> > URL: http://svn.apache.org/viewvc?rev=834049&view=rev
> > Log:
> > Make PUT with DAV_MODE_WRITE_TRUNC create a temporary file first
> > and, when the transfer has been completed successfully, move it
> > over the old file.
> >
> > Since this would break inode keyed locking, switch to filename
> > keyed locking exclusively.
> >
> > PR: 39815
> > Submitted by: Paul Querna, Stefan Fritsch
> >
> > Modified:
> > httpd/httpd/trunk/CHANGES
> > httpd/httpd/trunk/modules/dav/fs/lock.c
> > httpd/httpd/trunk/modules/dav/fs/repos.c
> >
> > Modified: httpd/httpd/trunk/CHANGES
> > URL:
> > http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=834049
> >&r1=834048&r2=834049&view=diff
> > =================================================================
> >============= --- httpd/httpd/trunk/CHANGES [utf-8] (original)
> > +++ httpd/httpd/trunk/CHANGES [utf-8] Mon Nov 9 13:14:07 2009
> > @@ -10,6 +10,13 @@
> > mod_proxy_ftp: NULL pointer dereference on error paths.
> > [Stefan Fritsch <sf fritsch.de>, Joe Orton]
> >
> > + *) mod_dav_fs: Make PUT create files atomically and no longer
> > destroy the + old file if the transfer aborted. PR 39815.
> > [Paul Querna, Stefan Fritsch] +
> > + *) mod_dav_fs: Remove inode keyed locking as this conflicts
> > with atomically + creating files. This is a format cange of
> > the DavLockDB. The old + DavLockDB must be deleted on
> > upgrade. [Stefan Fritsch] +
> > *) mod_log_config: Make ${cookie}C correctly match whole cookie
> > names instead of substrings. PR 28037. [Dan Franklin <dan
> > dan-franklin.com>, Stefan Fritsch]
> >...
>
> Why did you go with a format change of the DAVLockDB? It is quite
> possible that people will miss that step during an upgrade. You
> could just leave DAV_TYPE_FNAME in there.
That wouldn't help because it would still break with DAV_TYPE_INODE
locks existing in the DAVLockDB. Or am I missing something?
Re: svn commit: r834049 - in /httpd/httpd/trunk: CHANGES
modules/dav/fs/lock.c modules/dav/fs/repos.c
Posted by Greg Stein <gs...@gmail.com>.
On Mon, Nov 9, 2009 at 08:14, <sf...@apache.org> wrote:
> Author: sf
> Date: Mon Nov 9 13:14:07 2009
> New Revision: 834049
>
> URL: http://svn.apache.org/viewvc?rev=834049&view=rev
> Log:
> Make PUT with DAV_MODE_WRITE_TRUNC create a temporary file first and, when the
> transfer has been completed successfully, move it over the old file.
>
> Since this would break inode keyed locking, switch to filename keyed locking
> exclusively.
>
> PR: 39815
> Submitted by: Paul Querna, Stefan Fritsch
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/modules/dav/fs/lock.c
> httpd/httpd/trunk/modules/dav/fs/repos.c
>
> Modified: httpd/httpd/trunk/CHANGES
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=834049&r1=834048&r2=834049&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
> +++ httpd/httpd/trunk/CHANGES [utf-8] Mon Nov 9 13:14:07 2009
> @@ -10,6 +10,13 @@
> mod_proxy_ftp: NULL pointer dereference on error paths.
> [Stefan Fritsch <sf fritsch.de>, Joe Orton]
>
> + *) mod_dav_fs: Make PUT create files atomically and no longer destroy the
> + old file if the transfer aborted. PR 39815. [Paul Querna, Stefan Fritsch]
> +
> + *) mod_dav_fs: Remove inode keyed locking as this conflicts with atomically
> + creating files. This is a format cange of the DavLockDB. The old
> + DavLockDB must be deleted on upgrade. [Stefan Fritsch]
> +
> *) mod_log_config: Make ${cookie}C correctly match whole cookie names
> instead of substrings. PR 28037. [Dan Franklin <dan dan-franklin.com>,
> Stefan Fritsch]
>...
Why did you go with a format change of the DAVLockDB? It is quite
possible that people will miss that step during an upgrade. You could
just leave DAV_TYPE_FNAME in there.
Cheers,
-g
Re: svn commit: r834049 - in /httpd/httpd/trunk: CHANGES modules/dav/fs/lock.c modules/dav/fs/repos.c
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Monday 09 November 2009, Ruediger Pluem wrote:
> This causes the following warning:
>
> repos.c: In function 'dav_fs_open_stream':
> repos.c:900: warning: passing argument 2 of 'apr_file_mktemp'
> discards qualifiers from pointer target type
>
Thanks. Fixed.
Re: svn commit: r834049 - in /httpd/httpd/trunk: CHANGES modules/dav/fs/lock.c
modules/dav/fs/repos.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 11/09/2009 02:14 PM, sf@apache.org wrote:
> Author: sf
> Date: Mon Nov 9 13:14:07 2009
> New Revision: 834049
>
> URL: http://svn.apache.org/viewvc?rev=834049&view=rev
> Log:
> Make PUT with DAV_MODE_WRITE_TRUNC create a temporary file first and, when the
> transfer has been completed successfully, move it over the old file.
>
> Since this would break inode keyed locking, switch to filename keyed locking
> exclusively.
>
> PR: 39815
> Submitted by: Paul Querna, Stefan Fritsch
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/modules/dav/fs/lock.c
> httpd/httpd/trunk/modules/dav/fs/repos.c
>
> Modified: httpd/httpd/trunk/modules/dav/fs/repos.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/dav/fs/repos.c?rev=834049&r1=834048&r2=834049&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/dav/fs/repos.c (original)
> +++ httpd/httpd/trunk/modules/dav/fs/repos.c Mon Nov 9 13:14:07 2009
dav_stream **stream)
> @@ -876,7 +890,19 @@
>
> ds->p = p;
> ds->pathname = resource->info->pathname;
> - rv = apr_file_open(&ds->f, ds->pathname, flags, APR_OS_DEFAULT, ds->p);
> + ds->temppath = NULL;
> +
> + if (mode == DAV_MODE_WRITE_TRUNC) {
> + ds->temppath = apr_pstrcat(p, ap_make_dirstr_parent(p, ds->pathname),
> + DAV_FS_TMP_PREFIX "XXXXXX", NULL);
> + rv = apr_file_mktemp(&ds->f, ds->temppath, flags, ds->p);
This causes the following warning:
repos.c: In function 'dav_fs_open_stream':
repos.c:900: warning: passing argument 2 of 'apr_file_mktemp' discards qualifiers from pointer target type
Regards
Rüdiger