Re: apache 2.2 support

[Graham Dumpleton]

Jim Gallacher wrote ..
> I'd like to checkin my patch to support apache 2.2. It doesn't add any
> new functionality. Any objections?

If I recollect, the only real issue was the implications on Win32 platform
of changing the APR_STATUS_IS_SUCCESS(s) test to (s == APR_SUCCESS),
because of Win32 the definition was originally:

#define APR_STATUS_IS_SUCCESS(s) ((s) == APR_SUCCESS \ 
                || (s) == APR_OS_START_SYSERR + ERROR_SUCCESS) 

Anyway, this prompted be to look at cases where APR_STATUS_IS_SUCCESS()
is used. They are in connobject.c and filterobject.c. I noticed something that
may need to be looked at, not strictly related to Apache 2.2 support.

This is that we just made change in connobject.c to check for initial empty
bucket brigade and to loop when that occurred. This could have come about
because of EGAIN on socket read. The EGAIN wasn't able to be seen as a
distinct error as the lower level read routine squashed the error returning
success with empty bucket brigade. The changed code was:

    while (APR_BRIGADE_EMPTY(bb)) {          
        Py_BEGIN_ALLOW_THREADS;
        rc = ap_get_brigade(c->input_filters, bb, mode, APR_BLOCK_READ, 
bufsize);       
        Py_END_ALLOW_THREADS;
                                  
        if (! APR_STATUS_IS_SUCCESS(rc)) {
            PyErr_SetObject(PyExc_IOError,
                            PyString_FromString("Connection read error"));      
      return NULL;
        }   
    }                       

Now looking at instances in filterobject.c where ap_get_brigade() is used,
have checks such as:

        Py_BEGIN_ALLOW_THREADS;
        self->rc = ap_get_brigade(self->f->next, self->bb_in, self->mode,
                                  APR_BLOCK_READ, self->readbytes);
        Py_END_ALLOW_THREADS;

        if (!APR_STATUS_IS_EAGAIN(self->rc) && 
!APR_STATUS_IS_SUCCESS(self->rc)) {
            PyErr_SetObject(PyExc_IOError,
                            PyString_FromString("Input filter read error"));
            return NULL;
        }
    }

The code is similar to what was in connobject.c except that it checks for EGAIN
case. Already know that EGAIN may actually be squashed, but then, it doesn't
raise an error is sees EGAIN anyway.

The next section of code has:

    b = APR_BRIGADE_FIRST(self->bb_in);

    if (b == APR_BRIGADE_SENTINEL(self->bb_in))
        return PyString_FromString("");

Now I am assuming here that the check with APR_BRIGADE_SENTINEL() is
equivalent to APR_BRIGADE_EMPTY(bb). Yes/No? Would be nice to know
that they are. If it isn't then is an empty bucket brigade being handled?

Anyway, back to Win32 platform. I can't see that the APR_OS_START_SYSERR
offset check is important. This is because any function calls would only
return the error status if the call itself failed. Take for example socket
calls.

#ifndef _WIN32_WCE
    rv = WSARecv(sock->socketdes, &wsaData, 1, &dwBytes, &flags, NULL, NULL);
#else
    rv = recv(sock->socketdes, wsaData.buf, wsaData.len, 0);
    dwBytes = rv;
#endif 
    if (rv == SOCKET_ERROR) {
        lasterror = apr_get_netos_error();
        *len = 0;
        return lasterror;
    }

    *len = dwBytes;
    return dwBytes == 0 ? APR_EOF : APR_SUCCESS;

It only returns lasterror when it fails, otherwise APR_SUCCESS is returned
explicitly. I am going to guess that Apache would be consistent about
that. Thus highly doubt that non zero error status which indicated
success would occur and so extra check used before redundant.

In that context, see no problems with changes being commited.


Graham