Hi David, Thank you for taking a look at the patch and providing such thorough feedback so quickly. I have done everything that you asked:
* Changed printf() calls to use msg(). The prints are not actually used right now, but they are a part of the debug code that I wrote for testing the certificate reading functions. They were actually tested in a separate application, but I thought I should keep them with the rest of the code in case the certificate lookup functionality needs to be debugged inside of openvpn at a later date. * Changed malloc calls to the gc_arena api. * Removed the XCode project and updated configure.ac and Makefile.am to properly build the changes. I was well aware that this is what needed to be done, I only included the XCode project in the first patch in the hopes that someone with XCode and Automake knowledge would take a look at it and give me some pointers. You were correct, though, it was not as bad as I thought it was going to be. The information you provided was very useful. * Removed the dummy1() function from keychain.c. Keychain.c/.h are based on cryptoapi.c/.h in the openvpn source tree. When editing the original file to work with the Keychain services libraries instead of the Windows Crypto API, I decided to change as little as possible, the theory being that what was there was approved and accepted. You will see a static dummy() function defined at the bottom of that file as well. I had no idea what it was there for, but I figured there was a reason for it so I left it alone. I think I tacked on the 1 to the end without thinking too hard because I was being very defensive and trying to prevent a scope problem, which was silly of me because the function is static and you would never compile Windows-only and Mac-only code at the same time. If you grep for "dummy" in the source tree, you will see a number of places where this paradigm is used. Perhaps because some compilers will not compile a file that has had all of its code ifdef'd out? I tested this theory on my mac and no error occurred, so I went ahead and removed the function per your request. I should also point out that cryptoapi.c makes calls to calloc, which you may want to change at some point if you are trying to standardize on the gc_arena API. Please let me know if you have any questions or if more changes are required. Thanks, Brian Raderman
osxkeychain.patch
Description: Binary data
On Apr 28, 2010, at 2:53 AM, David Sommerseth wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi Brian,
>
> Thanks a lot for your patch! This is indeed a neat feature we want to
> really consider for inclusion! But there are of course a few things I
> spotted which I'd like to see being improved.
>
> I am not familiar with OSX details at all, but I hope someone else with
> better OSX understanding can review those parts more thoroughly. My
> comments will be of the more generic type.
>
> * usage of printf()
> Please use the msg() function instead. Don't print things to console
> directly via {f,}printf() directly. There are plenty of places in the
> code where you can see msg() being used for a lot of logging.
>
> * malloc()
> You use malloc() several places. If possible try to make use of the
> gc_arena API instead. This API is also used a lot of places in the
> code, so you'll most likely find some good examples there as well.
>
> * static void dummy1() function
> In osx/keychain.c you define a rather silly function, which is not used
> anywhere. Please take this one out.
>
> * xcode files and migration to autotools
> Even though Xcode is probably a very good tool on OSX, it's not what we
> can base OpenVPN on. I would like to see this migrated over to
> autotools. It's not as hard and difficult as you might think
> immediately, but there are several traps on the way. Some traps more
> visible than others.
>
> First of all, it's a pretty good autotools tutorial [1] which is worth
> going through to understand the concept and how to get started.
>
> Then you'll need to modify configure.ac and Makefile.am. And that's
> basically it. Have a look for SOCKS in configure.ac to get an idea.
> Further, you'll also need to add the source files of our osx/ C and H
> files to openvpn_SOURCES. But! These files should probably be depending
> on if this OSX feature is enabled or not. There's probably several ways
> how to solve this, and I'm not sure what's the best solution right now.
> But digging into automake docs[2] might be the proper place to start.
>
> When you think you're done, ./configure works and it seems to compile
> flawlessly by running 'make', try runnning 'make distcheck' to be sure
> it will package itself properly, that the test compilation and test run
> works as expected as well.
>
>
> That's basically all I had to comment so far. But thanks again for your
> patch! Looking forward to hear from you again with further updates as well.
>
>
> kind regards,
>
> David Sommerseth
>
>
> [1] <http://www.lrde.epita.fr/~adl/autotools.html>
> [2] <http://sources.redhat.com/automake/automake.html>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkvX23gACgkQDC186MBRfroWXgCfeZUA1pFrSsu55MMk3gg9TIcJ
> M0EAoKO/OYi2v5YMNDankep5vqMEX7hC
> =sXAB
> -----END PGP SIGNATURE-----
