Re: Reordering new API's that have a libctx, propq

[Matt Caswell]

On 21/09/2020 10:59, Matt Caswell wrote:
> 
> 
> On 16/09/2020 16:56, Matt Caswell wrote:
>>> "Adopt the coding style policy on function arguments as shown in chapter
>>> 6.1 of web PR 194 (commit 7b45b46d71f)"
> 
> This vote failed:
> 
> accepted:  no  (for: 2, against: 5, abstained: 2, not voted: 2)
> 
>>>
>>> "Adopt the coding style policy on extending existing functions as shown
>>> in chapter 6.2 of web PR 194 (commit 7b45b46d71f)"
> 
> 
> This vote is still in progress.

This vote has now also closed. It was passed:

accepted:  yes  (for: 5, against: 3, abstained: 2, not voted: 1)
> 
> Matt
> 
> 
>>>
>>> In the event that one vote passes but the other vote does not I will
>>> remove the section of text that did not pass from the PR and adjust
>>> chapter numbers accordingly.
>>>
>>> Matt
>>>

 Since we're not yet fully in agreement some compromises will have to be
 made. I hope I've come up with something which isn't too abhorrent to
 anyone.

 Please take a look.

 Matt


 On 05/09/2020 04:48, SHANE LONTIS wrote:
>
>   PR #12778 reorders all the API’s of the form:
>
>
>   EVP_XX_fetch(libctx, algname, propq) 
>
>
>   So that the algorithm name appears first.. 
>
>
>   e.g: EVP_MD_fetch(digestname, libctx, propq);
>
> This now logically reads as 'search for this algorithm using these
> parameters’.
>
> The libctx, propq should always appear together as a pair of parameters.
> There are only a few places where only the libctx is needed, which means
> that if there is no propq it is likely to cause a bug in a fetch at some
> point. 
>
> This keeps the API’s more consistent with other existing XXX_with_libctx
> API’s that put the libctx, propq at the end of the parameter list..
> The exception to this rule is that callback(s) and their arguments occur
> after the libctx, propq..
>
> e.g:
> typedef OSSL_STORE_LOADER_CTX *(*OSSL_STORE_open_with_libctx_fn)
>     (const OSSL_STORE_LOADER *loader,
>      const char *uri, OPENSSL_CTX *libctx, const char *propq,
>      const UI_METHOD *ui_method, void *ui_data);
>
> An otc_hold was put on this.. Do we need to have a formal vote?
> This really needs to be sorted out soon so the API’s can be locked down
> for beta.
>
> 
> Also discussed in a weekly meeting and numerous PR discussions was the
> removal of the long winded API’s ending with ‘with_libctx’
> e.g CMS_data_create_with_libctx
> The proposed renaming for this was to continue with the _ex notation..
> If there is an existing _ex name then it becomes _ex2, etc.
> There will most likely be additional parameters in the future for some
> API’s, so this notation would be more consistent with current API’s.
> Does this also need a vote?
>
> Regards,
> Shane
>
>

Re: Reordering new API's that have a libctx, propq

[Matt Caswell]

On 16/09/2020 16:56, Matt Caswell wrote:
>> "Adopt the coding style policy on function arguments as shown in chapter
>> 6.1 of web PR 194 (commit 7b45b46d71f)"

This vote failed:

accepted:  no  (for: 2, against: 5, abstained: 2, not voted: 2)

>>
>> "Adopt the coding style policy on extending existing functions as shown
>> in chapter 6.2 of web PR 194 (commit 7b45b46d71f)"


This vote is still in progress.

Matt


>>
>> In the event that one vote passes but the other vote does not I will
>> remove the section of text that did not pass from the PR and adjust
>> chapter numbers accordingly.
>>
>> Matt
>>
>>>
>>> Since we're not yet fully in agreement some compromises will have to be
>>> made. I hope I've come up with something which isn't too abhorrent to
>>> anyone.
>>>
>>> Please take a look.
>>>
>>> Matt
>>>
>>>
>>> On 05/09/2020 04:48, SHANE LONTIS wrote:

   PR #12778 reorders all the API’s of the form:


   EVP_XX_fetch(libctx, algname, propq) 


   So that the algorithm name appears first.. 


   e.g: EVP_MD_fetch(digestname, libctx, propq);

 This now logically reads as 'search for this algorithm using these
 parameters’.

 The libctx, propq should always appear together as a pair of parameters.
 There are only a few places where only the libctx is needed, which means
 that if there is no propq it is likely to cause a bug in a fetch at some
 point. 

 This keeps the API’s more consistent with other existing XXX_with_libctx
 API’s that put the libctx, propq at the end of the parameter list..
 The exception to this rule is that callback(s) and their arguments occur
 after the libctx, propq..

 e.g:
 typedef OSSL_STORE_LOADER_CTX *(*OSSL_STORE_open_with_libctx_fn)
     (const OSSL_STORE_LOADER *loader,
      const char *uri, OPENSSL_CTX *libctx, const char *propq,
      const UI_METHOD *ui_method, void *ui_data);

 An otc_hold was put on this.. Do we need to have a formal vote?
 This really needs to be sorted out soon so the API’s can be locked down
 for beta.

 
 Also discussed in a weekly meeting and numerous PR discussions was the
 removal of the long winded API’s ending with ‘with_libctx’
 e.g CMS_data_create_with_libctx
 The proposed renaming for this was to continue with the _ex notation..
 If there is an existing _ex name then it becomes _ex2, etc.
 There will most likely be additional parameters in the future for some
 API’s, so this notation would be more consistent with current API’s.
 Does this also need a vote?

 Regards,
 Shane

Re: Reordering new API's that have a libctx, propq

[Matt Caswell]

On 15/09/2020 12:25, Matt Caswell wrote:
> I plan to start two OTC votes on this tomorrow with the following wording:

These votes have now commenced. I'll report back with the results when
they are known.

Matt



> 
> "Adopt the coding style policy on function arguments as shown in chapter
> 6.1 of web PR 194 (commit 7b45b46d71f)"
> 
> "Adopt the coding style policy on extending existing functions as shown
> in chapter 6.2 of web PR 194 (commit 7b45b46d71f)"
> 
> In the event that one vote passes but the other vote does not I will
> remove the section of text that did not pass from the PR and adjust
> chapter numbers accordingly.
> 
> Matt
> 
>>
>> Since we're not yet fully in agreement some compromises will have to be
>> made. I hope I've come up with something which isn't too abhorrent to
>> anyone.
>>
>> Please take a look.
>>
>> Matt
>>
>>
>> On 05/09/2020 04:48, SHANE LONTIS wrote:
>>>
>>>   PR #12778 reorders all the API’s of the form:
>>>
>>>
>>>   EVP_XX_fetch(libctx, algname, propq) 
>>>
>>>
>>>   So that the algorithm name appears first.. 
>>>
>>>
>>>   e.g: EVP_MD_fetch(digestname, libctx, propq);
>>>
>>> This now logically reads as 'search for this algorithm using these
>>> parameters’.
>>>
>>> The libctx, propq should always appear together as a pair of parameters.
>>> There are only a few places where only the libctx is needed, which means
>>> that if there is no propq it is likely to cause a bug in a fetch at some
>>> point. 
>>>
>>> This keeps the API’s more consistent with other existing XXX_with_libctx
>>> API’s that put the libctx, propq at the end of the parameter list..
>>> The exception to this rule is that callback(s) and their arguments occur
>>> after the libctx, propq..
>>>
>>> e.g:
>>> typedef OSSL_STORE_LOADER_CTX *(*OSSL_STORE_open_with_libctx_fn)
>>>     (const OSSL_STORE_LOADER *loader,
>>>      const char *uri, OPENSSL_CTX *libctx, const char *propq,
>>>      const UI_METHOD *ui_method, void *ui_data);
>>>
>>> An otc_hold was put on this.. Do we need to have a formal vote?
>>> This really needs to be sorted out soon so the API’s can be locked down
>>> for beta.
>>>
>>> 
>>> Also discussed in a weekly meeting and numerous PR discussions was the
>>> removal of the long winded API’s ending with ‘with_libctx’
>>> e.g CMS_data_create_with_libctx
>>> The proposed renaming for this was to continue with the _ex notation..
>>> If there is an existing _ex name then it becomes _ex2, etc.
>>> There will most likely be additional parameters in the future for some
>>> API’s, so this notation would be more consistent with current API’s.
>>> Does this also need a vote?
>>>
>>> Regards,
>>> Shane
>>>
>>>

Re: Reordering new API's that have a libctx, propq

[Matt Caswell]

On 14/09/2020 11:30, Matt Caswell wrote:
> In order to try and move this discussion forward I have made a concrete
> proposal for how we should formulate the various ideas in this thread
> into an actual style. Please see here:
> 
> https://github.com/openssl/web/pull/194

I've updated this PR with some of the feedback received so far. Please
take another look.

I plan to start two OTC votes on this tomorrow with the following wording:

"Adopt the coding style policy on function arguments as shown in chapter
6.1 of web PR 194 (commit 7b45b46d71f)"

"Adopt the coding style policy on extending existing functions as shown
in chapter 6.2 of web PR 194 (commit 7b45b46d71f)"

In the event that one vote passes but the other vote does not I will
remove the section of text that did not pass from the PR and adjust
chapter numbers accordingly.

Matt

