Reordering new API's that have a libctx, propq

[Matt Caswell]

On 14/09/2020 11:52, Tim Hudson wrote:
> Any proposal needs to deal with the constructors consistently - whether
> they come from an OPENSSL_CTX or they come from an existing TYPE_CTX.
> That is absent in your PR.
> 
> Basically this leads to the ability to provide inconsistent argument
> order in functions.
> 
> TYPE *TYPE_make_from_ctx(TYPE_CTX *ctx, char *name)
> or
> TYPE *TYPE_make_from_ctx(char *name,TYPE_CTX *ctx)
> 
> It simply isn't consistent to basically allow both forms of this approach.
> 
> Seeing the OPENSSL_CTX as something different to the other APIs in terms
> of its usage when it is providing the context from which something is
> constructed is the underlying issue here.
> Your PR basically makes rules for "context" arguments which lead to
> allowing both the above forms - and making the current usage of CTX
> values a different logical order than the OPENSSL_CTX.

I must be misunderstanding your point because I don't follow your logic
at all.

In your example above the proposal seems clear (to me):

"Where present the TYPE argument should always be first. In constructors
or object factory type functions (such as a "fetch" function), the key
argument that identifies the thing be constructed should be placed first
(if such an argument exists)."

Your example is a "factory type" or constructor. Therefore the argument
that identifies the thing being constructued should be first, i.e. the name.

The section of the policy says:

"The remaining argument list should be put into order of importance."

Only then does it go on to list where the ctx sits in that order of
importance. The phrase "the remaining argument list" indicates that that
ordering is subordinate to the preceding paragraph.

So this is the correct form according to my proposed policy:

TYPE *TYPE_make_from_ctx(char *name,TYPE_CTX *ctx)


> 
> Separately, we should have consistency in the naming of the functions
> which take an OPENSSL_CTX - the _with_libctx makes no sense now that we
> settled on OPENSSL_CTX rather than OPENSSL_LIBCTX or OPENSSL_LIB_CTX as
> the naming. We also have a pile of _ex functions that were introduced
> just to add an OPENSSL_CTX - those should be consistently named. 

The policy is clear. We are adding extended forms of existing functions.
Therefore "_with_libctx" is not consistent with the policy. "_ex" (or
"_ex2" if necessary), is.


> 
> 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.

> 
> The suggested rules for extended functions is inconsistent in stating
> the order "should be retained" and then allowing for inserting "at any
> point".

This is not inconsistent. The *order* must be retained. Inserting
additional arguments does *not* change the order. So according to the
policy, if you have a function:

FOO_do_something(bar, wibble, blah);

Then this is ok:

FOO_do_something(new, bar, wibble, blah);

As is this:

FOO_do_something(bar, new, wibble, blah);

And this:

FOO_do_something(bar, wibble, blah, new);

But *not* this (because the order of existing arguments is different):

FOO_do_something(blah, wibble, bar, new);



> IHMO either the new function must preserve the existing order and just
> add to the end (to easy migration) or it should conform to the naming
> scheme and parameter argument order for new functions - one of those
> should be the driver rather than basically creating something that is
> neither - not easy to migrate to and also not following the documented
> order. We should be trying to achieve one of those two objectives.

I disagree that that should be an objective.

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.

Matt