> 
> My previous email was sent too early.
> 

Oops, didn't see this message before posting my last message. :)

> #149 I'm fine without a warning.
> 
> #156  Great catch.  I hadn't noticed that PKCS12_parse()
> would free the CA list, but not NULL it.  For shame!
> 
> #157  Okay.   Error paths are so hard to check.
> A friend in a large company (that incidentally
> uses a *lot* of python) says the culture there
> is to always call exit() on errors.  You can
> log a message first if you like, but don't bother
> wasting code on an error path which is futile
> to get right.
> 

Luckily for me, you basically provided the test case for this error path. :)  I probably wouldn't have noticed these problems if not for the commented-out attempt to load that MAC-less PKCS12 blob in one of the tests you wrote.  From there, it wasn't too hard to figure out what the fix was.  Of course, since the OpenSSL API is so bleh, there may be some other case that's still not tested and doesn't work right...  If I were going to pursue a rigorous testing policy for this code, I'd probably start by collecting a lot of PKCS12 data from a variety of sources and run them all through these pyOpenSSL APIs as part of the automated test suite.  Off the top of my head, I'm not sure where to go to find such data, though (I guess web browsers can spit it out somehow?  Or do they only load it?).

I think the current level of test coverage is good, even if it's not perfect.  Problems discovered later can be fixed later. :)

I'm sort of a fan of exiting immediately on errors, at least in certain circumstances.  I'd feel really bad putting that into a Python library, since I don't think most Python programmers expect libraries to do that.  Maybe it's actually worth doing for error paths that we're not able to construct tests for.  If people run into them in practice, that behavior would be a good motivation for them to construct a test case and contribute it back to the project. :)  For the time being though, I'll probably leave things as-is.


> --
> Rick
> 
> On Sun, Jul 26, 2009 at 01:57:57AM -0000, Jean-Paul Calderone wrote:
> > >
> > > Excellent points!  Thanks for the lesson.  I pushed all five
> > > fixes to lp:~rick-fdd/pyopenssl/pkcs12_mod_and_export2
> > >
> >
> > Awesome, thanks for all your work on this. :)  I made a bunch of cosmetic
> changes (I should put
> > together a style guide for pyOpenSSL, but I'm still not sure what it would
> say), but notices a
> > couple more actual potential issues, which I also tried to tackle.  My
> changes are in a branch
> > based on yours, <lp:~exarkun/pyopenssl/pkcs12_mod_and_export2>.  The
> revisions which actually
> > contain interesting changes are:
> >
> >   http://bazaar.launchpad.net/~exarkun/pyopenssl/pkcs12_mod_and_export2/revi
> sion/149
> >   http://bazaar.launchpad.net/~exarkun/pyopenssl/pkcs12_mod_and_export2/revi
> sion/156
> >   http://bazaar.launchpad.net/~exarkun/pyopenssl/pkcs12_mod_and_export2/revi
> sion/157
> >
> > I think I'm happy with the code now, so if you think these changes are
> sensible, I think I'll go ahead and merge them into trunk.
> >
> > Jean-Paul
> >
> > --
> > https://code.launchpad.net/~rick-
> fdd/pyopenssl/pkcs12_mod_and_export2/+merge/8962
> > You are the owner of lp:~rick-fdd/pyopenssl/pkcs12_mod_and_export2.
> 
> --
> Rick