> 
> Since we're not yet fully in agreement some compromises will have to be
> made. I hope I've come up with something which isn't too abhorrent to
> anyone.
> 
> Please take a look.
> 
> Matt
> 
> 
> On 05/09/2020 04:48, SHANE LONTIS wrote:
>>
>>   PR #12778 reorders all the API’s of the form:
>>
>>
>>   EVP_XX_fetch(libctx, algname, propq) 
>>
>>
>>   So that the algorithm name appears first.. 
>>
>>
>>   e.g: EVP_MD_fetch(digestname, libctx, propq);
>>
>> This now logically reads as 'search for this algorithm using these
>> parameters’.
>>
>> The libctx, propq should always appear together as a pair of parameters.
>> There are only a few places where only the libctx is needed, which means
>> that if there is no propq it is likely to cause a bug in a fetch at some
>> point. 
>>
>> This keeps the API’s more consistent with other existing XXX_with_libctx
>> API’s that put the libctx, propq at the end of the parameter list..
>> The exception to this rule is that callback(s) and their arguments occur
>> after the libctx, propq..
>>
>> e.g:
>> typedef OSSL_STORE_LOADER_CTX *(*OSSL_STORE_open_with_libctx_fn)
>>     (const OSSL_STORE_LOADER *loader,
>>      const char *uri, OPENSSL_CTX *libctx, const char *propq,
>>      const UI_METHOD *ui_method, void *ui_data);
>>
>> An otc_hold was put on this.. Do we need to have a formal vote?
>> This really needs to be sorted out soon so the API’s can be locked down
>> for beta.
>>
>> 
>> Also discussed in a weekly meeting and numerous PR discussions was the
>> removal of the long winded API’s ending with ‘with_libctx’
>> e.g CMS_data_create_with_libctx
>> The proposed renaming for this was to continue with the _ex notation..
>> If there is an existing _ex name then it becomes _ex2, etc.
>> There will most likely be additional parameters in the future for some
>> API’s, so this notation would be more consistent with current API’s.
>> Does this also need a vote?
>>
>> Regards,
>> Shane
>>
>>

Re: Reordering new API's that have a libctx, propq

[Richard Levitte]

On Mon, 14 Sep 2020 13:46:01 +0200,
Tim Hudson wrote:
> 
> [1  ]
> [2  ]
> On Mon, Sep 14, 2020 at 9:19 PM Matt Caswell  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 

Re: Reordering new API's that have a libctx, propq

[Matt Caswell]

On 14/09/2020 13:14, Tim Hudson wrote:
> On Mon, Sep 14, 2020 at 9:52 PM Matt Caswell  > wrote:
> 
> > And that is the point - this is not how the existing CTX functions
> work
> > (ignoring the OPENSSL_CTX stuff).
> 
> Do you have some concrete examples of existing functions that don't work
> this way?
> 
> 
> SSL_new()
> BIO_new_ssl()
> BIO_new_ssl_connect()

All of these three are consistent with my proposed policy.

The policy says:

"the key argument that identifies the thing be constructed should be
placed first (if such an argument exists)"

The parenthetical comes into play here "if such an argument exists". It
doesn't exist in any of these three cases and therefore this part
doesn't apply. In all cases the arguments are a single argument and so
trivially agree with the ordering rules.


> BIO_new_bio_pair()

This one is a slight oddity in that it doesn't create just one object
but two. I'm not aware of anywhere else where we do this, but even if we
do I don't think it is a common pattern, so I don't feel the need to
base our future policy on this precedent.


> etc
> 
> And all the existing method using functions which are also factories.
> 
> But I get the point - if there is only one argument is it logically
> coming first or last - obviously it can be seen both ways.
> 
> IMO, we absolutely MUST have the ability to delete parameters (for
> example ENGINEs). If we can do that, then I don't see why we can't add
> parameters.
> 
> 
> No - that doesn't follow. It is perfectly reasonable to have an ENGINE
> typedef that remains and is passed as NULL as usual - and in fact most
> of the top-level ENGINE stuff handles NULL as meaning no engine usage so
> that would remain consistent. There is no absolute requirement to
> delete a parameter for this or other purposes. If you want to reorder
> parameters I would argue it should be a new function name and not an _ex
> version.

I disagree with you on this.

Matt

Re: Reordering new API's that have a libctx, propq

[Tim Hudson]

On Mon, Sep 14, 2020 at 9:52 PM Matt Caswell  wrote:

> > And that is the point - this is not how the existing CTX functions work
> > (ignoring the OPENSSL_CTX stuff).
>
> Do you have some concrete examples of existing functions that don't work
> this way?
>

SSL_new()
BIO_new_ssl()
BIO_new_ssl_connect()
BIO_new_bio_pair()
etc

And all the existing method using functions which are also factories.

But I get the point - if there is only one argument is it logically coming
first or last - obviously it can be seen both ways.

IMO, we absolutely MUST have the ability to delete parameters (for
> example ENGINEs). If we can do that, then I don't see why we can't add
> parameters.
>

No - that doesn't follow. It is perfectly reasonable to have an ENGINE
typedef that remains and is passed as NULL as usual - and in fact most of
the top-level ENGINE stuff handles NULL as meaning no engine usage so that
would remain consistent. There is no absolute requirement to delete a
parameter for this or other purposes. If you want to reorder parameters I
would argue it should be a new function name and not an _ex version.

Tim.

Re: Reordering new API's that have a libctx, propq

[Matt Caswell]

On 14/09/2020 12:46, Tim Hudson wrote:
> On Mon, Sep 14, 2020 at 9:19 PM Matt Caswell  > 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).

Do you have some concrete examples of existing functions that don't work
this way?


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

My understanding of the current consensus from previous discussions is
that most people feel differently to you on this. Maybe I'm wrong.


> Having a naming indicating OPENSSL_CTX added isn't a bad thing to do in
> my view.
> 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.
> 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.
> 
> 
> 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). 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.

IMO, we absolutely MUST have the ability to delete parameters (for
example ENGINEs). If we can do that, then I don't see why we can't add
parameters.

Matt
 

Re: Reordering new API's that have a libctx, propq

[Matt Caswell]

On 14/09/2020 11:30, Matt Caswell wrote:
> In order to try and move this discussion forward I have made a concrete
> proposal for how we should formulate the various ideas in this thread
> into an actual style. Please see here:
> 
> https://github.com/openssl/web/pull/194
> 
> Since we're not yet fully in agreement some compromises will have to be
> made. I hope I've come up with something which isn't too abhorrent to
> anyone.
> 
> Please take a look.

I'm planning to gather feedback on this PR today - so if you have any
thoughts then please comment. I will post an updated version tomorrow
incorporating any feedback. I plan to start a vote on this later this
week (perhaps Wednesday).

Possibly I might do two votes: one for the proposed chapter 6.1, and one
for the proposed chapter 6.2.

Given the disagreements on this, I don't believe unanimous agreement is
going to be achievable. I hope to get it into a state where it is
acceptable to as many people as possible.

Matt


> 
> Matt
> 
> 
> On 05/09/2020 04:48, SHANE LONTIS wrote:
>>
>>   PR #12778 reorders all the API’s of the form:
>>
>>
>>   EVP_XX_fetch(libctx, algname, propq) 
>>
>>
>>   So that the algorithm name appears first.. 
>>
>>
>>   e.g: EVP_MD_fetch(digestname, libctx, propq);
>>
>> This now logically reads as 'search for this algorithm using these
>> parameters’.
>>
>> The libctx, propq should always appear together as a pair of parameters.
>> There are only a few places where only the libctx is needed, which means
>> that if there is no propq it is likely to cause a bug in a fetch at some
>> point. 
>>
>> This keeps the API’s more consistent with other existing XXX_with_libctx
>> API’s that put the libctx, propq at the end of the parameter list..
>> The exception to this rule is that callback(s) and their arguments occur
>> after the libctx, propq..
>>
>> e.g:
>> typedef OSSL_STORE_LOADER_CTX *(*OSSL_STORE_open_with_libctx_fn)
>>     (const OSSL_STORE_LOADER *loader,
>>      const char *uri, OPENSSL_CTX *libctx, const char *propq,
>>      const UI_METHOD *ui_method, void *ui_data);
>>
>> An otc_hold was put on this.. Do we need to have a formal vote?
>> This really needs to be sorted out soon so the API’s can be locked down
>> for beta.
>>
>> 
>> Also discussed in a weekly meeting and numerous PR discussions was the
>> removal of the long winded API’s ending with ‘with_libctx’
>> e.g CMS_data_create_with_libctx
>> The proposed renaming for this was to continue with the _ex notation..
>> If there is an existing _ex name then it becomes _ex2, etc.
>> There will most likely be additional parameters in the future for some
>> API’s, so this notation would be more consistent with current API’s.
>> Does this also need a vote?
>>
>> Regards,
>> Shane
>>
>>
> 

Re: Reordering new API's that have a libctx, propq

[Tim Hudson]

On Mon, Sep 14, 2020 at 9:19 PM Matt Caswell  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).


> > 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.
Having a naming indicating OPENSSL_CTX added isn't a bad thing to do in my
view.
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.
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.


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

Tim.

Re: 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

Re: Reordering new API's that have a libctx, propq

[Tim Hudson]

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.

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 PR should cover that context of how we name the variations of functions
which add the OPENSSL_CTX reference.

The suggested rules for extended functions is inconsistent in stating the
order "should be retained" and then allowing for inserting "at any point".
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.

Tim.


On Mon, Sep 14, 2020 at 8:30 PM Matt Caswell  wrote:

