Re: svn commit: r1879307 - /httpd/httpd/trunk/modules/dav/main/mod_dav.c

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1879307 - /httpd/httpd/trunk/modules/dav/main/mod_dav.c

Ruediger Pluem


On 6/28/20 3:28 PM, [hidden email] wrote:

> Author: minfrin
> Date: Sun Jun 28 13:28:19 2020
> New Revision: 1879307
>
> URL: http://svn.apache.org/viewvc?rev=1879307&view=rev
> Log:
> Add implementation of deliver_report and gather_reports to mod_dav.c.
>
> Modified:
>     httpd/httpd/trunk/modules/dav/main/mod_dav.c
>
> Modified: httpd/httpd/trunk/modules/dav/main/mod_dav.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/dav/main/mod_dav.c?rev=1879307&r1=1879306&r2=1879307&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/dav/main/mod_dav.c (original)
> +++ httpd/httpd/trunk/modules/dav/main/mod_dav.c Sun Jun 28 13:28:19 2020

> @@ -4228,24 +4271,36 @@ static int dav_method_report(request_rec
>      ap_set_content_type(r, DAV_XML_CONTENT_TYPE);
>  
>      /* run report hook */
> -    if ((err = (*vsn_hooks->deliver_report)(r, resource, doc,
> -                                            r->output_filters)) != NULL) {
> -        if (! r->sent_bodyct)
> -          /* No data has been sent to client yet;  throw normal error. */
> -          return dav_handle_err(r, err, NULL);
> -
> -        /* If an error occurred during the report delivery, there's
> -           basically nothing we can do but abort the connection and
> -           log an error.  This is one of the limitations of HTTP; it
> -           needs to "know" the entire status of the response before
> -           generating it, which is just impossible in these streamy
> -           response situations. */
> -        err = dav_push_error(r->pool, err->status, 0,
> -                             "Provider encountered an error while streaming"
> -                             " a REPORT response.", err);
> -        dav_log_err(r, err, APLOG_ERR);
> -        r->connection->aborted = 1;
> +    result = dav_run_deliver_report(r, resource, doc,
> +            r->output_filters, &err);
> +    switch (result) {
> +    case OK:
>          return DONE;

What happens if err != NULL. Should we handle that here like in the default case or
should the hook implementer ensure to not return OK if err != NULL?

> +    case DECLINED:
> +        /* No one handled the report */
> +        return HTTP_NOT_IMPLEMENTED;

Previously we were returning DECLINED in case there was no vsn_hooks and thus nothing to report. Now we return
HTTP_NOT_IMPLEMENTED. Why this change?

> +    default:
> +        if ((err) != NULL) {
> +
> +            if (! r->sent_bodyct) {
> +              /* No data has been sent to client yet;  throw normal error. */
> +              return dav_handle_err(r, err, NULL);
> +            }
> +
> +            /* If an error occurred during the report delivery, there's
> +               basically nothing we can do but abort the connection and
> +               log an error.  This is one of the limitations of HTTP; it
> +               needs to "know" the entire status of the response before
> +               generating it, which is just impossible in these streamy
> +               response situations. */
> +            err = dav_push_error(r->pool, err->status, 0,
> +                                 "Provider encountered an error while streaming"
> +                                 " a REPORT response.", err);
> +            dav_log_err(r, err, APLOG_ERR);
> +            r->connection->aborted = 1;
> +
> +            return DONE;
> +        }
>      }
>  
>      return DONE;

Regards

Rüdiger


Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1879307 - /httpd/httpd/trunk/modules/dav/main/mod_dav.c

Graham Leggett
On 29 Jun 2020, at 12:10, Ruediger Pluem <[hidden email]> wrote:

+    result = dav_run_deliver_report(r, resource, doc,
+            r->output_filters, &err);
+    switch (result) {
+    case OK:
        return DONE;

What happens if err != NULL. Should we handle that here like in the default case or
should the hook implementer ensure to not return OK if err != NULL?

Hmmm… this needs tightening up.

+    case DECLINED:
+        /* No one handled the report */
+        return HTTP_NOT_IMPLEMENTED;

Previously we were returning DECLINED in case there was no vsn_hooks and thus nothing to report. Now we return
HTTP_NOT_IMPLEMENTED. Why this change?

When mod_dav was written, it was assumed that the REPORT method was exclusive to https://tools.ietf.org/html/rfc3253. As a result, if a versioning implementation was present, the versioning implementation (eg svn) was expected to handle REPORT, and no other.

Other WebDAV extensions arrived, and they also used REPORT. But if a versioning implementation was present, that implementation would consume the report, not recognise the report, and then return an error. The mod_caldav code that Nokia worked on in the late 2000s hacked around this by reading the REPORT body first, then creating an input filter to re-insert the body if mod_caldav realised the REPORT wasn’t for it. Parsing XML bodies over and over is very ugly.

What this change does is formally recognise in code that multiple RFCs handle the REPORT method. We parse the XML body, then offer this parsed body to anyone who wants it via the deliver_report hook. Modules get their chance to handle the report. As a last resort, the mod_dav core passes the body to the versioning implementaton (eg svn) which then responds as previous, maintaining existing behaviour.

To answer you direct question above, at the point we return HTTP_NOT_IMPLEMENTED we have consumed the REPORT body. We can’t return DECLINED in this case.

There are a significant number of RFCs that mod_dav doesn't support, such as https://tools.ietf.org/html/rfc5689. We have RFC compliance work to do.

Regards,
Graham