mod_dav: optionally set mtime against x-oc-mtime header#556
mod_dav: optionally set mtime against x-oc-mtime header#556leo9800 wants to merge 19 commits intoapache:trunkfrom
Conversation
notroj
left a comment
notroj
left a comment
There was a problem hiding this comment.
This is an interesting feature. Are you aware of any standardisation effort for that header?
modules/dav/fs/repos.c
Outdated
| status = apr_file_mtime_set(resource->info->pathname, mtime, pool); | ||
|
|
||
| if (status != APR_SUCCESS) { | ||
| return dav_new_error(pool, HTTP_BAD_REQUEST, 0, status, "Could not set mtime."); |
There was a problem hiding this comment.
If we get this far this is likely not a client error so this should be mapped to something appropriate rather than a 400. Or default to 500?
There was a problem hiding this comment.
actually i was a bit unsure whether 4xx or 5xx should be returned in such case. despite most invalid patterns has already being sanitized in dav_parse_mtime, including:
- non-digits, e.g.
nonsense bytes - negatives, e.g.
-123123123 - non-decimal, e.g.
0xa0b1 - empty strings
- overflowing
while apr_file_mtime_set may still fail, in most cases it is clear that a 500 should be returned, e.g. kernel VFS failed to respond utime syscall. but i was wondering if there is any possibility that an invalid user input fails apr_file_mtime_set where 400 should be returned.
as of now I would return a 500 in such cases instead of 400.
seems there is not much such efforts (i.e. RFC drafts) from what i know. it is an approach OwnCloud (predecessor of NextCloud) adopts to enable this feature is also implemented by rclone's given that this is a non-standard, i would implement it but keep it disabled by default to avoid any non-standard breaking change. users who prefer to use this feature should have it opted in manually with |
notroj
left a comment
notroj
left a comment
There was a problem hiding this comment.
Thanks for the update, looking good. One minor style nit:
} else {
should be written as
}
else {
| } | ||
|
|
||
| /** | ||
| * @return 1 if valid x-oc-mtime, |
There was a problem hiding this comment.
I would prefer this with an apr_status_t return value, though it's a minor question of style
There was a problem hiding this comment.
i am not sure if apr_status_t is proper for this case personally.
since 0 is defined as APR_SUCCESS, while for the returning value of this function both 1 and 0 are considered success, and -1 stands for a failure.
actually i was imitating static int dav_parse_range(request_rec *r, apr_off_t *range_start, apr_off_t *range_end) in mod_dav.c
ref: apache#556 (review) Signed-off-by: Leo <i@hardrain980.com>
|
i am still a bit confused on how should there are 3 possible handlings:
the comment
suggests approach 3, but by invoking |
|
(3). For DEBUG-level or TRACE-level , omit the |
|
@notroj gotcha |
|
should i also rebase this patch to current i noticed a merge conflict related to log-number, ( |
|
Don't worry about APLOGNO() conflicts, leave that to a committer to fix up. I did a CI run - one issue here: https://github.com/notroj/httpd/actions/runs/18156145629/job/51676367710 |
|
@notroj gotcha seems we need to ensure conformity with C89/C90 |
|
Some more failures now https://github.com/notroj/httpd/actions/runs/18197214275/job/51806547259#logs and probably best to rebase onto the trunk again because the trunk tests are failing without 60ad836 |
ref: apache#556 (review) Signed-off-by: Leo <i@hardrain980.com>
…av_hooks_repository` modules/dav/fs/repos.c: implement `dav_fs_set_mtime` Signed-off-by: Leo <i@hardrain980.com>
Signed-off-by: Leo <i@hardrain980.com>
Signed-off-by: Leo <i@hardrain980.com>
Signed-off-by: Leo <i@hardrain980.com>
…mtime when `conf->honor_mtime_header == DAV_ENABLED_ON`,
check `set_mtime` callback is available before invoking, log debug-level messages
docs/log-message-tags: bumped with `docs/log-message-tags/update-log-msg-tags`
Signed-off-by: Leo <i@hardrain980.com>
…e_set` fails, emit error-level log message also docs/log-message-tags: bumped with `docs/log-message-tags/update-log-msg-tags` Signed-off-by: Leo <i@hardrain980.com>
Signed-off-by: Leo <i@hardrain980.com>
…y beginning of `dav_method_put()`, this avoids access of uninitialized structure Signed-off-by: Leo <i@hardrain980.com>
Signed-off-by: Leo <i@hardrain980.com>
ref: apache#556 (review) Signed-off-by: Leo <i@hardrain980.com>
…invocation is not required Signed-off-by: Leo <i@hardrain980.com>
…ssary terminologies Signed-off-by: Leo <i@hardrain980.com>
Signed-off-by: Leo <i@hardrain980.com>
…for debug and trace level logs Signed-off-by: Leo <i@hardrain980.com>
…-msg-tags` invocation Signed-off-by: Leo <i@hardrain980.com>
…cation times for directory (MKCOL) Signed-off-by: Leo <i@hardrain980.com>
Signed-off-by: Leo <i@hardrain980.com>
- remove unnecessary variable declarations: `mtime_aware`, `err3` - always initialize `mtime_ret` to `0` - simplify mtime-setting procedures Signed-off-by: Leo <i@hardrain980.com>
…OGNO()` after rebase Signed-off-by: Leo <i@hardrain980.com>
|
https://github.com/notroj/httpd/actions/runs/18216313264 🎉 I'll try to get it merged this week, thanks for your persistence! |
|
Sorry took me a bit longer as usual. @leo9800 if you want a full name in CHANGES for this please suggest one 😉 -otherwise your credit will be as seen here: 2d92bae#diff-732cc45bdcc31912401891f55bf0fb993b939d00bc92ad62f953f6c633c2fffaR1 |
|
@notroj gotcha i would prefer Leo LI as my fullname if it is possible to change it anyway 😉 |
2 participants
WebDAV RFCs did not specify a standard approach to set modification time for files or directory on the server, this feature enables better presevation of file metadata when uploading.
as a compensation, OwnCloud (now known as NextCloud) proposed a
X-OC-Mtimeheader which contains UNIX timestamp of mtime with file-modifying methods (e.g.PUT) which implements the feature above as a extension to the standard.this patch allows apache to optionally honor the
X-OC-Mtimeheader and set mtime accordingly ifDavHonorMtimeHeader Onis set.