> In order to try and move this discussion forward I have made a concrete
> proposal for how we should formulate the various ideas in this thread
> into an actual style. Please see here:
>
> https://github.com/openssl/web/pull/194
>
> Since we're not yet fully in agreement some compromises will have to be
> made. I hope I've come up with something which isn't too abhorrent to
> anyone.
>
> Please take a look.
>
> Matt
>
>
> On 05/09/2020 04:48, SHANE LONTIS wrote:
> >
> >   PR #12778 reorders all the API’s of the form:
> >
> >
> >   EVP_XX_fetch(libctx, algname, propq)
> >
> >
> >   So that the algorithm name appears first..
> >
> >
> >   e.g: EVP_MD_fetch(digestname, libctx, propq);
> >
> > This now logically reads as 'search for this algorithm using these
> > parameters’.
> >
> > The libctx, propq should always appear together as a pair of parameters.
> > There are only a few places where only the libctx is needed, which means
> > that if there is no propq it is likely to cause a bug in a fetch at some
> > point.
> >
> > This keeps the API’s more consistent with other existing XXX_with_libctx
> > API’s that put the libctx, propq at the end of the parameter list..
> > The exception to this rule is that callback(s) and their arguments occur
> > after the libctx, propq..
> >
> > e.g:
> > typedef OSSL_STORE_LOADER_CTX *(*OSSL_STORE_open_with_libctx_fn)
> > (const OSSL_STORE_LOADER *loader,
> >  const char *uri, OPENSSL_CTX *libctx, const char *propq,
> >  const UI_METHOD *ui_method, void *ui_data);
> >
> > An otc_hold was put on this.. Do we need to have a formal vote?
> > This really needs to be sorted out soon so the API’s can be locked down
> > for beta.
> >
> > 
> > Also discussed in a weekly meeting and numerous PR discussions was the
> > removal of the long winded API’s ending with ‘with_libctx’
> > e.g CMS_data_create_with_libctx
> > The proposed renaming for this was to continue with the _ex notation..
> > If there is an existing _ex name then it becomes _ex2, etc.
> > There will most likely be additional parameters in the future for some
> > API’s, so this notation would be more consistent with current API’s.
> > Does this also need a vote?
> >
> > Regards,
> > Shane
> >
> >
>

Re: Reordering new API's that have a libctx, propq

[Matt Caswell]

In order to try and move this discussion forward I have made a concrete
proposal for how we should formulate the various ideas in this thread
into an actual style. Please see here:

https://github.com/openssl/web/pull/194

Since we're not yet fully in agreement some compromises will have to be
made. I hope I've come up with something which isn't too abhorrent to
anyone.

Please take a look.

Matt


On 05/09/2020 04:48, SHANE LONTIS wrote:
> 
>   PR #12778 reorders all the API’s of the form:
> 
> 
>   EVP_XX_fetch(libctx, algname, propq) 
> 
> 
>   So that the algorithm name appears first.. 
> 
> 
>   e.g: EVP_MD_fetch(digestname, libctx, propq);
> 
> This now logically reads as 'search for this algorithm using these
> parameters’.
> 
> The libctx, propq should always appear together as a pair of parameters.
> There are only a few places where only the libctx is needed, which means
> that if there is no propq it is likely to cause a bug in a fetch at some
> point. 
> 
> This keeps the API’s more consistent with other existing XXX_with_libctx
> API’s that put the libctx, propq at the end of the parameter list..
> The exception to this rule is that callback(s) and their arguments occur
> after the libctx, propq..
> 
> e.g:
> typedef OSSL_STORE_LOADER_CTX *(*OSSL_STORE_open_with_libctx_fn)
>     (const OSSL_STORE_LOADER *loader,
>      const char *uri, OPENSSL_CTX *libctx, const char *propq,
>      const UI_METHOD *ui_method, void *ui_data);
> 
> An otc_hold was put on this.. Do we need to have a formal vote?
> This really needs to be sorted out soon so the API’s can be locked down
> for beta.
> 
> 
> Also discussed in a weekly meeting and numerous PR discussions was the
> removal of the long winded API’s ending with ‘with_libctx’
> e.g CMS_data_create_with_libctx
> The proposed renaming for this was to continue with the _ex notation..
> If there is an existing _ex name then it becomes _ex2, etc.
> There will most likely be additional parameters in the future for some
> API’s, so this notation would be more consistent with current API’s.
> Does this also need a vote?
> 
> Regards,
> Shane
> 
> 

Re: Reordering new API's that have a libctx, propq

[Michael Richardson]

Richard Levitte  wrote:
> There are many red herrings in here, and I would argue that trying to
> be too uniform in the way you think about all functions may be
> harmful, because not all functions are classified the same.

> We cannot deny that many of our interfaces have an OOP flair.  We may
> be programming in C, but we very obviously have that kind of pattern,
> a "poor man's OOP" in C.  So speaking about our interfaces in OOP
> terms is not far fetched, as long as we keep in mind that we can only
> take the argument so far.

It would be very nice if there was object-focused (rather than "oriented")
documentation for OpenSSL.
Tell me what all the structures/typedefs are, and then tell me what things 
operate on them.
(I tried to start documentation like that for the CMS parts, but I got 
distracted.)

--
]   Never tell me the odds! | ipv6 mesh networks [
]   Michael Richardson, Sandelman Software Works|IoT architect   [
] m...@sandelman.ca  http://www.sandelman.ca/|   ruby on rails[



signature.asc
Description: PGP signature

Re: Reordering new API's that have a libctx, propq

[Dr Paul Dale]

Still no need for the added complexity:

Push:
OPENSSL_CTX *prevctx = OPENSSL_CTX_set0_default(libctx);

Pop is:
OPENSSL_CTX_set0_default(prevctx);

Push before callback:
OPENSSL_CTX_set0_default(prevctx);

Pop after callback:
prevctx = OPENSSL_CTX_set0_default(libctx);
or
OPENSSL_CTX_set0_default(libctx);
Depending if we want to support call backs changing the Libctx directly — we 
should choose one and always recommend that.


Also the auto allocation of storage for the second on stack cannot fail, so no 
error checking spaghetti.


Pauli
-- 
Dr Paul Dale | Distinguished Architect | Cryptographic Foundations 
Phone +61 7 3031 7217
Oracle Australia




> On 10 Sep 2020, at 12:08 am, Tomas Mraz  wrote:
> 
> On Wed, 2020-09-09 at 22:29 +1000, Dr Paul Dale wrote:
>>> On 9 Sep 2020, at 9:38 pm, Tomas Mraz  wrote:
>>> 
>>> We could even provide a convenience thread local stack of lib
>>> contexts
>>> so the caller would not have to keep the old value but would just
>>> push
>>> the new libctx when entering and pop the old one when leaving. With
>>> that, I think the changes needed in the application code would be
>>> fairly simple and minimal.
>> 
>> Let’s not overcomplicate things.
>> We went through this discussion back when this was introduced.
>> 
>> 
>> Push is:
>>OPENSSL_CTX *prevctx = OPENSSL_CTX_set0_default(libctx);
>> 
>> Pop is:
>>OPENSSL_CTX_set0_default(prevctx)
>> 
>> 
>> I don’t see having an explicit stack of these is of any benefit to
>> anything but unwarranted complexity.
> 
> There is one thing where it would be IMO helpful - let's say libcurl
> has a callback into a calling application. With the current API in
> libcurl API calls you would put the
> calls OPENSSL_CTX_set0_default(libctx)/OPENSSL_CTX_set0_default(prevctx
> ) at the beginning and end. But you would have to save the prevctx
> somewhere in the libcurl context structure because on callbacks you
> would have to temoprarily reset the context to the prevctx value. If we
> implemented real stack it would not be needed. But yeah, I am not sure
> this convenience is that much worth it.
> 
> -- 
> Tomáš Mráz
> No matter how far down the wrong road you've gone, turn back.
>  Turkish proverb
> [You'll know whether the road is wrong if you carefully listen to your
> conscience.]

Re: Reordering new API's that have a libctx, propq

[Richard Levitte]

On Wed, 09 Sep 2020 16:08:10 +0200,
Tomas Mraz wrote:
> 
> On Wed, 2020-09-09 at 22:29 +1000, Dr Paul Dale wrote:
> > > On 9 Sep 2020, at 9:38 pm, Tomas Mraz  wrote:
> > > 
> > > We could even provide a convenience thread local stack of lib
> > > contexts
> > > so the caller would not have to keep the old value but would just
> > > push
> > > the new libctx when entering and pop the old one when leaving. With
> > > that, I think the changes needed in the application code would be
> > > fairly simple and minimal.
> > 
> > Let’s not overcomplicate things.
> > We went through this discussion back when this was introduced.
> > 
> > 
> > Push is:
> > OPENSSL_CTX *prevctx = OPENSSL_CTX_set0_default(libctx);
> > 
> > Pop is:
> > OPENSSL_CTX_set0_default(prevctx)
> > 
> > 
> > I don’t see having an explicit stack of these is of any benefit to
> > anything but unwarranted complexity.
> 
> There is one thing where it would be IMO helpful - let's say libcurl
> has a callback into a calling application. With the current API in
> libcurl API calls you would put the
> calls OPENSSL_CTX_set0_default(libctx)/OPENSSL_CTX_set0_default(prevctx
> ) at the beginning and end. But you would have to save the prevctx
> somewhere in the libcurl context structure because on callbacks you
> would have to temoprarily reset the context to the prevctx value. If we
> implemented real stack it would not be needed. But yeah, I am not sure
> this convenience is that much worth it.

A stack becomes potentially extra churn in memory allocation for every
push, and wondering what to do with the stack if the stack
functionality fails.  So yeah, I'm thinking that it's not really worth
it.

Cheers,
Richard

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

Re: Reordering new API's that have a libctx, propq

[Tomas Mraz]

On Wed, 2020-09-09 at 22:29 +1000, Dr Paul Dale wrote:
> > On 9 Sep 2020, at 9:38 pm, Tomas Mraz  wrote:
> > 
> > We could even provide a convenience thread local stack of lib
> > contexts
> > so the caller would not have to keep the old value but would just
> > push
> > the new libctx when entering and pop the old one when leaving. With
> > that, I think the changes needed in the application code would be
> > fairly simple and minimal.
> 
> Let’s not overcomplicate things.
> We went through this discussion back when this was introduced.
> 
> 
> Push is:
> OPENSSL_CTX *prevctx = OPENSSL_CTX_set0_default(libctx);
> 
> Pop is:
> OPENSSL_CTX_set0_default(prevctx)
> 
> 
> I don’t see having an explicit stack of these is of any benefit to
> anything but unwarranted complexity.

