dav_new_error_*() and errno, revisit before 2.4 GA

[Jeff Trawick]

At present, whatever was in errno at the time the dav_error {} was
created is treated as an apr_status_t by ap_log_rerror().

http://mail-archives.apache.org/mod_mbox/httpd-dev/200211.mbox/%3C20021101033848.B29006@...%3E

dav_error {} should have an apr_status_t field instead of an errno
field; functions which create a dav_error should have an apr_status_t
parameter.

If there's no direct apr_status_t representation of the error, the
caller will have to decide what to do (no magic portable solution
AFAIK, but no worse than today).

Concerns?

[Greg Stein-4]

On Wed, Nov 11, 2009 at 17:11, Jeff Trawick <trawick@...> wrote:

> At present, whatever was in errno at the time the dav_error {} was
> created is treated as an apr_status_t by ap_log_rerror().
>
> http://mail-archives.apache.org/mod_mbox/httpd-dev/200211.mbox/%3C20021101033848.B29006@...%3E
>
> dav_error {} should have an apr_status_t field instead of an errno
> field; functions which create a dav_error should have an apr_status_t
> parameter.
>
> If there's no direct apr_status_t representation of the error, the
> caller will have to decide what to do (no magic portable solution
> AFAIK, but no worse than today).
>
> Concerns?

dav_error was designed during the 1.3 series. APR wasn't even being
discussed. So yeah... there is definitely a disconnect from "then" to
"now".

I'd be quite supportive of changing that, though (strictly speaking)
that is probably an API change.

Cheers,
-g

[Jeff Trawick]

On Wed, Nov 11, 2009 at 6:58 PM, Greg Stein <gstein@...> wrote:

> On Wed, Nov 11, 2009 at 17:11, Jeff Trawick <trawick@...> wrote:
>> At present, whatever was in errno at the time the dav_error {} was
>> created is treated as an apr_status_t by ap_log_rerror().
>>
>> http://mail-archives.apache.org/mod_mbox/httpd-dev/200211.mbox/%3C20021101033848.B29006@...%3E
>>
>> dav_error {} should have an apr_status_t field instead of an errno
>> field; functions which create a dav_error should have an apr_status_t
>> parameter.
>>
>> If there's no direct apr_status_t representation of the error, the
>> caller will have to decide what to do (no magic portable solution
>> AFAIK, but no worse than today).
>>
>> Concerns?
>
> dav_error was designed during the 1.3 series. APR wasn't even being
> discussed. So yeah... there is definitely a disconnect from "then" to
> "now".
>
> I'd be quite supportive of changing that, though (strictly speaking)
> that is probably an API change.

well, yes it is an API change ;)  I think the issue is whether or not
you in the DAV community want to push it on your third-party
colleagues.

A third-party mod could define something like this for compatibility:

#if AP_MODULE_MAGIC_AT_LEAST(xxx,y)
#define foo_dav_new_error(p, stat, errid, rv, desc) \
    dav_new_error(p, stat, errid, rv, desc)
#else
#define foo_dav_new_error(p, stat, errid, rv, desc) \
  (errno = (rv),                                   \
   dav_new_error(p, stat, errid, desc))
#endif

and go ahead and change its code to pass in an apr_status_t.

If it is too ugly to force this on third-party modules, we'll need to
add new APIs and use those ourselves, as it is too ugly for us to keep
mismanaging the error codes.

[Greg Stein-4]

On Wed, Nov 11, 2009 at 20:09, Jeff Trawick <trawick@...> wrote:

> On Wed, Nov 11, 2009 at 6:58 PM, Greg Stein <gstein@...> wrote:
>> On Wed, Nov 11, 2009 at 17:11, Jeff Trawick <trawick@...> wrote:
>>> At present, whatever was in errno at the time the dav_error {} was
>>> created is treated as an apr_status_t by ap_log_rerror().
>>>
>>> http://mail-archives.apache.org/mod_mbox/httpd-dev/200211.mbox/%3C20021101033848.B29006@...%3E
>>>
>>> dav_error {} should have an apr_status_t field instead of an errno
>>> field; functions which create a dav_error should have an apr_status_t
>>> parameter.
>>>
>>> If there's no direct apr_status_t representation of the error, the
>>> caller will have to decide what to do (no magic portable solution
>>> AFAIK, but no worse than today).
>>>
>>> Concerns?
>>
>> dav_error was designed during the 1.3 series. APR wasn't even being
>> discussed. So yeah... there is definitely a disconnect from "then" to
>> "now".
>>
>> I'd be quite supportive of changing that, though (strictly speaking)
>> that is probably an API change.
>
> well, yes it is an API change ;)  I think the issue is whether or not
> you in the DAV community want to push it on your third-party
> colleagues.
>
> A third-party mod could define something like this for compatibility:
>
> #if AP_MODULE_MAGIC_AT_LEAST(xxx,y)
> #define foo_dav_new_error(p, stat, errid, rv, desc) \
>    dav_new_error(p, stat, errid, rv, desc)
> #else
> #define foo_dav_new_error(p, stat, errid, rv, desc) \
>  (errno = (rv),                                   \
>   dav_new_error(p, stat, errid, desc))
> #endif
>
> and go ahead and change its code to pass in an apr_status_t.
>
> If it is too ugly to force this on third-party modules, we'll need to
> add new APIs and use those ourselves, as it is too ugly for us to keep
> mismanaging the error codes.

