twisted/protocols/_c_urlarg.c compilation errors on IBM AIX

[aprilmay]

Errors building on IBM AIX 5.3 with native compiler xlc_r:

Running Twisted-9.0.0/setup.py -q bdist_egg --dist-dir /tmp/easy_install-dO3vwp/Twisted-9.0.0/egg-dist-tmp-ECS6yS
"conftest.c", line 1.10: 1506-296 (S) #include file <sys/epoll.h> not found.
"conftest.c", line 1.23: 1506-356 (W) Compilation unit is empty.
"twisted/protocols/_c_urlarg.c", line 65.28: 1506-280 (E) Function argument assignment between types "const char*" and "unsigned char*" is not allowed.
"twisted/protocols/_c_urlarg.c", line 75.28: 1506-280 (E) Function argument assignment between types "const char*" and "unsigned char*" is not allowed.
"twisted/protocols/_c_urlarg.c", line 83.28: 1506-280 (E) Function argument assignment between types "const char*" and "unsigned char*" is not allowed.
"twisted/protocols/_c_urlarg.c", line 85.28: 1506-280 (E) Function argument assignment between types "const char*" and "unsigned char*" is not allowed.
"twisted/protocols/_c_urlarg.c", line 93.20: 1506-280 (E) Function argument assignment between types "const char*" and "unsigned char*" is not allowed.
"twisted/protocols/_c_urlarg.c", line 96.20: 1506-280 (E) Function argument assignment between types "const char*" and "unsigned char*" is not allowed.
"twisted/protocols/_c_urlarg.c", line 97.20: 1506-280 (E) Function argument assignment between types "const char*" and "unsigned char*" is not allowed.

Unfortunately _c_urlarg.so is generated, so:

>>> from twisted.protocols._c_urlarg import unquote
>>> unquote("")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
MemoryError: out of memory

When running a twisted web server, there's a nasty CPU loop around line 122 of twisted/web/server.py. I guess other defects might occur so the easy workaround os to remove the _c_urlarg.so file.

Everything else seem to work like a charm on AIX (with python 2.6 from  http://www.perzl.org/aix/index.php?n=Main.Python)

[spiv]

Possible fix?

[spiv]

I wonder if explicitly casting to (const char *) in the OUTPUTCHAR macro would fix those warnings? I've attached a one-liner patch to this ticket that does that; it builds and tests cleanly for me.

[exarkun]

On AIX?

[spiv]

No, sorry for being unclear. What I should have said is that it builds and runs cleanly for me on Linux (Ubuntu Karmic, x86), so it is at least not a regression.

We still need someone with AIX, perhaps the reporter, to try it there.

[aprilmay]

The memory error still persists after applying the proposed patch (however the compilation errors have disappeared).

[spiv]

Oh, I bet I know what this is. On some platforms malloc(0) fails.

So probably the fix for the MemoryError is:

if (length == 0) {
    return PyString_FromStringAndSize("", 0);
}

This would need to happen just before the call to NewOutput. Patch attached.

[spiv]

fix for memory error

[aprilmay]

Yes, this new patch fixed the issue:

>>> from twisted.protocols._c_urlarg import unquote
>>> unquote("")
''

Web server runs smoothly on AIX now. Thanks !!!

A test case could be also added?

[spiv]

Thanks very much for testing that and reporting the results so quickly.

By the way, twisted.web.test.test_http.QueryArgumentsTestCase.testUnquote already tests this case:

        # Empty string
        self.failUnlessEqual(urllib.unquote(""), _c_urlarg.unquote(""))

You should be able to run that test yourself with trial twisted.web.test.test_http.QueryArgumentsTestCase.testUnquote, or even better just run the whole test suite (trial twisted) and file bugs for any other failures :)

[exarkun]

1. Warnings are gone on Ubuntu 8.10. This is good, right? Or is it? This kind of C fix always bothers me. I looked up s# in the Python docs ( http://python.org/doc/1.5.2p2/ext/parseTuple.html) and I see that it's actually declared as a char *. I wonder why the variable in this function is declared as a unsigned char *. If it were just char * then the cast wouldn't be necessary, right? Maybe there's some other good reason it's an unsigned char *, though.
2. The tests pass (again on Ubuntu 8.10). They did before, of course. Also, strange that the tests are in Twisted Web, when the code is in Twisted Core.
3. As I mentioned on IRC, I can't use combinator to merge the branch. The revision doesn't get picked up. Not a big deal here, since you're going to merge it and I suppose you know how. It'd be nice to avoid this case in the future, though. :)
4. Please also add a news entry to the topfiles directory. (ReviewProcess#Newsfiles)

It'd be great to hear that the unit test also passes on AIX before giving the ok to merge this (and if the unsigned char * thing changes at all, it'd be great to re-confirm that it still works on AIX).

Thanks!

[spiv]

1. char * would probably make more sense, except for the is_hexdigit function. It relies on an array defined as unsigned char hexdigits[256], and the unsigned chars are used as indices. There's probably a more elegant solution that avoids the ugly cast, but a trivial change to char * in unquote would simply transfer the ugly cast to is_hexdigit. Given that warnings are gone and this appears to be functioning correctly (tests pass), I think it's ok as is. I'll happily review patches to improve this, though :)
2. I filed #4162 about the misplaced test (or misplaced module?).
3. Yes, noted for future. I'll happily take the burden of doing the merge for this one.
4. I've added an entry with this text: “The optional _c_urlarg extension now handles unquote("") correctly on platforms where malloc(0) returns NULL, such as AIX. It also compiles with less warnings.”

[spiv]

(In [27898]) Merge portable-c-4142: Make _c_urlarg work on AIX.

Author: spiv Reviewer: exarkun Fixes: #4142

The optional _c_urlarg extension now handles unquote("") correctly on platforms where malloc(0) returns NULL, such as AIX. It also compiles with less warnings on all platforms.