Re: svn commit: r834049 - in /httpd/httpd/trunk: CHANGES modules/dav/fs/lock.c modules/dav/fs/repos.c

[Greg Stein-4]

On Mon, Nov 9, 2009 at 08:14,  <sf@...> 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

[Stefan Fritsch]

On Monday 09 November 2009, Greg Stein wrote:

> On Mon, Nov 9, 2009 at 08:14,  <sf@...> 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?

[Ruediger Pluem]

On 11/09/2009 02:14 PM, sf@... 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

[Stefan Fritsch]

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.

[Greg Stein-4]

On Mon, Nov 9, 2009 at 08:42, Stefan Fritsch <sf@...> wrote:

> On Monday 09 November 2009, Greg Stein wrote:
>> On Mon, Nov 9, 2009 at 08:14,  <sf@...> 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

[Stefan Fritsch]

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

[Greg Stein-4]

On Mon, Nov 9, 2009 at 14:46, Stefan Fritsch <sf@...> 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

[Greg Stein-4]

On Mon, Nov 9, 2009 at 15:21, Greg Stein <gstein@...> wrote:

> On Mon, Nov 9, 2009 at 14:46, Stefan Fritsch <sf@...> 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
>

[Stefan Fritsch]

On Monday 09 November 2009, Greg Stein wrote:

> On Mon, Nov 9, 2009 at 14:46, Stefan Fritsch <sf@...> 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."