You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by mi...@apache.org on 2020/06/27 23:41:00 UTC
svn commit: r1879285 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/core.xml include/ap_mmn.h include/http_core.h
include/http_protocol.h include/httpd.h modules/dav/fs/repos.c
modules/test/mod_dialup.c server/core.c server/util_etag.c
Author: minfrin
Date: Sat Jun 27 23:41:00 2020
New Revision: 1879285
URL: http://svn.apache.org/viewvc?rev=1879285&view=rev
Log:
"[mod_dav_fs etag handling] should really honor the FileETag setting".
- It now does.
- Add "Digest" to FileETag directive, allowing a strong ETag to be
generated using a file digest.
- Add ap_make_etag_ex() and ap_set_etag_fd() to allow full control over
ETag generation.
- Add concept of "binary notes" to request_rec, allowing packed bit flags
to be added to a request.
- First binary note - AP_REQUEST_STRONG_ETAG - allows modules to force
the ETag to a strong ETag to comply with RFC requirements, such as those
mandated by various WebDAV extensions.
Modified:
httpd/httpd/trunk/CHANGES
httpd/httpd/trunk/docs/manual/mod/core.xml
httpd/httpd/trunk/include/ap_mmn.h
httpd/httpd/trunk/include/http_core.h
httpd/httpd/trunk/include/http_protocol.h
httpd/httpd/trunk/include/httpd.h
httpd/httpd/trunk/modules/dav/fs/repos.c
httpd/httpd/trunk/modules/test/mod_dialup.c
httpd/httpd/trunk/server/core.c
httpd/httpd/trunk/server/util_etag.c
Modified: httpd/httpd/trunk/CHANGES
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=1879285&r1=1879284&r2=1879285&view=diff
==============================================================================
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Sat Jun 27 23:41:00 2020
@@ -1,6 +1,19 @@
-*- coding: utf-8 -*-
Changes with Apache 2.5.1
+ *) "[mod_dav_fs etag handling] should really honor the FileETag setting".
+ - It now does.
+ - Add "Digest" to FileETag directive, allowing a strong ETag to be
+ generated using a file digest.
+ - Add ap_make_etag_ex() and ap_set_etag_fd() to allow full control over
+ ETag generation.
+ - Add concept of "binary notes" to request_rec, allowing packed bit flags
+ to be added to a request.
+ - First binary note - AP_REQUEST_STRONG_ETAG - allows modules to force
+ the ETag to a strong ETag to comply with RFC requirements, such as those
+ mandated by various WebDAV extensions.
+ [Graham Leggett]
+
*) mod_ssl: Fix a race condition and possible crash when using a proxy client
certificate (SSLProxyMachineCertificateFile).
[Armin Abfalterer <a.abfalterer gmail.com>]
Modified: httpd/httpd/trunk/docs/manual/mod/core.xml
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/docs/manual/mod/core.xml?rev=1879285&r1=1879284&r2=1879285&view=diff
==============================================================================
--- httpd/httpd/trunk/docs/manual/mod/core.xml (original)
+++ httpd/httpd/trunk/docs/manual/mod/core.xml Sat Jun 27 23:41:00 2020
@@ -1926,15 +1926,18 @@ earlier.</compatibility>
<highlight language="config">
FileETag INode MTime Size
</highlight></dd>
+ <dt><strong>Digest</strong></dt>
+ <dd>If a document is file-based, the <code>ETag</code> field will be
+ calculated by taking the digest over the file.</dd>
<dt><strong>None</strong></dt>
<dd>If a document is file-based, no <code>ETag</code> field will be
included in the response</dd>
</dl>
- <p>The <code>INode</code>, <code>MTime</code>, and <code>Size</code>
- keywords may be prefixed with either <code>+</code> or <code>-</code>,
- which allow changes to be made to the default setting inherited
- from a broader scope. Any keyword appearing without such a prefix
+ <p>The <code>INode</code>, <code>MTime</code>, <code>Size</code> and
+ <code>Digest</code> keywords may be prefixed with either <code>+</code>
+ or <code>-</code>, which allow changes to be made to the default setting
+ inherited from a broader scope. Any keyword appearing without such a prefix
immediately and completely cancels the inherited setting.</p>
<p>If a directory's configuration includes
@@ -1943,18 +1946,10 @@ FileETag INode MTime Size
the setting for that subdirectory (which will be inherited by
any sub-subdirectories that don't override it) will be equivalent to
<code>FileETag MTime Size</code>.</p>
- <note type="warning"><title>Warning</title>
- Do not change the default for directories or locations that have WebDAV
- enabled and use <module>mod_dav_fs</module> as a storage provider.
- <module>mod_dav_fs</module> uses <code>MTime Size</code>
- as a fixed format for <code>ETag</code> comparisons on conditional requests.
- These conditional requests will break if the <code>ETag</code> format is
- changed via <directive>FileETag</directive>.
- </note>
<note><title>Server Side Includes</title>
An ETag is not generated for responses parsed by <module>mod_include</module>
- since the response entity can change without a change of the INode, MTime, or Size
- of the static file with embedded SSI directives.
+ since the response entity can change without a change of the INode, MTime,
+ Size or Digest of the static file with embedded SSI directives.
</note>
</usage>
Modified: httpd/httpd/trunk/include/ap_mmn.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/ap_mmn.h?rev=1879285&r1=1879284&r2=1879285&view=diff
==============================================================================
--- httpd/httpd/trunk/include/ap_mmn.h (original)
+++ httpd/httpd/trunk/include/ap_mmn.h Sat Jun 27 23:41:00 2020
@@ -636,6 +636,11 @@
* 20200420.5 (2.5.1-dev) Add pre_translate_name hook
* 20200420.6 (2.5.1-dev) Add map_encoded_one and map_encoded_all bits to
* proxy_server_conf
+ * 20200420.7 (2.5.1-dev) Add struct etag_rec, ap_make_etag_ex(),
+ * ap_set_etag_fd(). Add typedef ap_request_bnotes_t,
+ * macros AP_REQUEST_GET_BNOTE, AP_REQUEST_SET_BNOTE,
+ * AP_REQUEST_STRONG_ETAG, AP_REQUEST_IS_STRONG_ETAG.
+ * Add bnotes to request_rec.
*/
#define MODULE_MAGIC_COOKIE 0x41503235UL /* "AP25" */
@@ -643,7 +648,7 @@
#ifndef MODULE_MAGIC_NUMBER_MAJOR
#define MODULE_MAGIC_NUMBER_MAJOR 20200420
#endif
-#define MODULE_MAGIC_NUMBER_MINOR 6 /* 0...n */
+#define MODULE_MAGIC_NUMBER_MINOR 7 /* 0...n */
/**
* Determine if the server's current MODULE_MAGIC_NUMBER is at least a
Modified: httpd/httpd/trunk/include/http_core.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/http_core.h?rev=1879285&r1=1879284&r2=1879285&view=diff
==============================================================================
--- httpd/httpd/trunk/include/http_core.h (original)
+++ httpd/httpd/trunk/include/http_core.h Sat Jun 27 23:41:00 2020
@@ -489,12 +489,13 @@ typedef unsigned int overrides_t;
*/
typedef unsigned long etag_components_t;
-#define ETAG_UNSET 0
-#define ETAG_NONE (1 << 0)
-#define ETAG_MTIME (1 << 1)
-#define ETAG_INODE (1 << 2)
-#define ETAG_SIZE (1 << 3)
-#define ETAG_ALL (ETAG_MTIME | ETAG_INODE | ETAG_SIZE)
+#define ETAG_UNSET 0
+#define ETAG_NONE (1 << 0)
+#define ETAG_MTIME (1 << 1)
+#define ETAG_INODE (1 << 2)
+#define ETAG_SIZE (1 << 3)
+#define ETAG_DIGEST (1 << 4)
+#define ETAG_ALL (ETAG_MTIME | ETAG_INODE | ETAG_SIZE)
/* This is the default value used */
#define ETAG_BACKWARD (ETAG_MTIME | ETAG_SIZE)
Modified: httpd/httpd/trunk/include/http_protocol.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/http_protocol.h?rev=1879285&r1=1879284&r2=1879285&view=diff
==============================================================================
--- httpd/httpd/trunk/include/http_protocol.h (original)
+++ httpd/httpd/trunk/include/http_protocol.h Sat Jun 27 23:41:00 2020
@@ -166,6 +166,27 @@ AP_DECLARE(const char *) ap_make_content
*/
AP_DECLARE(void) ap_setup_make_content_type(apr_pool_t *pool);
+/** A structure with the ingredients for a file based etag */
+typedef struct etag_rec etag_rec;
+
+/**
+ * @brief A structure with the ingredients for a file based etag
+ */
+struct etag_rec {
+ /** Optional vary list validator */
+ const char *vlist_validator;
+ /** Time when the request started */
+ apr_time_t request_time;
+ /** finfo.protection (st_mode) set to zero if no such file */
+ apr_finfo_t *finfo;
+ /** File pathname used when generating a digest */
+ const char *pathname;
+ /** File descriptor used when generating a digest */
+ apr_file_t *fd;
+ /** Force a non-digest etag to be weak */
+ int force_weak;
+};
+
/**
* Construct an entity tag from the resource information. If it's a real
* file, build in some of the file characteristics.
@@ -177,12 +198,27 @@ AP_DECLARE(void) ap_setup_make_content_t
AP_DECLARE(char *) ap_make_etag(request_rec *r, int force_weak);
/**
+ * Construct an entity tag from information provided in the etag_rec
+ * structure.
+ * @param r The current request
+ * @param er The etag record, containing ingredients for the etag.
+ */
+AP_DECLARE(char *) ap_make_etag_ex(request_rec *r, etag_rec *er);
+
+/**
* Set the E-tag outgoing header
* @param r The current request
*/
AP_DECLARE(void) ap_set_etag(request_rec *r);
/**
+ * Set the E-tag outgoing header, with the option of forcing a strong ETag.
+ * @param r The current request
+ * @param fd The file descriptor
+ */
+AP_DECLARE(void) ap_set_etag_fd(request_rec *r, apr_file_t *fd);
+
+/**
* Set the last modified time for the file being sent
* @param r The current request
*/
@@ -762,7 +798,7 @@ AP_DECLARE_HOOK(const char *,http_scheme
AP_DECLARE_HOOK(apr_port_t,default_port,(const request_rec *r))
-#define AP_PROTOCOL_HTTP1 "http/1.1"
+#define AP_PROTOCOL_HTTP1 "http/1.1"
/**
* Determine the list of protocols available for a connection/request. This may
@@ -1011,6 +1047,7 @@ AP_DECLARE(void) ap_finalize_sub_req_pro
*/
AP_DECLARE(void) ap_send_interim_response(request_rec *r, int send_headers);
+
#ifdef __cplusplus
}
#endif
Modified: httpd/httpd/trunk/include/httpd.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/httpd.h?rev=1879285&r1=1879284&r2=1879285&view=diff
==============================================================================
--- httpd/httpd/trunk/include/httpd.h (original)
+++ httpd/httpd/trunk/include/httpd.h Sat Jun 27 23:41:00 2020
@@ -645,8 +645,6 @@ typedef apr_uint64_t ap_method_mask_t;
* The method mask bit to shift for anding with a bitmask.
*/
#define AP_METHOD_BIT ((ap_method_mask_t)1)
-/** @} */
-
/** @see ap_method_list_t */
typedef struct ap_method_list_t ap_method_list_t;
@@ -664,6 +662,49 @@ struct ap_method_list_t {
/** the array used for extension methods */
apr_array_header_t *method_list;
};
+/** @} */
+
+/**
+ * @defgroup bnotes Binary notes recognized by the server
+ * @ingroup APACHE_CORE_DAEMON
+ * @{
+ *
+ * @brief Binary notes recognized by the server.
+ */
+
+/**
+ * The type used for request binary notes.
+ */
+typedef apr_uint64_t ap_request_bnotes_t;
+
+/**
+ * These constants represent bitmasks for notes associated with this
+ * request. There are space for 64 bits in the apr_uint64_t.
+ *
+ */
+#define AP_REQUEST_STRONG_ETAG 1 >> 0
+
+/**
+ * This is a convenience macro to ease with getting specific request
+ * binary notes.
+ */
+#define AP_REQUEST_GET_BNOTE(r, mask) \
+ ((mask) & ((r)->bnotes))
+
+/**
+ * This is a convenience macro to ease with setting specific request
+ * binary notes.
+ */
+#define AP_REQUEST_SET_BNOTE(r, mask, val) \
+ (r)->bnotes = (((r)->bnotes & ~(mask)) | (val))
+
+/**
+ * Returns true if the strong etag flag is set for this request.
+ */
+#define AP_REQUEST_IS_STRONG_ETAG(r) \
+ AP_REQUEST_GET_BNOTE((r), AP_REQUEST_STRONG_ETAG)
+/** @} */
+
/**
* @defgroup module_magic Module Magic mime types
@@ -1097,6 +1138,11 @@ struct request_rec {
* TODO: compact elsewhere
*/
unsigned int flushed:1;
+ /** Request flags associated with this request. Use
+ * AP_REQUEST_GET_FLAGS() and AP_REQUEST_SET_FLAGS() to access
+ * the elements of this field.
+ */
+ ap_request_bnotes_t bnotes;
};
/**
Modified: httpd/httpd/trunk/modules/dav/fs/repos.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/dav/fs/repos.c?rev=1879285&r1=1879284&r2=1879285&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/dav/fs/repos.c (original)
+++ httpd/httpd/trunk/modules/dav/fs/repos.c Sat Jun 27 23:41:00 2020
@@ -1853,27 +1853,26 @@ static dav_error * dav_fs_walk(const dav
return dav_fs_internal_walk(params, depth, 0, NULL, response);
}
-/* dav_fs_etag: Stolen from ap_make_etag. Creates a strong etag
- * for file path.
- * ### do we need to return weak tags sometimes?
+/* dav_fs_etag: Creates an etag for the file path.
*/
static const char *dav_fs_getetag(const dav_resource *resource)
{
- dav_resource_private *ctx = resource->info;
- /* XXX: This should really honor the FileETag setting */
+ etag_rec er;
- if (!resource->exists)
- return apr_pstrdup(ctx->pool, "");
+ dav_resource_private *ctx = resource->info;
- if (ctx->finfo.filetype != APR_NOFILE) {
- return apr_psprintf(ctx->pool, "\"%" APR_UINT64_T_HEX_FMT "-%"
- APR_UINT64_T_HEX_FMT "\"",
- (apr_uint64_t) ctx->finfo.size,
- (apr_uint64_t) ctx->finfo.mtime);
+ if (!resource->exists) {
+ return "";
}
- return apr_psprintf(ctx->pool, "\"%" APR_UINT64_T_HEX_FMT "\"",
- (apr_uint64_t) ctx->finfo.mtime);
+ er.vlist_validator = NULL;
+ er.request_time = ctx->r->request_time;
+ er.finfo = &ctx->finfo;
+ er.pathname = ctx->pathname;
+ er.fd = NULL;
+ er.force_weak = 0;
+
+ return ap_make_etag_ex(ctx->r, &er);
}
static const dav_hooks_repository dav_hooks_repository_fs =
Modified: httpd/httpd/trunk/modules/test/mod_dialup.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/test/mod_dialup.c?rev=1879285&r1=1879284&r2=1879285&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/test/mod_dialup.c (original)
+++ httpd/httpd/trunk/modules/test/mod_dialup.c Sat Jun 27 23:41:00 2020
@@ -187,7 +187,7 @@ dialup_handler(request_rec *r)
/* copied from default handler: */
ap_update_mtime(r, r->finfo.mtime);
ap_set_last_modified(r);
- ap_set_etag(r);
+ ap_set_etag_fd(r, fd);
ap_set_accept_ranges(r);
ap_set_content_length(r, r->finfo.size);
Modified: httpd/httpd/trunk/server/core.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/core.c?rev=1879285&r1=1879284&r2=1879285&view=diff
==============================================================================
--- httpd/httpd/trunk/server/core.c (original)
+++ httpd/httpd/trunk/server/core.c Sat Jun 27 23:41:00 2020
@@ -2241,6 +2241,9 @@ static const char *set_etag_bits(cmd_par
else if (ap_cstr_casecmp(token, "INode") == 0) {
bit = ETAG_INODE;
}
+ else if (ap_cstr_casecmp(token, "Digest") == 0) {
+ bit = ETAG_DIGEST;
+ }
else {
return apr_pstrcat(cmd->pool, "Unknown keyword '",
token, "' for ", cmd->cmd->name,
@@ -5181,7 +5184,7 @@ static int default_handler(request_rec *
ap_update_mtime(r, r->finfo.mtime);
ap_set_last_modified(r);
- ap_set_etag(r);
+ ap_set_etag_fd(r, fd);
ap_set_accept_ranges(r);
ap_set_content_length(r, r->finfo.size);
if (bld_content_md5) {
Modified: httpd/httpd/trunk/server/util_etag.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util_etag.c?rev=1879285&r1=1879284&r2=1879285&view=diff
==============================================================================
--- httpd/httpd/trunk/server/util_etag.c (original)
+++ httpd/httpd/trunk/server/util_etag.c Sat Jun 27 23:41:00 2020
@@ -16,6 +16,8 @@
#include "apr_strings.h"
#include "apr_thread_proc.h" /* for RLIMIT stuff */
+#include "apr_sha1.h"
+#include "apr_base64.h"
#define APR_WANT_STRFUNC
#include "apr_want.h"
@@ -53,18 +55,42 @@ static char *etag_uint64_to_hex(char *ne
#define ETAG_WEAK "W/"
#define CHARS_PER_UINT64 (sizeof(apr_uint64_t) * 2)
+
+static void etag_start(char *etag, const char *weak, char **next)
+{
+ if (weak) {
+ while (*weak) {
+ *etag++ = *weak++;
+ }
+ }
+ *etag++ = '"';
+
+ *next = etag;
+}
+
+static void etag_end(char *next, const char *vlv, apr_size_t vlv_len)
+{
+ if (vlv) {
+ *next++ = ';';
+ apr_cpystrn(next, vlv, vlv_len);
+ }
+ else {
+ *next++ = '"';
+ *next = '\0';
+ }
+}
+
/*
* Construct an entity tag (ETag) from resource information. If it's a real
* file, build in some of the file characteristics. If the modification time
* is newer than (request-time minus 1 second), mark the ETag as weak - it
- * could be modified again in as short an interval. We rationalize the
- * modification time we're given to keep it from being in the future.
+ * could be modified again in as short an interval.
*/
-AP_DECLARE(char *) ap_make_etag(request_rec *r, int force_weak)
+AP_DECLARE(char *) ap_make_etag_ex(request_rec *r, etag_rec *er)
{
- char *weak;
- apr_size_t weak_len;
- char *etag;
+ char *weak = NULL;
+ apr_size_t weak_len = 0, vlv_len = 0;
+ char *etag, *vlv;
char *next;
core_dir_config *cfg;
etag_components_t etag_bits;
@@ -73,13 +99,96 @@ AP_DECLARE(char *) ap_make_etag(request_
cfg = (core_dir_config *)ap_get_core_module_config(r->per_dir_config);
etag_bits = (cfg->etag_bits & (~ cfg->etag_remove)) | cfg->etag_add;
+ if (er->force_weak) {
+ weak = ETAG_WEAK;
+ weak_len = sizeof(ETAG_WEAK);
+ }
+
+ if (r->vlist_validator) {
+
+ /* If we have a variant list validator (vlv) due to the
+ * response being negotiated, then we create a structured
+ * entity tag which merges the variant etag with the variant
+ * list validator (vlv). This merging makes revalidation
+ * somewhat safer, ensures that caches which can deal with
+ * Vary will (eventually) be updated if the set of variants is
+ * changed, and is also a protocol requirement for transparent
+ * content negotiation.
+ */
+
+ /* if the variant list validator is weak, we make the whole
+ * structured etag weak. If we would not, then clients could
+ * have problems merging range responses if we have different
+ * variants with the same non-globally-unique strong etag.
+ */
+
+ vlv = r->vlist_validator;
+ if (vlv[0] == 'W') {
+ vlv += 3;
+ weak = ETAG_WEAK;
+ weak_len = sizeof(ETAG_WEAK);
+ }
+ else {
+ vlv++;
+ }
+ vlv_len = strlen(vlv);
+
+ }
+ else {
+ vlv = NULL;
+ vlv_len = 0;
+ }
+
+ /*
+ * Did a module flag the need for a strong etag, or did the
+ * configuration tell us to generate a digest?
+ */
+ if (er->finfo->filetype == APR_REG &&
+ (AP_REQUEST_IS_STRONG_ETAG(r) || (etag_bits & ETAG_DIGEST))) {
+
+ apr_sha1_ctx_t context;
+ unsigned char buf[4096]; /* keep this a multiple of 64 */
+ unsigned char digest[APR_SHA1_DIGESTSIZE];
+ apr_file_t *fd = NULL;
+
+ apr_size_t nbytes;
+ apr_off_t offset = 0L;
+
+ if (er->fd) {
+ fd = er->fd;
+ }
+ else if (er->pathname) {
+ apr_file_open(&fd, er->pathname, APR_READ | APR_BINARY, 0, r->pool);
+ }
+ if (!fd) {
+ return "";
+ }
+
+ etag = apr_palloc(r->pool, weak_len + sizeof("\"\"") +
+ 4*(APR_SHA1_DIGESTSIZE/3) + vlv_len + 4);
+
+ apr_sha1_init(&context);
+ nbytes = sizeof(buf);
+ while (apr_file_read(fd, buf, &nbytes) == APR_SUCCESS) {
+ apr_sha1_update_binary(&context, buf, nbytes);
+ nbytes = sizeof(buf);
+ }
+ apr_file_seek(fd, APR_SET, &offset);
+ apr_sha1_final(digest, &context);
+
+ etag_start(etag, weak, &next);
+ next += apr_base64_encode_binary(next, digest, APR_SHA1_DIGESTSIZE) - 1;
+ etag_end(next, vlv, vlv_len);
+
+ return etag;
+ }
+
/*
* If it's a file (or we wouldn't be here) and no ETags
* should be set for files, return an empty string and
* note it for the header-sender to ignore.
*/
if (etag_bits & ETAG_NONE) {
- apr_table_setn(r->notes, "no-etag", "omit");
return "";
}
@@ -98,123 +207,117 @@ AP_DECLARE(char *) ap_make_etag(request_
* be modified again later in the second, and the validation
* would be incorrect.
*/
- if ((r->request_time - r->mtime > (1 * APR_USEC_PER_SEC)) &&
- !force_weak) {
- weak = NULL;
- weak_len = 0;
- }
- else {
+ if ((er->request_time - er->finfo->mtime < (1 * APR_USEC_PER_SEC))) {
weak = ETAG_WEAK;
weak_len = sizeof(ETAG_WEAK);
}
- if (r->finfo.filetype != APR_NOFILE) {
+ if (er->finfo->filetype != APR_NOFILE) {
/*
* ETag gets set to [W/]"inode-size-mtime", modulo any
* FileETag keywords.
*/
etag = apr_palloc(r->pool, weak_len + sizeof("\"--\"") +
- 3 * CHARS_PER_UINT64 + 1);
- next = etag;
- if (weak) {
- while (*weak) {
- *next++ = *weak++;
- }
- }
- *next++ = '"';
+ 3 * CHARS_PER_UINT64 + vlv_len + 2);
+
+ etag_start(etag, weak, &next);
+
bits_added = 0;
if (etag_bits & ETAG_INODE) {
- next = etag_uint64_to_hex(next, r->finfo.inode);
+ next = etag_uint64_to_hex(next, er->finfo->inode);
bits_added |= ETAG_INODE;
}
if (etag_bits & ETAG_SIZE) {
if (bits_added != 0) {
*next++ = '-';
}
- next = etag_uint64_to_hex(next, r->finfo.size);
+ next = etag_uint64_to_hex(next, er->finfo->size);
bits_added |= ETAG_SIZE;
}
if (etag_bits & ETAG_MTIME) {
if (bits_added != 0) {
*next++ = '-';
}
- next = etag_uint64_to_hex(next, r->mtime);
+ next = etag_uint64_to_hex(next, er->finfo->mtime);
}
- *next++ = '"';
- *next = '\0';
+
+ etag_end(next, vlv, vlv_len);
+
}
else {
/*
* Not a file document, so just use the mtime: [W/]"mtime"
*/
etag = apr_palloc(r->pool, weak_len + sizeof("\"\"") +
- CHARS_PER_UINT64 + 1);
- next = etag;
- if (weak) {
- while (*weak) {
- *next++ = *weak++;
- }
- }
- *next++ = '"';
- next = etag_uint64_to_hex(next, r->mtime);
- *next++ = '"';
- *next = '\0';
+ CHARS_PER_UINT64 + vlv_len + 2);
+
+ etag_start(etag, weak, &next);
+ next = etag_uint64_to_hex(next, er->finfo->mtime);
+ etag_end(next, vlv, vlv_len);
+
}
return etag;
}
+AP_DECLARE(char *) ap_make_etag(request_rec *r, int force_weak)
+{
+ etag_rec er;
+
+ er.vlist_validator = NULL;
+ er.request_time = r->request_time;
+ er.finfo = &r->finfo;
+ er.pathname = r->filename;
+ er.fd = NULL;
+ er.force_weak = force_weak;
+
+ return ap_make_etag_ex(r, &er);
+}
+
AP_DECLARE(void) ap_set_etag(request_rec *r)
{
char *etag;
- char *variant_etag, *vlv;
- int vlv_weak;
- if (!r->vlist_validator) {
- etag = ap_make_etag(r, 0);
+ etag_rec er;
- /* If we get a blank etag back, don't set the header. */
- if (!etag[0]) {
- return;
- }
+ er.vlist_validator = r->vlist_validator;
+ er.request_time = r->request_time;
+ er.finfo = &r->finfo;
+ er.pathname = r->filename;
+ er.fd = NULL;
+ er.force_weak = 0;
+
+ etag = ap_make_etag_ex(r, &er);
+
+ if (etag && etag[0]) {
+ apr_table_setn(r->headers_out, "ETag", etag);
}
else {
- /* If we have a variant list validator (vlv) due to the
- * response being negotiated, then we create a structured
- * entity tag which merges the variant etag with the variant
- * list validator (vlv). This merging makes revalidation
- * somewhat safer, ensures that caches which can deal with
- * Vary will (eventually) be updated if the set of variants is
- * changed, and is also a protocol requirement for transparent
- * content negotiation.
- */
+ apr_table_setn(r->notes, "no-etag", "omit");
+ }
- /* if the variant list validator is weak, we make the whole
- * structured etag weak. If we would not, then clients could
- * have problems merging range responses if we have different
- * variants with the same non-globally-unique strong etag.
- */
+}
- vlv = r->vlist_validator;
- vlv_weak = (vlv[0] == 'W');
+AP_DECLARE(void) ap_set_etag_fd(request_rec *r, apr_file_t *fd)
+{
+ char *etag;
- variant_etag = ap_make_etag(r, vlv_weak);
+ etag_rec er;
- /* If we get a blank etag back, don't append vlv and stop now. */
- if (!variant_etag[0]) {
- return;
- }
+ er.vlist_validator = r->vlist_validator;
+ er.request_time = r->request_time;
+ er.finfo = &r->finfo;
+ er.pathname = NULL;
+ er.fd = fd;
+ er.force_weak = 0;
- /* merge variant_etag and vlv into a structured etag */
- variant_etag[strlen(variant_etag) - 1] = '\0';
- if (vlv_weak) {
- vlv += 3;
- }
- else {
- vlv++;
- }
- etag = apr_pstrcat(r->pool, variant_etag, ";", vlv, NULL);
+ etag = ap_make_etag_ex(r, &er);
+
+ if (etag && etag[0]) {
+ apr_table_setn(r->headers_out, "ETag", etag);
+ }
+ else {
+ apr_table_setn(r->notes, "no-etag", "omit");
}
- apr_table_setn(r->headers_out, "ETag", etag);
}
Re: svn commit: r1879285 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/core.xml include/ap_mmn.h include/http_core.h
include/http_protocol.h include/httpd.h modules/dav/fs/repos.c
modules/test/mod_dialup.c server/core.c server/util_etag.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
> On Jun 29, 2020, at 10:27 AM, Graham Leggett <mi...@sharp.fm> wrote:
>
> On 29 Jun 2020, at 14:49, Yann Ylavic <ylavic.dev@gmail.com <ma...@gmail.com>> wrote:
>
>>> Yes we can and should (but in separate commits).
>>>
>>> I have my eye on the r->proxyreq flag, we can pack this into the binary notes too, values don’t need to be one bit wide.
>>
>> Actually I was thinking the other way around, have the new "unsigned
>> int strong_etag:1" bitfield rather than changing the existing ones...
>> Why adding complexity with bit(s) macros while bitfields exist?
>
> The problem with bitfields in the public APIs is that they’re not binary compatible across compilers. While it is very rare that a module will be built with a different compiler than httpd was, it’s still theoretically possible, and we should probably avoid it. Bitfields aren’t a problem for in-module or in-core code.
++1
Re: svn commit: r1879285 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/core.xml include/ap_mmn.h include/http_core.h
include/http_protocol.h include/httpd.h modules/dav/fs/repos.c
modules/test/mod_dialup.c server/core.c server/util_etag.c
Posted by Graham Leggett <mi...@sharp.fm>.
On 29 Jun 2020, at 14:49, Yann Ylavic <yl...@gmail.com> wrote:
>> Yes we can and should (but in separate commits).
>>
>> I have my eye on the r->proxyreq flag, we can pack this into the binary notes too, values don’t need to be one bit wide.
>
> Actually I was thinking the other way around, have the new "unsigned
> int strong_etag:1" bitfield rather than changing the existing ones...
> Why adding complexity with bit(s) macros while bitfields exist?
The problem with bitfields in the public APIs is that they’re not binary compatible across compilers. While it is very rare that a module will be built with a different compiler than httpd was, it’s still theoretically possible, and we should probably avoid it. Bitfields aren’t a problem for in-module or in-core code.
Regards,
Graham
—
Re: svn commit: r1879285 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/core.xml include/ap_mmn.h include/http_core.h
include/http_protocol.h include/httpd.h modules/dav/fs/repos.c
modules/test/mod_dialup.c server/core.c server/util_etag.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Jun 29, 2020 at 1:59 PM Graham Leggett <mi...@sharp.fm> wrote:
>
> On 29 Jun 2020, at 13:08, Yann Ylavic <yl...@gmail.com> wrote:
>
> >> /**
> >> * @defgroup module_magic Module Magic mime types
> >> @@ -1097,6 +1138,11 @@ struct request_rec {
> >> * TODO: compact elsewhere
> >> */
> >> unsigned int flushed:1;
> >> + /** Request flags associated with this request. Use
> >> + * AP_REQUEST_GET_FLAGS() and AP_REQUEST_SET_FLAGS() to access
> >> + * the elements of this field.
> >> + */
> >> + ap_request_bnotes_t bnotes;
> >> };
> >
> > Can't we use a single bitfield (like "flushed" above) for the single
> > AP_REQUEST_STRONG_ETAG flag?
>
> Yes we can and should (but in separate commits).
>
> I have my eye on the r->proxyreq flag, we can pack this into the binary notes too, values don’t need to be one bit wide.
Actually I was thinking the other way around, have the new "unsigned
int strong_etag:1" bitfield rather than changing the existing ones...
Why adding complexity with bit(s) macros while bitfields exist?
Regards;
Yann.
Re: svn commit: r1879285 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/core.xml include/ap_mmn.h include/http_core.h
include/http_protocol.h include/httpd.h modules/dav/fs/repos.c
modules/test/mod_dialup.c server/core.c server/util_etag.c
Posted by Graham Leggett <mi...@sharp.fm>.
On 29 Jun 2020, at 13:08, Yann Ylavic <yl...@gmail.com> wrote:
>> /**
>> * @defgroup module_magic Module Magic mime types
>> @@ -1097,6 +1138,11 @@ struct request_rec {
>> * TODO: compact elsewhere
>> */
>> unsigned int flushed:1;
>> + /** Request flags associated with this request. Use
>> + * AP_REQUEST_GET_FLAGS() and AP_REQUEST_SET_FLAGS() to access
>> + * the elements of this field.
>> + */
>> + ap_request_bnotes_t bnotes;
>> };
>
> Can't we use a single bitfield (like "flushed" above) for the single
> AP_REQUEST_STRONG_ETAG flag?
Yes we can and should (but in separate commits).
I have my eye on the r->proxyreq flag, we can pack this into the binary notes too, values don’t need to be one bit wide.
Regards,
Graham
—
Re: svn commit: r1879285 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/core.xml include/ap_mmn.h include/http_core.h
include/http_protocol.h include/httpd.h modules/dav/fs/repos.c
modules/test/mod_dialup.c server/core.c server/util_etag.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Sun, Jun 28, 2020 at 1:41 AM <mi...@apache.org> wrote:
>
> Author: minfrin
> Date: Sat Jun 27 23:41:00 2020
> New Revision: 1879285
>
> URL: http://svn.apache.org/viewvc?rev=1879285&view=rev
[]
> --- httpd/httpd/trunk/include/httpd.h (original)
> +++ httpd/httpd/trunk/include/httpd.h Sat Jun 27 23:41:00 2020
[]
> +/**
> + * @defgroup bnotes Binary notes recognized by the server
> + * @ingroup APACHE_CORE_DAEMON
> + * @{
> + *
> + * @brief Binary notes recognized by the server.
> + */
> +
> +/**
> + * The type used for request binary notes.
> + */
> +typedef apr_uint64_t ap_request_bnotes_t;
> +
> +/**
> + * These constants represent bitmasks for notes associated with this
> + * request. There are space for 64 bits in the apr_uint64_t.
> + *
> + */
> +#define AP_REQUEST_STRONG_ETAG 1 >> 0
> +
> +/**
> + * This is a convenience macro to ease with getting specific request
> + * binary notes.
> + */
> +#define AP_REQUEST_GET_BNOTE(r, mask) \
> + ((mask) & ((r)->bnotes))
> +
> +/**
> + * This is a convenience macro to ease with setting specific request
> + * binary notes.
> + */
> +#define AP_REQUEST_SET_BNOTE(r, mask, val) \
> + (r)->bnotes = (((r)->bnotes & ~(mask)) | (val))
> +
> +/**
> + * Returns true if the strong etag flag is set for this request.
> + */
> +#define AP_REQUEST_IS_STRONG_ETAG(r) \
> + AP_REQUEST_GET_BNOTE((r), AP_REQUEST_STRONG_ETAG)
> +/** @} */
> +
>
> /**
> * @defgroup module_magic Module Magic mime types
> @@ -1097,6 +1138,11 @@ struct request_rec {
> * TODO: compact elsewhere
> */
> unsigned int flushed:1;
> + /** Request flags associated with this request. Use
> + * AP_REQUEST_GET_FLAGS() and AP_REQUEST_SET_FLAGS() to access
> + * the elements of this field.
> + */
> + ap_request_bnotes_t bnotes;
> };
Can't we use a single bitfield (like "flushed" above) for the single
AP_REQUEST_STRONG_ETAG flag?
Regards;
Yann.
Re: svn commit: r1879285 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/core.xml include/ap_mmn.h include/http_core.h
include/http_protocol.h include/httpd.h modules/dav/fs/repos.c
modules/test/mod_dialup.c server/core.c server/util_etag.c
Posted by Graham Leggett <mi...@sharp.fm>.
On 03 Jul 2020, at 11:19, Ruediger Pluem <rp...@apache.org> wrote:
> Thanks for the pointer. Is Content-MD5 really used? And given that it has been removed in the RFC my approach would be as follows:
>
> 1. Continue with your new additions as is. Do not try to merge any of this code with Content-MD5 related content.
> 2. Backport them.
> 3. Leave Content-MD5 untouched in 2.4.x.
> 4. Remove Content-MD5 in trunk.
Thought about it some more and was going to suggest the above instead, +1.
Regards,
Graham
—
Re: svn commit: r1879285 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/core.xml include/ap_mmn.h include/http_core.h
include/http_protocol.h include/httpd.h modules/dav/fs/repos.c
modules/test/mod_dialup.c server/core.c server/util_etag.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 7/3/20 10:54 AM, Graham Leggett wrote:
> On 29 Jun 2020, at 16:37, Ruediger Pluem <rp...@apache.org> wrote:
>
>> Makes sense.
>> Do you see a possibility to merge this code and the one of ap_md5digest to a more generic procedure that
>> allows to choose the digest algorithm while using 'MMAPED' reads?
>> BTW: Is sha1 mandatory for strong etags? If not wouldn't MD5 be enough and if MD5 is seen as too insecure
>> why isn't sha1?
>
> I chose sha1 as it was a) widely available in APR and b) better than md5, but that was it.
>
> I am wondering if for 2.4 if we use md5 instead, and then set Content-MD5 at the same time in the same code instead of calculating the md5 twice.
>
> Then - as per the removal of Content-MD5 from https://tools.ietf.org/html/rfc7231#appendix-B - we separately switch it to sha1 and remove Content-MD5 in trunk.
Thanks for the pointer. Is Content-MD5 really used? And given that it has been removed in the RFC my approach would be as follows:
1. Continue with your new additions as is. Do not try to merge any of this code with Content-MD5 related content.
2. Backport them.
3. Leave Content-MD5 untouched in 2.4.x.
4. Remove Content-MD5 in trunk.
Regards
Rüdiger
Re: svn commit: r1879285 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/core.xml include/ap_mmn.h include/http_core.h
include/http_protocol.h include/httpd.h modules/dav/fs/repos.c
modules/test/mod_dialup.c server/core.c server/util_etag.c
Posted by Graham Leggett <mi...@sharp.fm>.
On 29 Jun 2020, at 16:37, Ruediger Pluem <rp...@apache.org> wrote:
> Makes sense.
> Do you see a possibility to merge this code and the one of ap_md5digest to a more generic procedure that
> allows to choose the digest algorithm while using 'MMAPED' reads?
> BTW: Is sha1 mandatory for strong etags? If not wouldn't MD5 be enough and if MD5 is seen as too insecure
> why isn't sha1?
I chose sha1 as it was a) widely available in APR and b) better than md5, but that was it.
I am wondering if for 2.4 if we use md5 instead, and then set Content-MD5 at the same time in the same code instead of calculating the md5 twice.
Then - as per the removal of Content-MD5 from https://tools.ietf.org/html/rfc7231#appendix-B - we separately switch it to sha1 and remove Content-MD5 in trunk.
Is that sane?
Regards,
Graham
—
Re: svn commit: r1879285 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/core.xml include/ap_mmn.h include/http_core.h
include/http_protocol.h include/httpd.h modules/dav/fs/repos.c
modules/test/mod_dialup.c server/core.c server/util_etag.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 6/29/20 4:08 PM, Graham Leggett wrote:
> On 29 Jun 2020, at 11:41, Ruediger Pluem <rpluem@apache.org <ma...@apache.org>> wrote:
>
>>> + etag = apr_palloc(r->pool, weak_len + sizeof("\"\"") +
>>> + 4*(APR_SHA1_DIGESTSIZE/3) + vlv_len + 4);
>>
>> Using apr_base64_encode_len in the formula above would make it easier to understand and read IMHO.
>
> It would also mean it could no longer be optimised to a constant. What side do we want to fall on?
Fair enough. The above is faster. Nevertheless I would prefer to group that better. Probably a separate define
SHA1_DIGEST_BASE64_LEN that only defines the base64 length of the base64 encode SHA1 digest. This would mean
more calc work for the compiler and not during runtime, but I guess this is acceptable.
>
>>> +
>>> + apr_sha1_init(&context);
>>> + nbytes = sizeof(buf);
>>> + while (apr_file_read(fd, buf, &nbytes) == APR_SUCCESS) {
>>
>> I assume that the code has been taken from ap_md5digest, but
>> if we have MMAP available and if we have not disabled MMAP we use MMAP to read the contents of the file
>> if sendfile is not possible (e.g. SSL connections).
>> Wouldn't using MMAP more performant here especially in case of larger files?
>
> It makes sense, but I would want to change something like this separately.
Makes sense.
Do you see a possibility to merge this code and the one of ap_md5digest to a more generic procedure that
allows to choose the digest algorithm while using 'MMAPED' reads?
BTW: Is sha1 mandatory for strong etags? If not wouldn't MD5 be enough and if MD5 is seen as too insecure
why isn't sha1?
>>> + apr_sha1_update_binary(&context, buf, nbytes);
>>> + nbytes = sizeof(buf);
>>> + }
>>> + apr_file_seek(fd, APR_SET, &offset);
>>
>> Why do we always reset the file pointer to 0? Why don't we get the actual file pointer of fd before we do the reading
>> and restore it afterwards?
>
> I am guessing that the why is lost in the mists of time. As a guess, it avoids an additional call.
>
> Using the actual file pointer is cleaner, as there is a guarantee that the function does not have any side effects.
>
> http://svn.apache.org/viewvc?rev=1879332&view=rev
Thanks. Another option that just came to mind would it be also fine in case that we get an fd passed to do
an apr_file_dup() on that descriptor such that we also do not touch the actual buffering (if enabled) of the used fd?
Furthermore don't we need to do a seek to 0 before we start reading? Who tells us that the filepointer is at 0 for a passed fd?
Regards
Rüdiger
Re: svn commit: r1879285 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/core.xml include/ap_mmn.h include/http_core.h
include/http_protocol.h include/httpd.h modules/dav/fs/repos.c
modules/test/mod_dialup.c server/core.c server/util_etag.c
Posted by Graham Leggett <mi...@sharp.fm>.
On 29 Jun 2020, at 11:41, Ruediger Pluem <rp...@apache.org> wrote:
>> + etag = apr_palloc(r->pool, weak_len + sizeof("\"\"") +
>> + 4*(APR_SHA1_DIGESTSIZE/3) + vlv_len + 4);
>
> Using apr_base64_encode_len in the formula above would make it easier to understand and read IMHO.
It would also mean it could no longer be optimised to a constant. What side do we want to fall on?
>> +
>> + apr_sha1_init(&context);
>> + nbytes = sizeof(buf);
>> + while (apr_file_read(fd, buf, &nbytes) == APR_SUCCESS) {
>
> I assume that the code has been taken from ap_md5digest, but
> if we have MMAP available and if we have not disabled MMAP we use MMAP to read the contents of the file
> if sendfile is not possible (e.g. SSL connections).
> Wouldn't using MMAP more performant here especially in case of larger files?
It makes sense, but I would want to change something like this separately.
>> + apr_sha1_update_binary(&context, buf, nbytes);
>> + nbytes = sizeof(buf);
>> + }
>> + apr_file_seek(fd, APR_SET, &offset);
>
> Why do we always reset the file pointer to 0? Why don't we get the actual file pointer of fd before we do the reading
> and restore it afterwards?
I am guessing that the why is lost in the mists of time. As a guess, it avoids an additional call.
Using the actual file pointer is cleaner, as there is a guarantee that the function does not have any side effects.
http://svn.apache.org/viewvc?rev=1879332&view=rev <http://svn.apache.org/viewvc?rev=1879332&view=rev>
Regards,
Graham
—
Re: svn commit: r1879285 - in /httpd/httpd/trunk: CHANGES
docs/manual/mod/core.xml include/ap_mmn.h include/http_core.h
include/http_protocol.h include/httpd.h modules/dav/fs/repos.c
modules/test/mod_dialup.c server/core.c server/util_etag.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 6/28/20 1:41 AM, minfrin@apache.org wrote:
> Author: minfrin
> Date: Sat Jun 27 23:41:00 2020
> New Revision: 1879285
>
> URL: http://svn.apache.org/viewvc?rev=1879285&view=rev
> Log:
> "[mod_dav_fs etag handling] should really honor the FileETag setting".
> - It now does.
> - Add "Digest" to FileETag directive, allowing a strong ETag to be
> generated using a file digest.
> - Add ap_make_etag_ex() and ap_set_etag_fd() to allow full control over
> ETag generation.
> - Add concept of "binary notes" to request_rec, allowing packed bit flags
> to be added to a request.
> - First binary note - AP_REQUEST_STRONG_ETAG - allows modules to force
> the ETag to a strong ETag to comply with RFC requirements, such as those
> mandated by various WebDAV extensions.
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/docs/manual/mod/core.xml
> httpd/httpd/trunk/include/ap_mmn.h
> httpd/httpd/trunk/include/http_core.h
> httpd/httpd/trunk/include/http_protocol.h
> httpd/httpd/trunk/include/httpd.h
> httpd/httpd/trunk/modules/dav/fs/repos.c
> httpd/httpd/trunk/modules/test/mod_dialup.c
> httpd/httpd/trunk/server/core.c
> httpd/httpd/trunk/server/util_etag.c
>
> Modified: httpd/httpd/trunk/server/util_etag.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util_etag.c?rev=1879285&r1=1879284&r2=1879285&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/util_etag.c (original)
> +++ httpd/httpd/trunk/server/util_etag.c Sat Jun 27 23:41:00 2020
> @@ -73,13 +99,96 @@ AP_DECLARE(char *) ap_make_etag(request_
> cfg = (core_dir_config *)ap_get_core_module_config(r->per_dir_config);
> etag_bits = (cfg->etag_bits & (~ cfg->etag_remove)) | cfg->etag_add;
>
> + if (er->force_weak) {
> + weak = ETAG_WEAK;
> + weak_len = sizeof(ETAG_WEAK);
> + }
> +
> + if (r->vlist_validator) {
> +
> + /* If we have a variant list validator (vlv) due to the
> + * response being negotiated, then we create a structured
> + * entity tag which merges the variant etag with the variant
> + * list validator (vlv). This merging makes revalidation
> + * somewhat safer, ensures that caches which can deal with
> + * Vary will (eventually) be updated if the set of variants is
> + * changed, and is also a protocol requirement for transparent
> + * content negotiation.
> + */
> +
> + /* if the variant list validator is weak, we make the whole
> + * structured etag weak. If we would not, then clients could
> + * have problems merging range responses if we have different
> + * variants with the same non-globally-unique strong etag.
> + */
> +
> + vlv = r->vlist_validator;
> + if (vlv[0] == 'W') {
> + vlv += 3;
> + weak = ETAG_WEAK;
> + weak_len = sizeof(ETAG_WEAK);
> + }
> + else {
> + vlv++;
> + }
> + vlv_len = strlen(vlv);
> +
> + }
> + else {
> + vlv = NULL;
> + vlv_len = 0;
> + }
> +
> + /*
> + * Did a module flag the need for a strong etag, or did the
> + * configuration tell us to generate a digest?
> + */
> + if (er->finfo->filetype == APR_REG &&
> + (AP_REQUEST_IS_STRONG_ETAG(r) || (etag_bits & ETAG_DIGEST))) {
> +
> + apr_sha1_ctx_t context;
> + unsigned char buf[4096]; /* keep this a multiple of 64 */
> + unsigned char digest[APR_SHA1_DIGESTSIZE];
> + apr_file_t *fd = NULL;
> +
> + apr_size_t nbytes;
> + apr_off_t offset = 0L;
> +
> + if (er->fd) {
> + fd = er->fd;
> + }
> + else if (er->pathname) {
> + apr_file_open(&fd, er->pathname, APR_READ | APR_BINARY, 0, r->pool);
> + }
> + if (!fd) {
> + return "";
> + }
> +
> + etag = apr_palloc(r->pool, weak_len + sizeof("\"\"") +
> + 4*(APR_SHA1_DIGESTSIZE/3) + vlv_len + 4);
Using apr_base64_encode_len in the formula above would make it easier to understand and read IMHO.
> +
> + apr_sha1_init(&context);
> + nbytes = sizeof(buf);
> + while (apr_file_read(fd, buf, &nbytes) == APR_SUCCESS) {
I assume that the code has been taken from ap_md5digest, but
if we have MMAP available and if we have not disabled MMAP we use MMAP to read the contents of the file
if sendfile is not possible (e.g. SSL connections).
Wouldn't using MMAP more performant here especially in case of larger files?
> + apr_sha1_update_binary(&context, buf, nbytes);
> + nbytes = sizeof(buf);
> + }
> + apr_file_seek(fd, APR_SET, &offset);
Why do we always reset the file pointer to 0? Why don't we get the actual file pointer of fd before we do the reading
and restore it afterwards?
> + apr_sha1_final(digest, &context> +
> + etag_start(etag, weak, &next);
> + next += apr_base64_encode_binary(next, digest, APR_SHA1_DIGESTSIZE) - 1;
> + etag_end(next, vlv, vlv_len);
> +
> + return etag;
> + }
> +
> /*
> * If it's a file (or we wouldn't be here) and no ETags
> * should be set for files, return an empty string and
> * note it for the header-sender to ignore.
> */
> if (etag_bits & ETAG_NONE) {
> - apr_table_setn(r->notes, "no-etag", "omit");
> return "";
> }
>
Regards
Rüdiger