Allow to track multiple uploads in one request

[BenBE]

As mentioned in #30, this PR supersedes said PR and skips the UDP notification feature; thus keeping only the changes needed for tracking of multiple uploads in one request. This PR basically ports the changes from @devgs onto v0.9.3.

Additionally there's a huge commit that only tries to do code formatting for consistency across the codebase. The style used is based on the code style in htop with some minor adaptations for better fit with the existing code.

Notable changes in the code style include:

• Avoiding hanging indentation for line continuation
• Soft-dropping a line length limit (still trying to keep below 80 chars though)
• Opening braces for blocks on same line as the block statement
• Space after keywords (if( condition ) … -> if (condition) { …)
• Complex conditions have been broken onto multiple lines with the logical operator at the end
• Avoiding if-statements without explicit blocks (also single-line statements have been broken to multiple lines)

What I (mostly) didn't do in this first run is:

• Alignment of pointer types
• Scope reduction of variables
• Header sorting and IWYU

What's (still) missing:

• Local testing that everything works
• Cross-checking that I didn't eff up the rebase
• Proper attribution in the git commit messages (Co-Authored-By lines)
• Some refactoring to avoid code duplication introduced by the original patch set.

[masterzen]

LGTM

[BenBE]

When I went through the source for formatting, this loop condition (and the place in the duplicated routine) irritated me somewhat, as p is guaranteed non-NULL. May *p++ be the actually intended check?

[masterzen]

I don't thinkargs.data is null terminated anyway, so *p++ is not even a good guard here.

I think the intent is that len is going to be 0 as we scan the args.data and reach it's length, which will make the loop exit with the else condition.

So basically we never check anything in that while condition.

We could change this to a for loop to make this more correct.

[BenBE]

A first correction (as part of this PR) might be as follows:

diff --git a/ngx_http_uploadprogress_module.c b/ngx_http_uploadprogress_module.c
index 44a7852a..d9d7997a 100644
--- a/ngx_http_uploadprogress_module.c
+++ b/ngx_http_uploadprogress_module.c
@@ -382,10 +382,8 @@ get_tracking_id(ngx_http_request_t * r)
                 i = 1;
                 break;
             }
-            else if (!len) {
-                break;
-            }
-        } while (p++);
+            p++;
+        } while (len > 0);
 
         if (i) {
             start_p = p += upcf->header.len + 1;
@@ -475,10 +473,8 @@ get_tracking_ids_mul(ngx_http_request_t * r)
                 i = 1;
                 break;
             }
-            else if (!len) {
-                break;
-            }
-        } while (p++);
+            p++;
+        } while (len > 0);
 
         if (i) {
             start_p = p += upcf->header_mul.len + 1;

That's not perfect either, but already an improvement over the initial variant. The overall routine looks like it has some unconventional way to parse things, so a proper refactoring is needed for cleaning up things. Like that i being used as a bool; instead of using bool for this sort of thing.

[masterzen]

Yes, that would work better indeed.

[BenBE]

Did one refactoring to remove some duplicate code. For reference, this is what (in the original patch), was the difference between these two routines:

--- get_tracking_id 2024-12-12 16:29:12.119211606 +0100
+++ get_tracking_ids_mul   2024-12-12 16:29:21.557208620 +0100
@@ -1,5 +1,5 @@
 static ngx_str_t*
-get_tracking_id(ngx_http_request_t * r)
+get_tracking_ids_mul(ngx_http_request_t * r)
 {
     u_char                          *p, *start_p;
     ngx_uint_t                       i;
@@ -10,7 +10,7 @@
 
     upcf = ngx_http_get_module_loc_conf(r, ngx_http_uploadprogress_module);
 
-    ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "upload-progress: get_tracking_id");
+    ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "upload-progress: get_tracking_ids");
 
     part = &r->headers_in.headers.part;
     header = part->elts;
@@ -27,20 +27,20 @@
             i = 0;
         }
 
-        if (header[i].key.len == upcf->header.len
-            && ngx_strncasecmp(header[i].key.data, upcf->header.data,
+        if (header[i].key.len == upcf->header_mul.len
+            && ngx_strncasecmp(header[i].key.data, upcf->header_mul.data,
                            header[i].key.len) == 0) {
             ret = ngx_calloc(sizeof(ngx_str_t), r->connection->log );
             ret->data = header[i].value.data;
             ret->len = header[i].value.len;
             ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, 
-                            "upload-progress: get_tracking_id found header: %V", ret);
+                            "upload-progress: get_tracking_ids found header: %V", ret);
             return ret;
         }
     }
 
     ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, 
-                    "upload-progress: get_tracking_id no header found");
+                    "upload-progress: get_tracking_ids no header found");
 
     /* not found, check as a request arg */
     /* it is possible the request args have not been yet created (or already released) */
@@ -55,8 +55,8 @@
         p = args.data;
         do {
             ngx_uint_t len = args.len - (p - args.data);
-            if (len >= (upcf->header.len + 1) && ngx_strncasecmp(p, upcf->header.data, upcf->header.len) == 0
-                && p[upcf->header.len] == '=') {
+            if (len >= (upcf->header_mul.len + 1) && ngx_strncasecmp(p, upcf->header_mul.data, upcf->header_mul.len) == 0
+                && p[upcf->header_mul.len] == '=') {
               ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, 
                              "upload-progress: get_tracking_id found args: %s",p);
                 i = 1;
@@ -68,7 +68,7 @@
         while(p++);
 
         if (i) {
-            start_p = p += upcf->header.len + 1;
+            start_p = p += upcf->header_mul.len + 1;
             while (p < args.data + args.len) {
                 if (*((p++) + 1) == '&') {
                     break;

The multi parameter in my implementation is currently unused, but is intended to account for allowing multiple IDs in the header string (and thus possibly multiple instances of the header field).

A similar code dedup can likely be done for all the ngx_http_upload_progress_*_output functions.

[masterzen]

I fail to understand what this is doing?

[masterzen]

Also, multi is not used anywhere in get_tracking_id_internal if I'm not mistaken?

[BenBE]

Correct. The (void)varname; is a common idiom for "this variable is unused". See also my remark about this in my previous comment:

The multi parameter in my implementation is currently unused, but is intended to account for allowing multiple IDs in the header string (and thus possibly multiple instances of the header field).

[masterzen]

👍 that's way nicer, thanks!