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