On Mon, 14 Feb 2005, Martin Paljak wrote:

>> I disagree. I know very little about these things, so even if the changes
>> themselves might be clear, I don't know what the purpose if the whole thing
>> is

> Well, people who know about ssl engines most probly know about dynamic
> engines too.

Yes, that might be so, but I'm pretty clueless about those things and I am the
maintainer of the (lib)curl sources! ;-)

> The patch is different because it tries to pay special attention to enable
> even _other_ crypto modules (namely, pkcs11 modules) via openssl engine
> provided by OpenSC project. There are way more different pkcs11 modules than
> openssl modules, i guess...
>
> And PKCS#11 should be pretty well known thing in crypto world.

If we add code and features to libcurl, you can be very sure that sooner or
later someone who does not have all the clues will come asking for info on
what it is and how to use it.

Then we need to have info and docs around that provide answers, as we can't
expect every single contributor to always be around to answer questions about
their particular source code contributions. I think I've ended up in that
corner a little too many times in the past, where I have had to dig up the
info and spend lots of time learning things I shouldn't need to, just because
I've happily applied patches to provide more features and please users. While
that is fine short-term, it often bounces back in my face long-term.

> You bet. i made the previous one at night and didn't pay much attention to
> what was going on. Actually it is totally bogus and worked only because....
> well, no comments.

:-)

> Attached is a proper patch for both library and user command, that actually
> does what it should do.

This was incomplete, as there was no lib/urldata.h patch in this. But see
below...

> Well, i just did a blind and fast 'grep engine' and added necessary things
> nearby (as that was the obvious place, imho). Feel free to adjust it to
> whatever curl-style ways you feel appropriate. As you can see, they are just
> parameters to be used when creating the engine, so they most probly don't
> have to be where one keeps session information.

Well they do need to be there for reasons I already stated. You did however
made me aware of the fact that the existing engine stuff is stored in the
wrong struct as well... Or possibly we should document that
curl_easy_duphandle() doesn't duplicate engine info. Not only because it
doesn't do this today but also because duplicating that data is not done by
simply memcpy()ing the data.

>> - +#define HAVE_ENGINE_LOAD_BUILTIN_ENGINES 1
>> This is clearly a mistake
>
> I couldn't find a place where this was defined and it is needed to make the
> dynamic engine appear in the first place.

Hm, it should be checked for in the configure script. Doing the #define there
is just wrong and won't work on many systems.

This couldn't be working already before your patch...!

>> - I see you strcmp() the engine name for "dynamic" to see if this is
>> activated. Isn't it better to rely on one of the options you
>> added that is bound to be set when this is used? Like SSLENGINE_ID. If not,
>> is it really sane and safe to just allocate an engine name to check for
>> this?

> dynamic is the ssl 'engine' that allows to load other, actual engines.
> SSLENGINE_ID shall be the ID that shall be given to the new, loaded engine.

Okay, then it seems like a good choice. Thanks for sharing this.

> As i said - it was much of a quick and dirty hack. Though i'd appreciate if
> you took some time to adjust it (as it aint that huge) to curlisms and apply
> to baseline.

I'll do that, if you provide a patch for docs/curl.1 and
docs/libcurl/curl_easy_setopt.1 as well! ;-)

And again: thanks for your contribution!

-- 
      Daniel Stenberg -- http://curl.haxx.se -- http://daniel.haxx.se
       Dedicated custom curl help for hire: http://haxx.se/curl.html