There is one thing where it would be IMO helpful - let's say libcurl
has a callback into a calling application. With the current API in
libcurl API calls you would put the
calls OPENSSL_CTX_set0_default(libctx)/OPENSSL_CTX_set0_default(prevctx
) at the beginning and end. But you would have to save the prevctx
somewhere in the libcurl context structure because on callbacks you
would have to temoprarily reset the context to the prevctx value. If we
implemented real stack it would not be needed. But yeah, I am not sure
this convenience is that much worth it.

-- 
Tomáš Mráz
No matter how far down the wrong road you've gone, turn back.
  Turkish proverb
[You'll know whether the road is wrong if you carefully listen to your
conscience.]

Re: Reordering new API's that have a libctx, propq

[Richard Levitte]

On Wed, 09 Sep 2020 13:38:42 +0200,
Tomas Mraz wrote:
> > Regarding model 3, it must be said that there is potential for
> > confusion
> > on what it's supposed to do, replace the default property query
> > string
> > (settable with EVP_set_default_properties()), or merge with it.
> > Remember that a property query string given through any XXX_fetch()
> > function is temporarly merged with the default properties when
> > looking
> > for fitting algorithms.
> 
> Here I would actually argue, that there should be another property
> query in the libctx in addition to the default, that would have the
> exactly same semantics as the propq parameter has now and for the
> functions with the propq parameter it even would not be applied. How to
> name it so it won't be confused with the default property query, that's
> hard to say though.

"current" was mentioned several times during yesterday's videocall,
that could actually be used for a name.

I'm ok with putting such a property query string into the library
context, As Long As there is the understanding that it's not *tied*
to that library context, i.e. it can be used to pass a property query
string on to a provider implementation, to be used with whatever
library context the provider uses (the FIPS module has its own, for
example).

Side note: the function OPENSSL_CTX_set0_default() is a confusing
name, as there's an internal default ("default default", "ultimate
default"?) library context already, it's possible that we should
rename that to OPENSSL_CTX_set0_current().

Cheers,
Richard

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

Re: Reordering new API's that have a libctx, propq

[Dr Paul Dale]

> On 9 Sep 2020, at 9:38 pm, Tomas Mraz  wrote:
> 
> We could even provide a convenience thread local stack of lib contexts
> so the caller would not have to keep the old value but would just push
> the new libctx when entering and pop the old one when leaving. With
> that, I think the changes needed in the application code would be
> fairly simple and minimal.

Let’s not overcomplicate things.
We went through this discussion back when this was introduced.


Push is:
OPENSSL_CTX *prevctx = OPENSSL_CTX_set0_default(libctx);

Pop is:
OPENSSL_CTX_set0_default(prevctx)


I don’t see having an explicit stack of these is of any benefit to anything but 
unwarranted complexity.



Pauli

Re: Reordering new API's that have a libctx, propq

[Tomas Mraz]

On Wed, 2020-09-09 at 11:41 +0200, Richard Levitte wrote:
> 
> Regarding the library context, when viewed as a global state, it
> makes
> sense to have it as a first argument where it's being passed, if at
> all.  The question is, where should we actually pass it?  We have a
> few different suggestions, in wording or in code, on the table:
> 
> 1.  Pass it as first argument everywhere.  This is problematic, as
> "everywhere" is a *lot*, and if we do this, we might as well
> re-design the whole libcrypto API [*], and that's definitely not
> what OpenSSL 3.0 is about, quite the contrary.
> 
> This has been partially done, with all the _with_libctx
> functions.  As far as I've understood, these have been added on
> need basis, i.e. somewhere down the code path, a library context
> or a property query string is needed.  I can't say if this gives
> any sense of completeness or consistency, viewed as an cohesive
> API, or if we stand any chance of getting there without a
> complete
> API re-design.
> 
> Something to be noted is that it doesn't make sense to pass the
> library context everywhere, because it's already part of other
> structures that are passed.  For example, any method structure,
> such as EVP_CIPHER, EVP_MD, etc are tied to a provider, which is
> in turn tied to a library context.  Passing another library
> context to anything that includes one of those method structures
> would only be confusing.
> 
> 2.  Pass it when constructing different structures, mostly other
> context structures.  As an example, EVP_PKEY_CTX_new_from_pkey()
> that I displayed above.  There may be cases where we need to pass
> it directly to functions that aren't constructors, but I expect
> those cases to be relatively few.
> 
> This has been done for some other structures as well, on an as
> needed basis:
> 
> X509 *X509_new_with_libctx(OPENSSL_CTX *libctx, const char
> *propq);
> 
> X509_STORE_CTX *X509_STORE_CTX_new_with_libctx(OPENSSL_CTX
> *libctx,
>const char
> *propq);
> 
> (the following are internal only)
> 
> int x509_set0_libctx(X509 *x, OPENSSL_CTX *libctx, const char
> *propq);
> int x509_crl_set0_libctx(X509_CRL *x, OPENSSL_CTX *libctx,
> const char *propq);
> 
> 3.  Set a current library context, a pointer in a thread local
> variable.  We already have support for that, which this function:
> 
> OPENSSL_CTX *OPENSSL_CTX_set0_default(OPENSSL_CTX *libctx);
> 
> The usage model is this:
> 
> OPENSSL_CTX *prevctx = OPENSSL_CTX_set0_default(libctx);
> 
> /* do stuff with |libctx| as implicit current global state */
> 
> OPENSSL_CTX_set0_default(prevctx)
> 
> Looking at that list now, I realise that it goes from most intrusive
> to least intrusive, viewed as a public API.
> 
> The third choice in particular would let any application or library
> just set their library context for the duration of the code that
> should be execute with that as a "global state", and restore it when
> done, leaving the rest of the OpenSSL calls untouched.

We could even provide a convenience thread local stack of lib contexts
so the caller would not have to keep the old value but would just push
the new libctx when entering and pop the old one when leaving. With
that, I think the changes needed in the application code would be
fairly simple and minimal.

> Something to be noted is that model 2 and 3 is possible to combine,
> which could give us a smoother transition between the current API and
> whatever we design going forward.

Although I have big sympathy with people that worked hard to add the
various _with_libctx() calls I would say that keeping the new
with_libctx variants would be too confusing. Especially for the reason
we would not have a complete set of them anyway. The exception might be
the low level EVP calls where we deal with the libctx directly and
where the algorithm fetched is actually associated with the context.

> 
> 
> Regarding the property query string, looking at it separate from the
> library context, the question remains where to put it, and a few
> proposals are on the table:
> 
> 1.  Put it last.
> 
> 2.  Put it next to the algorithm name or the object that an algorithm
> name is inferred from.
> 
> 3.  Set it "globally" with a thread local variable, a bit like what
> OPENSSL_CTX_set0_default() does for library contexts.
> 
> For this model, it's been argued if it should simply be stored in
> the current library context, thereby avoiding to add another
> thread local variable (which, for all intents and purposes, is
> another actually global thing to deal with, even though on
> per-thread level).
> 
> In my mind, model 2 would be more sensible than model 1, because of
> the implied tie between algorithm name and property 

Re: Reordering new API's that have a libctx, propq

[Richard Levitte]

A few of the more active developers currently have a videocall meeting
every tuesday, in the morning for us in Europe.

We talked about this issue yesterday, and realised quite a few things.
One pretty important thing to realise is that while many new functions
take a library context and a property query string that are located
together, that doesn't mean that they are the coupled pair that we
made them out to be, quite the contrary.

The library context has been called many things...  during the vidcall
yesterday, it was compared to "a global state", which could be a term
that we can agree with, since its primary function is to be a
collection of stuff that were previously global.