I have no opinion in this space. I'm very much out of touch nowadays
with the mod_dav users/community. The one particular user of mod_dav
that I'm concerned with is mod_dav_svn, and *that* is about to become
an Apache project :-)

svn can certainly use that macro. "You" get to measure the disruption
to other API users of Apache.

In fact, we floated the idea of whether mod_dav_svn can/should be
transferred over to httpd, rather than maintained as part of svn.
Total random comment, but a couple people went "hmm...." It would be
kinda neat for any Apache server to have built-in support for svn :-)
(and would solve a bunch of versioning combinatorics for svn...)

Cheers,
-g

[Jeff Trawick]

On Wed, Nov 11, 2009 at 10:06 PM, Greg Stein <gstein@...> wrote:

> On Wed, Nov 11, 2009 at 20:09, Jeff Trawick <trawick@...> wrote:
>> On Wed, Nov 11, 2009 at 6:58 PM, Greg Stein <gstein@...> wrote:
>>> On Wed, Nov 11, 2009 at 17:11, Jeff Trawick <trawick@...> wrote:
>>>> At present, whatever was in errno at the time the dav_error {} was
>>>> created is treated as an apr_status_t by ap_log_rerror().
>>>>
>>>> http://mail-archives.apache.org/mod_mbox/httpd-dev/200211.mbox/%3C20021101033848.B29006@...%3E
>>>>
>>>> dav_error {} should have an apr_status_t field instead of an errno
>>>> field; functions which create a dav_error should have an apr_status_t
>>>> parameter.
>>>>
>>>> If there's no direct apr_status_t representation of the error, the
>>>> caller will have to decide what to do (no magic portable solution
>>>> AFAIK, but no worse than today).
>>>>
>>>> Concerns?
>>>
>>> dav_error was designed during the 1.3 series. APR wasn't even being
>>> discussed. So yeah... there is definitely a disconnect from "then" to
>>> "now".
>>>
>>> I'd be quite supportive of changing that, though (strictly speaking)
>>> that is probably an API change.
>>
>> well, yes it is an API change ;)  I think the issue is whether or not
>> you in the DAV community want to push it on your third-party
>> colleagues.
>>
>> A third-party mod could define something like this for compatibility:
>>
>> #if AP_MODULE_MAGIC_AT_LEAST(xxx,y)
>> #define foo_dav_new_error(p, stat, errid, rv, desc) \
>>    dav_new_error(p, stat, errid, rv, desc)
>> #else
>> #define foo_dav_new_error(p, stat, errid, rv, desc) \
>>  (errno = (rv),                                   \
>>   dav_new_error(p, stat, errid, desc))
>> #endif
>>
>> and go ahead and change its code to pass in an apr_status_t.
>>
>> If it is too ugly to force this on third-party modules, we'll need to
>> add new APIs and use those ourselves, as it is too ugly for us to keep
>> mismanaging the error codes.
>
> I have no opinion in this space. I'm very much out of touch nowadays
> with the mod_dav users/community. The one particular user of mod_dav
> that I'm concerned with is mod_dav_svn, and *that* is about to become
> an Apache project :-)
>
> svn can certainly use that macro. "You" get to measure the disruption
> to other API users of Apache.

Other folks will just have to recompile to accommodate the new MMN.
That has to be okay for pre-GA.

Posting a heads-up rough patch to dav lists for other DAV modules will help.

> In fact, we floated the idea of whether mod_dav_svn can/should be
> transferred over to httpd, rather than maintained as part of svn.
> Total random comment, but a couple people went "hmm...." It would be
> kinda neat for any Apache server to have built-in support for svn :-)

shrug

> (and would solve a bunch of versioning combinatorics for svn...)

sure

--/--

I'll post an errno->apr_status_t patch in the next day or two.

[Jeff Trawick]

On Thu, Nov 12, 2009 at 7:04 AM, Jeff Trawick <trawick@...> wrote:

> On Wed, Nov 11, 2009 at 10:06 PM, Greg Stein <gstein@...> wrote:
>> On Wed, Nov 11, 2009 at 20:09, Jeff Trawick <trawick@...> wrote:
>>> On Wed, Nov 11, 2009 at 6:58 PM, Greg Stein <gstein@...> wrote:
>>>> On Wed, Nov 11, 2009 at 17:11, Jeff Trawick <trawick@...> wrote:
>>>>> At present, whatever was in errno at the time the dav_error {} was
>>>>> created is treated as an apr_status_t by ap_log_rerror().
>>>>>
>>>>> http://mail-archives.apache.org/mod_mbox/httpd-dev/200211.mbox/%3C20021101033848.B29006@...%3E
>>>>>
>>>>> dav_error {} should have an apr_status_t field instead of an errno
>>>>> field; functions which create a dav_error should have an apr_status_t
>>>>> parameter.
>>>>>
>>>>> If there's no direct apr_status_t representation of the error, the
>>>>> caller will have to decide what to do (no magic portable solution
>>>>> AFAIK, but no worse than today).
>>>>>
>>>>> Concerns?
>>>>
>>>> dav_error was designed during the 1.3 series. APR wasn't even being
>>>> discussed. So yeah... there is definitely a disconnect from "then" to
>>>> "now".
>>>>
>>>> I'd be quite supportive of changing that, though (strictly speaking)
>>>> that is probably an API change.
>>>
>>> well, yes it is an API change ;)  I think the issue is whether or not
>>> you in the DAV community want to push it on your third-party
>>> colleagues.
>>>
>>> A third-party mod could define something like this for compatibility:
>>>
>>> #if AP_MODULE_MAGIC_AT_LEAST(xxx,y)
>>> #define foo_dav_new_error(p, stat, errid, rv, desc) \
>>>    dav_new_error(p, stat, errid, rv, desc)
>>> #else
>>> #define foo_dav_new_error(p, stat, errid, rv, desc) \
>>>  (errno = (rv),                                   \
>>>   dav_new_error(p, stat, errid, desc))
>>> #endif
>>>
>>> and go ahead and change its code to pass in an apr_status_t.
>>>
>>> If it is too ugly to force this on third-party modules, we'll need to
>>> add new APIs and use those ourselves, as it is too ugly for us to keep
>>> mismanaging the error codes.
>>
>> I have no opinion in this space. I'm very much out of touch nowadays
>> with the mod_dav users/community. The one particular user of mod_dav
>> that I'm concerned with is mod_dav_svn, and *that* is about to become
>> an Apache project :-)
>>
>> svn can certainly use that macro. "You" get to measure the disruption
>> to other API users of Apache.
>
> Other folks will just have to recompile to accommodate the new MMN.
> That has to be okay for pre-GA.
>
> Posting a heads-up rough patch to dav lists for other DAV modules will help.
>
>> In fact, we floated the idea of whether mod_dav_svn can/should be
>> transferred over to httpd, rather than maintained as part of svn.
>> Total random comment, but a couple people went "hmm...." It would be
>> kinda neat for any Apache server to have built-in support for svn :-)
>
> shrug
>
>> (and would solve a bunch of versioning combinatorics for svn...)
>
> sure
>
> --/--
>
> I'll post an errno->apr_status_t patch in the next day or two.

- For mod_dav in httpd trunk/future 2.4:
http://people.apache.org/~trawick/mod_dav_err_api.txt

The dav_new_error*() functions and dav_error structure already have
status and error_id parameter/fields; what's a better choice than
"aprerr" to replace the old "save_errno"?

- For subversion modules: http://people.apache.org/~trawick/svn_err_api.txt

svn already had its own dav_svn__new_error_tag() wrapper around
dav_new_error_tag() to deal with errno issues, so I added another
wrapper for dav_new_error().  I didn't add the apr_status_t value to
the interface of the dav_svn wrapper, as it seemed that there was
almost never an applicable apr_status_t handy to pass in.

The MMN number in the #if isn't correct; that will be determined when
the httpd patch is committed.

--/--

Concerns (or "aprerr" replacement) before I commit the httpd changes?
Does somebody want to fix any other mod_dav APIs in the same MMN
change?

[Joe Orton]

On Wed, Nov 18, 2009 at 07:29:46AM -0500, Jeff Trawick wrote:
> - For mod_dav in httpd trunk/future 2.4:
> http://people.apache.org/~trawick/mod_dav_err_api.txt

Great stuff!   Looks good here.   Joe

[Jeff Trawick]

On Thu, Nov 19, 2009 at 11:41 AM, Joe Orton <jorton@...> wrote:
> On Wed, Nov 18, 2009 at 07:29:46AM -0500, Jeff Trawick wrote:
>> - For mod_dav in httpd trunk/future 2.4:
>> http://people.apache.org/~trawick/mod_dav_err_api.txt
>
> Great stuff!   Looks good here.   Joe

Thanks for looking.  This has been committed to httpd trunk as r882274
and r882280, and the Subversion modules for httpd will no longer
compile against trunk.

I just updated the previous svn patch at
http://people.apache.org/~trawick/svn_err_api.txt to check for the
proper httpd module magic number.