[mod_python] Some observations after writing my own modpython

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Graham Dumpleton wrote:
> Maybe you would like to review my code for me. :-)
> 
>  http://www.modwsgi.org

I've taken a quick look but am not too familiar with what it is doing.
(Pretty much all the Python wrapping of other libraries I do is to
deliberately ignore Python "standards" and follow what the libraries do
instead :-)

I am somewhat baffled as to why you have the "daemon" mode.  Surely
ProxyPass would be sufficient.  And if it wasn't then fastcgi/scgi
should be.

I'm astounded at how much effort you have put into being compatible
across so many Apache versions and packaging tweaks on the various
distributions.

You've also got a lot of options and complexity for those options, such
as optionally disabling protection against stdio usage and signals.  My
own tastes are towards insisting on standards compliance and not have
more code (and hence more complexity and possibility for bugs).

At line 729 after the PyErr_Fetch, I believe you should use
PyErr_Normalize to set unset value/tracebacks.

The Log_output function has lots of scary string arithmetic and memory
allocations.  Wouldn't the various apr_str functions do the trick?

The if(*msg) in Log_write is redundant since format "s" to
PyArg_ParseTuple will always fill in the char*.

You may find it worthwhile using PyObject_CallFunction instead of
BuildValue and CallObject.  You have one less thing to track refcounts
for and one less line of code.

Input_read also has the string arithmetic and memory allocations.  Why
not use the Python PyString or apr routines for all of those?

For the various objects that have a request_rec*, I don't see how they
deal with outliving the request_req.

A lot of the config values will accept nonsensical values for booleans
because the config routines effectively do a caseless compare for "off"
and treat everything else as on, including "0", "1" etc.  I'd suggest
tightening that up to only accept the values you want rather than
anything.  (You know someday someone will make a configuration mistake
and get unexpected behaviour and take ages to track down that "0" didn't
actually turn something off or "1" didn't turn it on).

On the whole the code is clear and readable.

> All my work on SWIGing the Apache APIs could probably have saved you a
> lot of effort. :-)

Not really since I only wrapped the data and methods I needed which
amounts to about 20 items.  However it was very obvious that an
automated approach to making a wrapper is by far the best way of doing
it.  There are so many very similar things, but with the odd exception
or tweak here and there.

> Is what you have done patches to mod_python, or have you gone and
> written your own Apache module from scratch which is independent of
> mod_python, or uses the exported mod_python optional functions for
> getting access to interpreters? 

It is completely independent started from scratch code.  I didn't bother
with multiple interpreters since all the Python code that runs is mine
and it isn't "hostile".

> Are you intending to post your code anywhere so we can have a look?

Sadly I can't since I don't own it :-(  However it isn't that
interesting because it is subset of Apache apis, only request_rec is
wrapped, only the main interpreter etc.  I also am not constrained by
backwards compatibility for Python (using 2.5) or Apache (using 2.2) or
by an installed base as modpython is.

> And there are a lot more problems than that. Personally, in terms of
> adding more and more access to the Apache API I am far from convinced
> that mod_python is a good starting point. :-)

I certainly chose not to start from mod_python and technically only
needed one more api :-)  It would be useful to look at the modperl code
generator since they must have the meta-information about apis and types
somewhere.

> Trying to cache values,
> especially when they aren't all read only and can be updated by other
> Apache functions, just makes the job of writing a hand coded wrapper
> even more work so I don't begrudge the original author for not doing
> it.

For OOR you just have to stash the Python object pointer somewhere in
the Apache object and so the pool note functions would work.  I don't
see anywhere that values are cached, unless you mean something like
getting the same PyString objects out when setting a setting field.
That certainly is going too far.

This has all been a valuable learning experience and I am very glad to
be able to use Apache for all authentication and authorization.

Roger
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)

iD8DBQFGXSptmOOfHg372QQRAiUiAKCjPbAu0jwCkmhE3lvFrjCwXR8+fACgw1nC
bB6CkpocNG/aSA23JbRQ1S0=
=dqja
-----END PGP SIGNATURE-----