The property query string, on the other hand, is only interesting for
fetching implementations, so if it's to be conceptually coupled with
anything, it's with the name of the algorithm that's being fetched.
This is most visible when we pass a cipher or digest name to some
provider-side implementations; they also always take a property query
string (if we've missed a spot, please raise an issue).

Do note that there are places where we pass a property query string
without passing an algorithm name.  That happen when the algorithm
name is inferred from another object that's passed in.  For example:

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

This function would classically be used when you have a |pkey| with
just domain parameters, and you want to generate a key from it.  In
this case, the algorithm name is inferred from the |pkey|.

With this, it makes sense to conceptually decouple the library context
and the property query string, and to view the passing of them to
diverse functions in form of a pair as accidental.  We can thereby
talk about them separately.



Regarding the library context, when viewed as a global state, it makes
sense to have it as a first argument where it's being passed, if at
all.  The question is, where should we actually pass it?  We have a
few different suggestions, in wording or in code, on the table:

1.  Pass it as first argument everywhere.  This is problematic, as
"everywhere" is a *lot*, and if we do this, we might as well
re-design the whole libcrypto API [*], and that's definitely not
what OpenSSL 3.0 is about, quite the contrary.

This has been partially done, with all the _with_libctx
functions.  As far as I've understood, these have been added on
need basis, i.e. somewhere down the code path, a library context
or a property query string is needed.  I can't say if this gives
any sense of completeness or consistency, viewed as an cohesive
API, or if we stand any chance of getting there without a complete
API re-design.

Something to be noted is that it doesn't make sense to pass the
library context everywhere, because it's already part of other
structures that are passed.  For example, any method structure,
such as EVP_CIPHER, EVP_MD, etc are tied to a provider, which is
in turn tied to a library context.  Passing another library
context to anything that includes one of those method structures
would only be confusing.

2.  Pass it when constructing different structures, mostly other
context structures.  As an example, EVP_PKEY_CTX_new_from_pkey()
that I displayed above.  There may be cases where we need to pass
it directly to functions that aren't constructors, but I expect
those cases to be relatively few.

This has been done for some other structures as well, on an as
needed basis:

X509 *X509_new_with_libctx(OPENSSL_CTX *libctx, const char *propq);

X509_STORE_CTX *X509_STORE_CTX_new_with_libctx(OPENSSL_CTX *libctx,
   const char *propq);

(the following are internal only)

int x509_set0_libctx(X509 *x, OPENSSL_CTX *libctx, const char *propq);
int x509_crl_set0_libctx(X509_CRL *x, OPENSSL_CTX *libctx, const char 
*propq);

3.  Set a current library context, a pointer in a thread local
variable.  We already have support for that, which this function:

OPENSSL_CTX *OPENSSL_CTX_set0_default(OPENSSL_CTX *libctx);

The usage model is this:

OPENSSL_CTX *prevctx = OPENSSL_CTX_set0_default(libctx);

/* do stuff with |libctx| as implicit current global state */

OPENSSL_CTX_set0_default(prevctx)

Looking at that list now, I realise that it goes from most intrusive
to least intrusive, viewed as a public API.

The third choice in particular would let any application or library
just set their library context for the duration of the code that
should be execute with that as a "global state", and restore it when
done, leaving the rest of the OpenSSL calls untouched.

Something to be noted is that model 2 

Re: Reordering new API's that have a libctx, propq

[Richard Levitte]

On Sun, 06 Sep 2020 23:40:53 +0200,
SHANE LONTIS wrote:
> 
> If it is decided that the libctx is more important then the API
> should really be something like this..
> OSSL_LIBCTX_MD_fetch(), (and I dont know if that is a good idea.)..

I would rather not see that.

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

Re: Reordering new API's that have a libctx, propq

[SHANE LONTIS]

The example given was EVP_PKEY_get_() and from the API name it is fairly clear 
what the first param should be EVP_PKEY *pkey (the API tells you this).

For the fetch case, I still think that the algorithm is the most important 
param.
Libctx, propq are optional parameters as far as I am concerned.

If it is decided that the libctx is more important then the API should really 
be something like this..
OSSL_LIBCTX_MD_fetch(), (and I dont know if that is a good idea.)..

In some places where libctx is used it is a second class parameter whose only 
job is to perform a sub task of fetching during a bigger operation (such as in 
a load),
And in these cases it should not be the first parameter..

Shane

> On 6 Sep 2020, at 4:02 pm, Richard Levitte  wrote:
> 
> There are many red herrings in here, and I would argue that trying to
> be too uniform in the way you think about all functions may be
> harmful, because not all functions are classified the same.
> 
> We cannot deny that many of our interfaces have an OOP flair.  We may
> be programming in C, but we very obviously have that kind of pattern,
> a "poor man's OOP" in C.  So speaking about our interfaces in OOP
> terms is not far fetched, as long as we keep in mind that we can only
> take the argument so far.
> 
> I would argue that EVP_XXX_get_whatever() / EVP_XXX_get_whatever() are
> classic accessors, and I assume that we have all been brought up with
> the description of "this" as the hidden first argument to any class
> method in C++, so it's not very far fetched to emulate that in C by
> making the instance you want details from or change details of as the
> (explicit) first argument.
> 
> I would further argue that EVP_XXX_fetch() is a constructor (of an
> instance of EVP_XXX), and that with the assumption that there is no
> "this" in a constructor, the discussion about the arguments and their
> order must be different.
> 
> What I hear, even though not in such terms, is an argument of the
> what the library context is and how that should affect the interface.
> In relation to EVP_XXX_fetch(), it seems to be more of a factory (or
> strictly speaking, the pool of resources, with hidden factory
> functions), and depending on the factory model you lean on, it might
> or might not be sensible to have it as first argument.  My you, I'm
> trying quite hard to see it from a fresh user experience (as far as I
> can imagine it...  after all, 20ish years with my fingers deep in
> OpenSSL entrails makes you not so fresh).
> 
> I think, BTW, that this really comes down to how we view the library
> context.  So far, I've seen all these interpretations (not all said
> explicitly, but clearly visible in code or how we argue about it):
> 
> - framework
> - scope (I'm unsure if that differs much from framework)
> - factory / factory pool
> - bag of resources
> 
> Personally, I have zero problems viewing it as all of these combined,
> but that requires us to be clear on how it's used in different places.
> 
> Cheers,
> Richard
> 
> On Sat, 05 Sep 2020 23:18:21 +0200,
> Tim Hudson wrote:
>> 
>> 
>> On Sun, Sep 6, 2020 at 6:55 AM Richard Levitte  wrote:
>> 
>>I'd rank the algorithm name as the most important, it really can't do
>>anything of value without it.
>> 
>> It also cannot do anything without knowing which libctx to use. Look at the 
>> implementation.
>> Without the libctx there is no "from-where" to specify. 
>> 
>> This is again hitting the concept of where do things come from and what is a 
>> default.
>> Once "global" disappears as such, logically everything comes from a libctx.
>> 
>> Your argument is basically "what" is more important than "from" or "where".
>> And the specific context here is where you see "from" or "where" can be 
>> defaulted to a value so it
>> can be deduced so it isn't (as) important in the API.
>> 
>> That would basically indicate you would (applying the same pattern/rule in a 
>> different context)
>> change:
>> 
>> int EVP_PKEY_get_int_param(EVP_PKEY *pkey, const char *key_name, int *out);
>> 
>> To the following (putting what you want as the most important rather than 
>> from):
>> 
>> int EVP_PKEY_get_int_param(char *key_name, EVP_PKEY *pkey, int *out);
>> 
>> Or pushing it right to the end after the output parameter:
>> 
>> int EVP_PKEY_get_int_param(char *key_name, int *out,EVP_PKEY *pkey);
>> 
>> The context of where things come from is actually the most critical item in 
>> any of the APIs we
>> have.
>> Even though what you want to get from where you want to get it is in the 
>> point of the API call you
>> need to specify where from first as that context sets the frame of the call.
>> 
>> Think of it around this way - we could have an implementation where we 
>> remember the last key that
>> we have used and allow you to simply indicate you use the last key or if we 
>> didn't want the last
>> key used to be able to specify it via a non-NULL pointer. This isn't that 
>> unusual an API but not
>> something I'm 

Re: Reordering new API's that have a libctx, propq

[Richard Levitte]

There are many red herrings in here, and I would argue that trying to
be too uniform in the way you think about all functions may be
harmful, because not all functions are classified the same.

We cannot deny that many of our interfaces have an OOP flair.  We may
be programming in C, but we very obviously have that kind of pattern,
a "poor man's OOP" in C.  So speaking about our interfaces in OOP
terms is not far fetched, as long as we keep in mind that we can only
take the argument so far.

I would argue that EVP_XXX_get_whatever() / EVP_XXX_get_whatever() are
classic accessors, and I assume that we have all been brought up with
the description of "this" as the hidden first argument to any class
method in C++, so it's not very far fetched to emulate that in C by
making the instance you want details from or change details of as the
(explicit) first argument.

I would further argue that EVP_XXX_fetch() is a constructor (of an
instance of EVP_XXX), and that with the assumption that there is no
"this" in a constructor, the discussion about the arguments and their
order must be different.

What I hear, even though not in such terms, is an argument of the
what the library context is and how that should affect the interface.
In relation to EVP_XXX_fetch(), it seems to be more of a factory (or
strictly speaking, the pool of resources, with hidden factory
functions), and depending on the factory model you lean on, it might
or might not be sensible to have it as first argument.  My you, I'm
trying quite hard to see it from a fresh user experience (as far as I
can imagine it...  after all, 20ish years with my fingers deep in
OpenSSL entrails makes you not so fresh).

I think, BTW, that this really comes down to how we view the library
context.  So far, I've seen all these interpretations (not all said
explicitly, but clearly visible in code or how we argue about it):

- framework
- scope (I'm unsure if that differs much from framework)
- factory / factory pool
- bag of resources

Personally, I have zero problems viewing it as all of these combined,
but that requires us to be clear on how it's used in different places.

Cheers,
Richard

On Sat, 05 Sep 2020 23:18:21 +0200,
Tim Hudson wrote:
> 
> 
> On Sun, Sep 6, 2020 at 6:55 AM Richard Levitte  wrote:
> 
> I'd rank the algorithm name as the most important, it really can't do
> anything of value without it.
> 
> It also cannot do anything without knowing which libctx to use. Look at the 
> implementation.
> Without the libctx there is no "from-where" to specify. 
> 
> This is again hitting the concept of where do things come from and what is a 
> default.
> Once "global" disappears as such, logically everything comes from a libctx.
> 
> Your argument is basically "what" is more important than "from" or "where".
> And the specific context here is where you see "from" or "where" can be 
> defaulted to a value so it
> can be deduced so it isn't (as) important in the API.
> 
> That would basically indicate you would (applying the same pattern/rule in a 
> different context)
> change:
> 
> int EVP_PKEY_get_int_param(EVP_PKEY *pkey, const char *key_name, int *out);
> 
> To the following (putting what you want as the most important rather than 
> from):
> 
> int EVP_PKEY_get_int_param(char *key_name, EVP_PKEY *pkey, int *out);
> 
> Or pushing it right to the end after the output parameter:
> 
> int EVP_PKEY_get_int_param(char *key_name, int *out,EVP_PKEY *pkey);
> 
> The context of where things come from is actually the most critical item in 
> any of the APIs we
> have.
> Even though what you want to get from where you want to get it is in the 
> point of the API call you
> need to specify where from first as that context sets the frame of the call.
> 
> Think of it around this way - we could have an implementation where we 
> remember the last key that
> we have used and allow you to simply indicate you use the last key or if we 
> didn't want the last
> key used to be able to specify it via a non-NULL pointer. This isn't that 
> unusual an API but not
> something I'm suggesting we add - just trying to get the point across that 
> you are still thinking
> of global and libctx as something super special with an exception in its 
> handling rather than
> applying a general rule which is pretty much what we use everywhere else.
> 
> And in which case where you generally don't provide a reference as there is 
> some default meaning
> for it in general and can provide a reference for that sort of API would this 
> make sense to you:
> 
> int EVP_PKEY_get_int_param(char *key_name, int *out,EVP_PKEY *pkey);
> 
> If pkey is NULL then you use the last key that you referenced, if it is not 
> then you use the
> specified pkey. For the application the specific key_name is the most 
> important thing (using your
> argument that basically states the "what" is what counts). 
> 
> I would suggest that you really would still want to place the EVP_PKEY first 
> - even if you had a

Re: Reordering new API's that have a libctx, propq

[Richard Levitte]

Hi,

so...  "importance" quite obviously carries different meaning to
different people.  What I see below is the meaning "having the longest
life span" or possibly "being the biggest and most powerful resource".

I've a different interpretation of "importance".

Looking at EVP_XXX_fetch(), it's primary function is to get the caller
an implementation of an algorithm, so a pretty important input for it
to function is the name of that algorithm.  Of course, it also needs
to know where to go look and under what conditions (i.e. what properties
need to apply), but in terms of importance for this function to work,
I'd rank the algorithm name as the most important, it really can't do
anything of value without it.

With the in mind, the current (libctx, algoname, propq) argument order
is...  odd.
I don't quite see if there was a suggestion to have (libctx, propq,
algoname) as argument order; that's just plain weird in my mind.

