httpd-apreq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Max Kellermann <...@duempel.org>
Subject [PATCH] first snapshot of my "let's kill void*env" patch
Date Sat, 08 Jan 2005 20:55:16 GMT
Hi,

I mentioned last week that I'm not satisfied with the current apreq2
API, it has a number of flaws:

- despite the vtable-like apreq_env_t struct, there can only be one
  env module within a process
- the apreq_env_module does not help here, because it is not thread
  safe; the caller must ensure that the current env module is not
  changed within the lifetime of any apreq object
- to go around the "one env module only" limitation, there is a second
  parameter in apreq_request() and apreq_cookie(), which lets the
  caller specify a string to parse (which is not an elegant hack)
- the nature of the void*env parameter is defined by the caller, but
  is interpreted by the env module. That means, the caller must check
  which env module is active before each call, otherwise the results
  are undefined (SIGSEGV?). But this check is not possible without
  thread safety, and adds responsibility to the application (bloat)
  which can be omitted with a proper design.
- casting to void* (and casting back) is dangerous and should be
  avoided; a common type of bug cannot be caught with C compiler
  warnings. This concerns apreq even more because the two casts are in
  different software layers (caller, and apreq itself) and therefore
  not under direct control of a single entity. void* should go away.

Solution:
- remove the global apreq_env variable, it is of no use
- every env module gets its own explicit constructor, which accepts
  concrete arguments (e.g. request_rec*) instead of an untyped pointer
  and returns a handle (let's call it apreq_handle_t* for now)
- this handle is given to apreq_request() and apreq_cookie() (and all
  the other functions which are currently taking void* parameters)
- remove the second parameter from apreq_request() and apreq_cookie()
  and implement an own env module for parsing caller-defined strings
  instead

Example:

old: req = apreq_request(r, NULL);
new: req = apreq_request(apreq_get_apache2(r));

The "old" example is unsafe, you can't check or guarantee if the
currently selected env module understands a request_rec pointer. Such
an error would result in a C compiler error in the "new" example. If
you forget to load mod_apreq, Apache will refuse to load your module,
because the symbol "apreq_get_apache2" is not available - no "late"
segmentation fault in the first invocation of your module.

It makes no sense to hide several modules wanting different parameters
under the hood of one function, like apreq_request() and the others do
- the caller code has to hard code which env module it requires
anyway, it does not hurt to let it call a different constructor for
the Apache 2 env module than for a CGI env module. Once you created
the apreq_handle_t object, all further calls are transparent.


I have implemented my idea, two patches for the subversion trunk
(revision 124680) are attached to this email. First apply
apreq-fix_gcc_warnings.patch (I always try to use gcc with all
warnings enabled), then apreq-lets_kill_void_env-0.0.1.patch.

This is untested work in progress, I'm publishing this snapshot only
to discuss my idea on this mailing list.

Todo:
- decide if my idea is good
- this is untested code. test it. we more test cases.
- I'm unsure about my subrequest handling changes; test that
  thoroughly
- no work has been done on the perl glue yet
  - review the Perl API
- remove the second parameter from apreq_request() and apreq_cookie()
- review names: apreq_handle_t, null_module, constructor names
  apreq_get_X()
- review the apreq_env_t interface
- more apreq cleanup
  - redesign apreq_parser_initialize() in a thread safe way
  - some strings go through apr_strdup() twice, e.g. in
    apreq_env_temp_dir()

Max


Mime
View raw message