Reordering new API's that have a libctx, propq

[Richard Levitte]

On Mon, 14 Sep 2020 13:46:01 +0200,
Tim Hudson wrote:
> 
> [1  <text/plain; UTF-8 (7bit)>]
> [2  <text/html; UTF-8 (quoted-printable)>]
> On Mon, Sep 14, 2020 at 9:19 PM Matt Caswell <matt at openssl.org> wrote:
> 
>     I must be misunderstanding your point because I don't follow your logic
>     at all.
>    
>     So this is the correct form according to my proposed policy:
>    
>     TYPE *TYPE_make_from_ctx(char *name,TYPE_CTX *ctx)
> 
> And that is the point - this is not how the existing CTX functions
> work (ignoring the OPENSSL_CTX stuff).

I'd say that the example here is flawed, at least for libcrypto.

(libssl has some very different patterns that are quite confusing when
relating them to what's been said here, so I'm choosing to ignore them
for the moment, for the sake of clarity)

The prime example I have for that sort of constructor constructor is this:

    EVP_PKEY_CTX *EVP_PKEY_CTX_new_from_name(OPENSSL_CTX *libctx,
                                             const char *name,
                                             const char *propquery);

Function name wise, it's obvious that the name is of primary interest.
The library context is located as first argument, because "library
global state" or factory pool, which is fine.

However, other than SSL contra SSL_CTX, I can't recall that we have
much functions that contruct a TYPE from a TYPE_CTX.

( mind you, our TYPE naming is all over the place and quite inconsistent.
  In many EVP APIs, we have a TYPE / TYPE_CTX pair where TYPE is the
  structure with method pointers and TYPE_CTX is the structure with
  instance data...  except with EVP_PKEY, where we had (roughly and in
  legacy terms) the EVP_PKEY_ASN1_METHOD / EVP_PKEY pair and the
  EVP_PKEY_METHOD / EVP_PKEY_CTX pairs...  and with non EVP APIs (such
  as BIO), we often have the TYPE_METHOD / TYPE pair

  I haven't even begun to analyse the SSL types in similar terms, if
  that's doable )

>     > The PR should cover that context of how we name the variations of
>     > functions which add the OPENSSL_CTX reference.
>    
>     IMO, it does. They should be called "*_ex" as stated in chapter 6.2. I
>     don't see why we need to special case OPENSSL_CTX in the policy. Its
>     written in a generic way an can be applied to the OPENSSL_CTX case.
> 
> And that means renaming all the with_libctx to _ex which frankly I
> don't think makes a lot of sense to do.

I don't see a lot of sense in _with_libctx (and I scold myself for
starting that pattern).  Imagine the next time we have something
important that we want to add as argument to the same essential
function, what then?  TYPE_blah_with_libctx_with_whatever()?
That doesn't set a sensible pattern...

> Having a naming indicating OPENSSL_CTX added isn't a bad thing to do
> in my view.

Can you guarantee that this is the one and only time we add something
like '_with_libctx' to a function name, or do we risk ending up
feeling "inspired" some time in the future?

> Collecting a whole pile of _ex functions and having to look at each
> one to figure out what the "extension" is does add additional burden
> for the user.

A sensible thing to do is to deprecate the old and clunky.  That'll
tell the user which if variants A, B, and C they should use going
forward.

I'm with you insofar that I thoroughly dislike _ex, but I've made
peace with it when someone pointed out that _ex{n} is possible.
And quite frankly, I dislike more or less every scheme on giving new
names to functions that are just extended but otherwise have the same
essential functionality as the previous one.  They're all ugly.  So in
my mind mind, it comes down to choosing a scheme that one can live
with and that holds for a long time going forward.  And just so we're
clear, if you take a function and make a replacement that takes some
slightly different arguments but that essentially (i.e. perhaps not
exactly in the details, but in broader terms) gives the same result,
then I'd say that the new function is an extension of the old, not an
entirely new function.

I'll also point out that when we make functions that are "concurrent"
variants of the same, then it does make sense to have specific naming,
like these:

EVP_PKEY_CTX *EVP_PKEY_CTX_new_from_name(OPENSSL_CTX *libctx,
                                         const char *name,
                                         const char *propquery);
EVP_PKEY_CTX *EVP_PKEY_CTX_new_from_pkey(OPENSSL_CTX *libctx,
                                         EVP_PKEY *pkey, const char *propquery);

These are not replacements of each other, so it makes sense here to
put the distinguishing feature of each as part of the function name.

> There is a reason that _with_libctx was added rather than picking
> _ex as the function names - it clearly communicates then intent
> rather than be a generic extension-of-API naming.

Considering I came up with that, I can easily declare that the intent
wasn't originally as grand as you make it out to be.  It was a name I
came up with spur of the moment.  Today, I wish I never had, at least
in the public API (I'm less fussy about internal functions).  It was a
bad choice on my part.

>     IMO *the* most confusing thing would be to *change* an existing ordering
> 
>     (for example swap two existing parameters around). I see no problem with
>     adding new ones anywhere that's appropriate.
> 
> Clearly we disagree on that - if you are making an extended version
> of a function and you aren't keeping the same existing parameter
> order (which you are not if you add or remove parameters which is
> the point I was making - the order isn't the same as the existing
> function because you've removed items - what you have is the order
> of whatever parameters remain in the extended function are the same
> - and that's a pretty pointless distinction to make - if you aren't
> simply adding additional items on the end you make for a change over
> mess and this I think we should be trying to avoid).

I think we have some very fundamentaly different understanding of
"order".  I'm with Matt on this; if you have a list A, B, C, D and
remove C, then A, B, D are still in the same order.

As to where to add arguments, I would say that yes, it's good to have
a general guideline, but that it will have to be decided more
carefully on a per function basis.  For example, I wouldn't
necessarily add a property query string at the very end, if the
algorithm name or the object the algorithm name can be derived from
isn't there.  So, I would not at all want to see something like this
(this isn't what we have now, but with the idea that we always place
OPENSSL_CTX as first argument and other added arguments last, this
would be the result):

    OSSL_STORE_CTX *
    OSSL_STORE_open(OPENSSL_CTX *, const char *uri,
                    const UI_METHOD *ui_method, void *ui_data,
                    OSSL_STORE_post_process_info_fn post_process,
                    void *post_process_data, const char *propq);

Because the property query string is strongly associated with the
implementation name (which is derived from the URI in this case), the
more meaningful placement would be this:

    OSSL_STORE_CTX *
    OSSL_STORE_open(OPENSSL_CTX *, const char *uri, const char *propq,
                    const UI_METHOD *ui_method, void *ui_data,
                    OSSL_STORE_post_process_info_fn post_process,
                    void *post_process_data, const char *propq);

(that also follows the current _fetch function model)

> My context there is for the users of the existing API.
> It becomes especially problematic if you have type equivalence when
> the order is changed around so there are no compiler warnings if the
> user hasn't picked up on reordering of parameters.

That is indeed a problem, but I fail to see how removed arguments or
the location of added arguments matter much for this.  Unless we get
into +1 -1 situation and that a 'void *' just so happens to end up in
a place that had a different type that gets silently passed anyway, I
doubt that will be much of a problem.

Cheers,
Richard

-- 
Richard Levitte         levitte at openssl.org
OpenSSL Project         http://www.openssl.org/~levitte/