Those were late evening thoughts, I'll get back when I'm mulled over
this a bit more.

Cheers,
Richard

On Sat, 05 Sep 2020 12:44:51 +0200,
Nicola Tuveri wrote:
> 
> 
> On Sat, Sep 5, 2020, 12:13 Tim Hudson  wrote:
> 
> On Sat, Sep 5, 2020 at 6:38 PM Nicola Tuveri  wrote:
>
> In most (if not all) cases in our functions, both libctx and 
> propquery are optional
> arguments, as we have global defaults for them based on the loaded 
> config file.
> As such, explicitly passing non-NULL libctx and propquery, is likely 
> to be an exceptional
> occurrence rather than the norm.
> 
> And that is where we have a conceptual difference, the libctx is always 
> used. If it is
> provided as a NULL parameter then it is a shortcut to make the call to 
> get the default or to
> get the already set one.
> Conceptually it is always required for the function to operate.
>
> And the conceptual issue is actually important here - all of these 
> functions require the
> libctx to do their work - if it is not available then they are unable to 
> do their work.
> We just happen to have a default-if-NULL.
>
> If C offered the ability to default a parameter if not provided (and many 
> languages offer
> that) I would expect we would be using it. 
> But it doesn't - we are coding in C.
>
> So it is really where-is-what-this-function-needs-coming-from that 
> actually is the important
> thing - the source of the information the function needs to make its 
> choice.
> It isn't which algorithm is being selected - the critical thing is from 
> which pool of
> algorithm implementations are we operating. The pool must be specified 
> (this is C code), but
> we have a default value.
>
> And that is why I think the conceptual approach here is getting confused 
> by the arguments
> appearing to be optional - conceptually they are not - we just have a 
> defaulting mechanism and
> that isn't the same conceptually as the arguments actually being optional.
>
> Clearer?
> 
> It's not yet clear to me the distinction you are trying to make.
> 
> I'll try to spell out what I extrapolated from your answer, and I apologize 
> in advance if I am
> misunderstanding your argument, please be patient and correct me in that case 
> so I can better
> understand your point! 
> 
> It seems to me you are making a conceptual difference between
> - a function that internally requires an instance of foo to work (and has a 
> default if foo is
> given as NULL among the arguments); e.g libctx is necessary for a fetch, if a 
> NULL libctx is given
> a mechanism is in place to retrieve the global default one
> - a function that internally uses an instance of foo only if a non-NULL one 
> is passed as argument;
> e.g. bnctx, if the user provides it this is used by the callee and passed to 
> its callee, if the
> user passes NULL the function creates a fresh one for itself and/or its 
> callees
> 
> But as a consumer of the API that difference is not visible and probably not 
> even interesting, as
> we are programming in C and passing pointers, there are certain arguments 
> that are required and
> must be passed as valid pointers, others that appear optional because as a 
> consumer of that API I
> can pass NULL and let the function internally default to a reasonable 
> behavior (and whatever this
> "reasonable behavior" is — whether the first or the second case from above, 
> or another one I did
> not list —, it's part of the documentation of that API).
> 
> IMHO, in the consumer POV, libctx and propq are optional arguments (even in C 
> where optional or
> default arguments do not technically exist and the caller needs to always 
> specify a value for each
> argument, which are always positional) in the sense that they can pass NULL 
> as a value rather than
> a pointer to a fully instantiated object of the required type.
> Even more so given that, excluding a minority of cases, we can expect 
> consumers of the 

Re: Reordering new API's that have a libctx, propq

[Nicola Tuveri]

On Sat, Sep 5, 2020, 14:01 Tim Hudson  wrote:

> On Sat, Sep 5, 2020 at 8:45 PM Nicola Tuveri  wrote:
>
>> Or is your point that we are writing in C, all the arguments are
>> positional, none is ever really optional, there is no difference between
>> passing a `(void*) NULL` or a valid `(TYPE*) ptr` as the value of a `TYPE
>> *` argument, so "importance" is the only remaining sorting criteria, hence
>> (libctx, propq) are always the most important and should go to the
>> beginning of the args list (with the exception of the `self/this` kind of
>> argument that always goes first)?
>>
>
> That's a reasonable way to express things.
>
> The actual underlying point I am making is that we should have a rule in
> place that is documented and that works within the programming language we
> are using and that over time doesn't turn into a mess.
> We do add parameters (in new function names) and we do like to keep the
> order of the old function - and ending up with a pile of things in the
> "middle" is in my view is one of the messes that we should be avoiding.
>

We are already adding new functions, with the ex suffix, to allow users to
keep using the old version, so given that the users passing to the "_ex"
function are already altering their source, why are we limiting us from
rationalizing the signature from the PoV of new users and old users alike
that are in general quite critic of our API usability, even if it means
that applying both "required-ness" and "importance" as sorting criteria
sometime we end up adding in the middle?

I don't think I am being dismissive of the needs of existing applications
here: if a maintainer is altering their code from using "EVP_foo()" to
"EVP_foo_ex()", they will likely also be looking at the documentation of
the old and the new API versions and there shouldn't be really any
additional significanf cost in adding a parameter a the edges or in the
middle.

I think the importance argument is the one that helps for setting order and
> on your "required args" coming first approach the importance argument also
> applies because it is actually a required argument simply with an implied
> default - which again puts it not at the end of the argument list. The
> library context is required - always - there is just a default - and that
> parameter must be present because we are working in C.
>

I think I disagree with this, from the API CONSUMER PoV there is a clear
difference between a function where they don't need to pass a valid libctx
pointer and instead pass NULL (because there is a default associated with
passing NULL) and a function like an hypothetical
OSSL_LIBCTX_get_this(libctx) or OSSL_LIBCTX_set_that(libctx, value).

In the first case the function operates despite the programmer not
providing an explicit libctx (because a default one is used), in the latter
the programmer is querying/altering directly the libctx and one must be
provided.

*… actually only now that I wrote out the greyed text above I think I am
starting to understand what you meant all along!*

Your point is that any API that uses a `libctx` (and `propq`) is always
querying/altering a libctx, even if we use NULL as a shortcut to mean "the
global one", so if we are fetching an algorithm, getting/setting something
on a libctx scope, creating a new object, we are always doing so from a
given libctx.
In that sense, when an API consumer is passing NULL they are not passing
"nothing" (on which my "optional arguments" approach is based), but they
are passing NULL as a valid reference to a very specific libctx.

I now see what you meant, and I can see how it is a relevant thing to make
evident also in the function signature/usage.

But I guess we can also agree that passing NULL as a very specific
reference instead of as "nothing", can create a lot of confusion for
external consumers of the API, if it took this long for one of us — ­*ok,
it's me, so probably everyone else understood your point immediately, but
still…* — to understand the difference you were pointing out, even knowing
how these functions work internally and being somewhat familiar with
dealing with libctx-s.
If we want to convey this difference properly to our users, would it be
better to use a new define?

#define OSSL_DEFAULT ((void*)42)

- OSSL_DFLT could be a shorter alternative, if we prefer brevity.
- I am intentionally not specifying _LIBCTX as we might reuse a similar
mechanism to avoid overloading NULL in other similar situations: e.g. for
propq, where NULL is again a shortcut for the global ones, compared with
passing "" as an empty properties query, seems like another good candidate
for using a symbol to explicitly highlight that the default propq will be
used
- I would avoid making OSSL_DFLT an alias for NULL to prevent users from
sidestepping the define completely and use NULL directly (because "it just
works anyway"): that is why I am picking 42 as an example, but any magic
number different from NULL/0 and that is most likely/always 

Re: Reordering new API's that have a libctx, propq

[Tim Hudson]

On Sat, Sep 5, 2020 at 8:45 PM Nicola Tuveri  wrote:

> Or is your point that we are writing in C, all the arguments are
> positional, none is ever really optional, there is no difference between
> passing a `(void*) NULL` or a valid `(TYPE*) ptr` as the value of a `TYPE
> *` argument, so "importance" is the only remaining sorting criteria, hence
> (libctx, propq) are always the most important and should go to the
> beginning of the args list (with the exception of the `self/this` kind of
> argument that always goes first)?
>

That's a reasonable way to express things.

The actual underlying point I am making is that we should have a rule in
place that is documented and that works within the programming language we
are using and that over time doesn't turn into a mess.
We do add parameters (in new function names) and we do like to keep the
order of the old function - and ending up with a pile of things in the
"middle" is in my view is one of the messes that we should be avoiding.

I think the importance argument is the one that helps for setting order and
on your "required args" coming first approach the importance argument also
applies because it is actually a required argument simply with an implied
default - which again puts it not at the end of the argument list. The
library context is required - always - there is just a default - and that
parameter must be present because we are working in C.

Whatever it is that we end up decided to do, the rule should be captured
and should also allow for the fact that we will evolve APIs and create _ex
versions and those two will also evolve and a general rule should be one
that doesn't result in an inconsistent treatment for argument order as we
add _ex versions.

Tim.

Re: Reordering new API's that have a libctx, propq

[Nicola Tuveri]

On Sat, Sep 5, 2020, 12:13 Tim Hudson  wrote:

> On Sat, Sep 5, 2020 at 6:38 PM Nicola Tuveri  wrote:
>
>> In most (if not all) cases in our functions, both libctx and propquery
>> are optional arguments, as we have global defaults for them based on the
>> loaded config file.
>> As such, explicitly passing non-NULL libctx and propquery, is likely to
>> be an exceptional occurrence rather than the norm.
>>
>
> And that is where we have a conceptual difference, the libctx is *always* 
> used.
> If it is provided as a NULL parameter then it is a shortcut to make the
> call to get the default or to get the already set one.
> Conceptually it is always required for the function to operate.
>
> And the conceptual issue is actually important here - all of these
> functions require the libctx to do their work - if it is not available then
> they are unable to do their work.
> We just happen to have a default-if-NULL.
>
> If C offered the ability to default a parameter if not provided (and many
> languages offer that) I would expect we would be using it.
> But it doesn't - we are coding in C.
>
> So it is really where-is-what-this-function-needs-coming-from that
> actually is the important thing - the source of the information the
> function needs to make its choice.
> It isn't which algorithm is being selected - the critical thing is from
> which pool of algorithm implementations are we operating. The pool must be
> specified (this is C code), but we have a default value.
>
> And that is why I think the conceptual approach here is getting confused
> by the arguments appearing to be optional - conceptually they are not - we
> just have a defaulting mechanism and that isn't the same conceptually as
> the arguments actually being optional.
>
> Clearer?
>

It's not yet clear to me the distinction you are trying to make.

I'll try to spell out what I extrapolated from your answer, and I apologize
in advance if I am misunderstanding your argument, please be patient and
correct me in that case so I can better understand your point!

It seems to me you are making a conceptual difference between
- a function that internally requires an instance of foo to work (and has a
default if foo is given as NULL among the arguments); e.g libctx is
necessary for a fetch, if a NULL libctx is given a mechanism is in place to
retrieve the global default one
- a function that internally uses an instance of foo only if a non-NULL one
is passed as argument; e.g. bnctx, if the user provides it this is used by
the callee and passed to its callee, if the user passes NULL the function
creates a fresh one for itself and/or its callees


But as a consumer of the API that difference is not visible and probably
not even interesting, as we are programming in C and passing pointers,
there are certain arguments that are required and must be passed as valid
pointers, others that appear optional because as a consumer of that API I
can pass NULL and let the function internally default to a reasonable
behavior (and whatever this "reasonable behavior" is — whether the first or
the second case from above, or another one I did not list —, it's part of
the documentation of that API).

IMHO, in the consumer POV, libctx and propq are optional arguments (even in
C where optional or default arguments do not technically exist and the
caller needs to always specify a value for each argument, which are always
positional) in the sense that they can pass NULL as a value rather than a
pointer to a fully instantiated object of the required type.
Even more so given that, excluding a minority of cases, we can expect
consumers of the APIs taking libctx and propq as arguments to pass NULL for
both of them and rely on the openssl config mechanism.

So while I agree with Tim that sometime it is valuable to make a difference
among the consequences of passing NULL as arguments in the context of one
kind of function and another, I believe the place for that is the
documentation not its signature.
The signature of the function should be designed for consumer usability,
and the conventional pattern there seems to be
- required args
- optional args
- callback+cb_args
and inside each group the "importance" factor should be the secondary
sorting criteria.

"importance" is probably going to be affected by the difference you are
making (or my understanding of it): e.g. if a function took both libctx and
bnctx, the fact that a valid pre-existing libctx is required to work (and a
global already existing one will be retrieved in case none is given), while
a fresh short-lived bnctx is going to be created only for the lifetime of
the called function in case none is given seems to indicate that libctx is
of vital importance for the API functionality, while bnctx is of minor
relevance.

But... going this way as a generalized approach, would bring us to the "add
in the middle" scenario that we'd like to avoid.
I recognize that this is a point you already made in your original writeup,
as the 

Re: Reordering new API's that have a libctx, propq

[Tim Hudson]

On Sat, Sep 5, 2020 at 6:38 PM Nicola Tuveri  wrote:

> In most (if not all) cases in our functions, both libctx and propquery are
> optional arguments, as we have global defaults for them based on the loaded
> config file.
> As such, explicitly passing non-NULL libctx and propquery, is likely to be
> an exceptional occurrence rather than the norm.
>

And that is where we have a conceptual difference, the libctx is *always* used.
If it is provided as a NULL parameter then it is a shortcut to make the
call to get the default or to get the already set one.
Conceptually it is always required for the function to operate.

And the conceptual issue is actually important here - all of these
functions require the libctx to do their work - if it is not available then
they are unable to do their work.
We just happen to have a default-if-NULL.

If C offered the ability to default a parameter if not provided (and many
languages offer that) I would expect we would be using it.
But it doesn't - we are coding in C.

So it is really where-is-what-this-function-needs-coming-from that actually
is the important thing - the source of the information the function needs
to make its choice.
It isn't which algorithm is being selected - the critical thing is from
which pool of algorithm implementations are we operating. The pool must be
specified (this is C code), but we have a default value.

And that is why I think the conceptual approach here is getting confused by
the arguments appearing to be optional - conceptually they are not - we
just have a defaulting mechanism and that isn't the same conceptually as
the arguments actually being optional.

Clearer?

Tim.

Re: Reordering new API's that have a libctx, propq

[Nicola Tuveri]

Thanks Tim for the writeup!


I tend to agree with Tim's conclusions in general, but I fear the analysis
here is missing an important premise that could influence the outcome of
our decision.

In most (if not all) cases in our functions, both libctx and propquery are
optional arguments, as we have global defaults for them based on the loaded
config file.
As such, explicitly passing non-NULL libctx and propquery, is likely to be
an exceptional occurrence rather than the norm.

For optional parameters most developers from C and a variety of languages,
would expect them to come at the end of the list of parameters, and this
also follows the rule of thumb of "importance" used by Tim to pick 1) and
B/A).

