Code review comment for lp://staging/~rick-fdd/pyopenssl/pkcs12_mod_and_export2

I should have mentioned earlier that there is someone who recently showed up on the mailing list (over on sourceforge) who is also a bit interested in this area (although he says he's more keen on CRLs than PKCS12). He's pushed a branch, <lp:~phil-mayers/pyopenssl/crl+morepkcs12>, which has some overlap with this one. I mostly prefer the PKCS12 code in this branch. I particularly like the documentation additions and many of the test suite additions. Here are some areas I think could be improved, though:

  1. crypto_PKCS12_set_certificate documents its return type as an X509 object. This looks like a copy/paste mistake, I guess. The function does return self, the PKCS12 instance. It should probably return None, though, I think.
  2. There's a left over C99-style comment near the definition of crypto_PKCS12_get_privatekey which should likely just be deleted.
  3. crypto_PKCS12_set_privatekey has one of the same problems as crypto_PKCS12_set_certificate with respect to its return value.
  4. crypto_PKCS12_set_ca_certificates does as well.
  5. Hm. The way PKCS12 handles its CA certificates list has a hole. In trunk, crypto_PKCS12_New always creates cacerts, and makes it a tuple. This is good, since get_ca_certificates will return it directly, and if it were a list, callers could modify it in ways which would probably result in a crash later. With this branch, they can pass a list to set_ca_certificates. It might be valid at the time they pass it in, but then it can be mutated later. Requiring a tuple in crypto_PKCS12_set_ca_certificates instead of any sequence is probably a good idea.
  6. All the code handling the key, cert, and cacerts fields of crypto_PKCS12Obj structures would probably be simpler if only one of NULL or Py_None were possible values. I think that may really already be the case anyway. crypto_PKCS12_New starts of setting key and cert to NULL, but then immediately either sets them to a PyObject* or goes to the error case. What happens when the NULL checks for key, cert, and cacerts are removed, and just the Py_None checks are left in place (where necessary)?
  7. Regarding int vs Py_ssize_t, we should probably put a version-protected typedef into util.h or someplace, so we can spell this in a way that works with all supported versions and actually does the right thing on newer Pythons.
  8. Still in crypto_PKCS12_set_ca_certificates, PySequence_Length and PySequence_GetItem can have errors that should be checked for. I'm not sure if PyTuple_GetSize and PyTuple_GetItem (cf point 5) also can, maybe not.
  9. crypto_PKCS12_set_ca_certificates also returns self, should be None.
  10. Test method docstrings. I would prefer docstrings which describe some desired behavior which the test is verifying pyOpenSSL actually provides.
  11. test_export_and_load is too long. :) Any chance this can be broken into a few shorter pieces?

I kinda skimmed the export code, I'm too sleepy at this point to do it justice. I'll have another look soon. Thanks.