For this reason I would instead suggest to go with 2) and C) in this case
(with the caveat of keeping callback and its args as the very last
arguments, again this is a non-written convention not only for us but quite
widespread).



Nicola



On Sat, Sep 5, 2020, 10:10 Tim Hudson  wrote:

> I place the OTC hold because I don't believe we should be making parameter
> reordering decisions without actually having documented what approach we
> want to take so there is clear guidance.
> This was the position I expressed in the last face to face OTC meeting -
> that we need to write such things down - so that we avoid this precise
> situation - where we have new functions that are added that introduce the
> inconsistency that has been noted here that PR#12778 is attempting to
> address.
>
> However, this is a general issue and not a specific one to OPENSSL_CTX and
> it should be discussed in the broader context and not just be a last minute
> (before beta1) API argument reordering.
> That does not provide anyone with sufficient time to consider whether or
> not the renaming makes sense in the broader context.
> I also think that things like API argument reordering should have been
> discussed on openssl-project so that the broader OpenSSL community has an
> opportunity to express their views.
>
> Below is a quick write up on APIs in an attempt to make it easier to hold
> an email discussion about the alternatives and implications of the various
> alternatives over time.
> I've tried to outline the different options.
>
> In general, the OpenSSL API approach is of the following form:
>
>
>
> *rettype  TYPE_get_something(TYPE *,[args])rettype  TYPE_do_something(TYPE
> *,[args])TYPE*TYPE_new([args])*
>
> This isn't universally true, but it is the case for the majority of
> OpenSSL functions.
>
> In general, the majority of the APIs place the "important" parameters
> first, and the ancillary information afterwards.
>
> In general, for output parameters we tend to have those as the return
> value of the function or an output parameter that
> tends to be at the end of the argument list. This is less consistent in
> the OpenSSL APIs.
>
> We also have functions which operate on "global" information where the
> information used or updated or changed
> is not explicitly provided as an API parameter - e.g. all the byname
> functions.
>
> Adding the OPENSSL_CTX is basically providing where to get items from that
> used to be "global".
> When performing a lookup, the query string is a parameter to modify the
> lookup being performed.
>
> OPENSSL_CTX is a little different, as we are adding in effectively an
> explicit parameter where there was an implicit (global)
> usage in place. But the concept of adding parameters to functions over
> time is one that we should have a policy for IMHO.
>
> For many APIs we basically need the ability to add the OPENSSL_CTX that is
> used to the constructor so that
> it can be used for handling what used to be "global" information where
> such things need the ability to
> work with other-than-the-default OPENSSL_CTX (i.e. not the previous single
> "global" usage).
>
> That usage works without a query string - as it isn't a lookup as such -
> so there is no modifier present.
> For that form of API usage we have three choices as to where we place
> things:
>
> 1) place the context first
>
> *TYPE *TYPE_new(OPENSSL_CTX *,[args])*
>
> 2) place the context last
>
> *TYPE *TYPE_new([args],OPENSSL_CTX *)*
>
> 3) place the context neither first nor last
>
> *TYPE *TYPE_new([some-args],OPENSSL_CTX *,[more-args])*
>
> Option 3 really isn't a sensible choice to make IMHO.
>
> When we are performing effectively a lookup that needs a query string, we
> have a range of options.
> If we basically state that for a given type, you must use the OPENSSL_CTX
> you have been provided with on construction,
> (not an unreasonable position to take), then you don't need to modify the
> existing APIs.
>
> If we want to allow for a different OPENSSL_CTX for a specific existing
> function, then we have to add items.
> Again we have a range of choices:
>
> A) place the new arguments first
>
>
> *rettype  TYPE_get_something(OPENSSL_CTX *,TYPE *,[args])rettype
>  TYPE_do_something(OPENSSL_CTX *,TYPE *,[args])*

Re: Reordering new API's that have a libctx, propq

[SHANE LONTIS]

Thanks for the writeup..

> My view is that the importance of arguments is what sets their order - and 
> that is why for the TYPE functions the TYPE pointer
> comes first. We could have just as easily specified it as last or some other 
> order, but we didn’t.

Isn’t that the order that the fetch is in if it changes to what it is in the 
PR? I would argue that the algorithm is the most important parameter in the 
fetch.

Deciding what argument is more ‘important’ is not always easy either.
If we are doing an operation such as a get or load and it needs a whole lot of 
input params in order to return something,
then aren’t they more important/ or just as important as the libctx,propq - 
(since the libctx,propq is just used normally to fetch something)?
This then means the libctx ends up last, or maybe in the middle again.

Also it is quite common for the new() methods (such as for X509) to need both 
the libctx and propq..
The X509 object gets passed to an d2i method that internally needs to do 
fetches (SHA1) and cache.

Shane

> On 5 Sep 2020, at 5:09 pm, Tim Hudson  wrote:
> 
> My view is that the importance of arguments is what sets their order - and 
> that is why for the TYPE functions the TYPE pointer
> comes first. We could have just as easily specified it as last or some other 
> order, but we didn't.

Re: Reordering new API's that have a libctx, propq

[Tim Hudson]

I place the OTC hold because I don't believe we should be making parameter
reordering decisions without actually having documented what approach we
want to take so there is clear guidance.
This was the position I expressed in the last face to face OTC meeting -
that we need to write such things down - so that we avoid this precise
situation - where we have new functions that are added that introduce the
inconsistency that has been noted here that PR#12778 is attempting to
address.

However, this is a general issue and not a specific one to OPENSSL_CTX and
it should be discussed in the broader context and not just be a last minute
(before beta1) API argument reordering.
That does not provide anyone with sufficient time to consider whether or
not the renaming makes sense in the broader context.
I also think that things like API argument reordering should have been
discussed on openssl-project so that the broader OpenSSL community has an
opportunity to express their views.

Below is a quick write up on APIs in an attempt to make it easier to hold
an email discussion about the alternatives and implications of the various
alternatives over time.
I've tried to outline the different options.

In general, the OpenSSL API approach is of the following form:



*rettype  TYPE_get_something(TYPE *,[args])rettype  TYPE_do_something(TYPE
*,[args])TYPE*TYPE_new([args])*

This isn't universally true, but it is the case for the majority of OpenSSL
functions.

In general, the majority of the APIs place the "important" parameters
first, and the ancillary information afterwards.

In general, for output parameters we tend to have those as the return value
of the function or an output parameter that
tends to be at the end of the argument list. This is less consistent in the
OpenSSL APIs.

We also have functions which operate on "global" information where the
information used or updated or changed
is not explicitly provided as an API parameter - e.g. all the byname
functions.

Adding the OPENSSL_CTX is basically providing where to get items from that
used to be "global".
When performing a lookup, the query string is a parameter to modify the
lookup being performed.

OPENSSL_CTX is a little different, as we are adding in effectively an
explicit parameter where there was an implicit (global)
usage in place. But the concept of adding parameters to functions over time
is one that we should have a policy for IMHO.

For many APIs we basically need the ability to add the OPENSSL_CTX that is
used to the constructor so that
it can be used for handling what used to be "global" information where such
things need the ability to
work with other-than-the-default OPENSSL_CTX (i.e. not the previous single
"global" usage).

That usage works without a query string - as it isn't a lookup as such - so
there is no modifier present.
For that form of API usage we have three choices as to where we place
things:

1) place the context first

*TYPE *TYPE_new(OPENSSL_CTX *,[args])*

2) place the context last

*TYPE *TYPE_new([args],OPENSSL_CTX *)*

3) place the context neither first nor last

*TYPE *TYPE_new([some-args],OPENSSL_CTX *,[more-args])*

Option 3 really isn't a sensible choice to make IMHO.

When we are performing effectively a lookup that needs a query string, we
have a range of options.
If we basically state that for a given type, you must use the OPENSSL_CTX
you have been provided with on construction,
(not an unreasonable position to take), then you don't need to modify the
existing APIs.

If we want to allow for a different OPENSSL_CTX for a specific existing
function, then we have to add items.
Again we have a range of choices:

A) place the new arguments first


*rettype  TYPE_get_something(OPENSSL_CTX *,TYPE *,[args])rettype
 TYPE_do_something(OPENSSL_CTX *,TYPE *,[args])*

B) place the new arguments first after the TPYE



*rettype  TYPE_get_something(TYPE *,OPENSSL_CTX *,[args])rettype
 TYPE_do_something(TYPE *,OPENSSL_CTX *,[args])*
C) place the new arguments last


*rettype  TYPE_get_something(TYPE *,[args], OPENSSL_CTX *)rettype
 TYPE_do_something(TYPE *,[args], OPENSSL_CTX *)*

D) place the new arguments neither first nor last



*rettype  TYPE_get_something(TYPE *,[some-args], OPENSSL_CTX
*,[more-args])rettype  TYPE_do_something(OPENSSL_CTX *,TYPE *,[some-args],
OPENSSL_CTX *,[more-args])*
Option D really isn't a sensible choice to make IMHO.

My view is that the importance of arguments is what sets their order - and
that is why for the TYPE functions the TYPE pointer
comes first. We could have just as easily specified it as last or some
other order, but we didn't.

Now when we need to add a different location from which to retrieve other
information we need to determine where this gets added.
I'd argue that it is at constructor time that we should be adding any
OPENSSL_CTX or query parameter for any existing TYPE usage
in OpenSSL. If we feel the need to cross OPENSSL_CTX's (logically that is
what is being done) 

RE: Reordering new API's that have a libctx, propq

[Dr. Matthias St. Pierre]

Both suggestions make sense to me and they were discussed several times in the 
weekly calls.
So you have my support whether there will be the need for a formal vote or not.

> An otc_hold was put on this.. Do we need to have a formal vote?

Since Tim placed the hold, only he can remove it. If he doesn’t, an OTC vote is 
inevitable.

https://github.com/openssl/openssl/pull/12778#issuecomment-686348526

In that case, let’s just have the vote and finish it quickly.

Matthias


From: openssl-project  On Behalf Of SHANE 
LONTIS
Sent: Saturday, September 5, 2020 5:48 AM
To: openssl-project@openssl.org
Subject: Reordering new API's that have a libctx, propq

PR #12778 reorders all the API’s of the form:
EVP_XX_fetch(libctx, algname, propq)
So that the algorithm name appears first..
e.g: EVP_MD_fetch(digestname, libctx, propq);
This now logically reads as 'search for this algorithm using these parameters’.

The libctx, propq should always appear together as a pair of parameters.
There are only a few places where only the libctx is needed, which means that 
if there is no propq it is likely to cause a bug in a fetch at some point.
This keeps the API’s more consistent with other existing XXX_with_libctx API’s 
that put the libctx, propq at the end of the parameter list..
The exception to this rule is that callback(s) and their arguments occur after 
the libctx, propq..

e.g:
typedef OSSL_STORE_LOADER_CTX *(*OSSL_STORE_open_with_libctx_fn)
(const OSSL_STORE_LOADER *loader,
 const char *uri, OPENSSL_CTX *libctx, const char *propq,
 const UI_METHOD *ui_method, void *ui_data);

An otc_hold was put on this.. Do we need to have a formal vote?
This really needs to be sorted out soon so the API’s can be locked down for 
beta.


Also discussed in a weekly meeting and numerous PR discussions was the removal 
of the long winded API’s ending with ‘with_libctx’
e.g CMS_data_create_with_libctx
The proposed renaming for this was to continue with the _ex notation.. If there 
is an existing _ex name then it becomes _ex2, etc.
There will most likely be additional parameters in the future for some API’s, 
so this notation would be more consistent with current API’s.
Does this also need a vote?

Regards,
Shane

Reordering new API's that have a libctx, propq

[SHANE LONTIS]

PR #12778 reorders all the API’s of the form:

EVP_XX_fetch(libctx, algname, propq) 

So that the algorithm name appears first.. 

e.g: EVP_MD_fetch(digestname, libctx, propq);

This now logically reads as 'search for this algorithm using these parameters’.

The libctx, propq should always appear together as a pair of parameters.
There are only a few places where only the libctx is needed, which means that 
if there is no propq it is likely to cause a bug in a fetch at some point. 

This keeps the API’s more consistent with other existing XXX_with_libctx API’s 
that put the libctx, propq at the end of the parameter list..
The exception to this rule is that callback(s) and their arguments occur after 
the libctx, propq..

e.g:
typedef OSSL_STORE_LOADER_CTX *(*OSSL_STORE_open_with_libctx_fn)
(const OSSL_STORE_LOADER *loader,
 const char *uri, OPENSSL_CTX *libctx, const char *propq,
 const UI_METHOD *ui_method, void *ui_data);

An otc_hold was put on this.. Do we need to have a formal vote?
This really needs to be sorted out soon so the API’s can be locked down for 
beta.


Also discussed in a weekly meeting and numerous PR discussions was the removal 
of the long winded API’s ending with ‘with_libctx’
e.g CMS_data_create_with_libctx
The proposed renaming for this was to continue with the _ex notation.. If there 
is an existing _ex name then it becomes _ex2, etc.
There will most likely be additional parameters in the future for some API’s, 
so this notation would be more consistent with current API’s.
Does this also need a vote?

Regards,
Shane