Refactor OSSL_PARAM_allocate_from_text() for better flexibility by levitte · Pull Request #11046 · openssl/openssl

levitte · 2020-02-09T12:42:53Z

OSSL_PARAM_allocate_from_text() and OSSL_PARAM_construct_from_text()
were "do-it-all" kind of functions, which didn't allow much
flexibility in code that wants to adapt to situations such as the key
not being found in the descriptor OSSL_PARAM array.

We therefore split the functionality to make key lookup and value
processing two different functions, thus resulting in the following
functions:

OSSL_PARAM_parse_locate_const(), which parses the key and sets flags
accordingly, but otherwise works like OSSL_PARAM_locate_const().
OSSL_PARAM_construct_from_text(), which doesn't look up the key any
more, but rather only parses the given value, with direction from a
template OSSL_PARAM item, which could come from a call of
OSSL_PARAM_parse_locate_const().
OSSL_PARAM_allocate_from_text(), which is modified exactly like
OSSL_PARAM_construct_from_text().

With this split, callers can also ignore OSSL_PARAM_parse_locate_const()
completely, and work with an descriptor OSSL_PARAM item of their own
choosing.

romen · 2020-02-09T14:00:59Z

Travis red cross is relevant (duplicated NOTES section)

levitte · 2020-02-09T15:05:26Z

Travis red cross is relevant (duplicated NOTES section)

Thanks. Fixed

paulidale

All this change for one instance where the return is used?
I'm dubious as to the value this provides.

Is it possible to instead consider a sequence like:


if ((p = OSSL_PARAM_locate_const(setable, "name")) == NULL)
    return -2;
if (!OSSL_PARAM_allocate_from_text(&params[0], p, name, value, strlen(value)))
    return 0;

It's less of a kludge, the same number of lines of code, clearer and no slower.

crypto/params_from_text.c

levitte · 2020-02-10T02:31:35Z

@paulidale, I was going to suggest exactly what you suggest, until I realised that it's not that simple, the caller must then also deal with any key variant (such as names prefixed with "hex").

As for only one spot, I'm pretty sure we have more than one ctrl_str function, no? I haven't looked at the others yet.

paulidale · 2020-02-10T02:43:45Z

Good point about the hex version. I still think returning the information via a misused structure field isn't ideal.

Is a ossl_param_locate_ctrl_str function a possibility?
It would be less mystifying to use than this PR.

paulidale · 2020-02-10T02:54:31Z

The function would be:

OSSL_PARAM *ossl_param_locate_ctrl_str(const OSSL_PARAM *paramdefs,
                                       const char *key, int *ishex)
{
    *ishex = strncmp(key, "hex", 3) == 0;
    if (*ishex)
        key += 3;
    return OSSL_PARAM_locate_const(paramdefs, key);
}

levitte · 2020-02-10T03:05:57Z

This function is also used in apps/mac.c, and I really abhor having to use internal functions in the app. Eating our own dog food and all that.

paulidale · 2020-02-10T03:17:04Z

Either make the new function public or don't distinguish "unknown parameter" from other possible failures.

I abhor magic flags which is what this boils down to.

Changing the OSSL_PARAM_allocate_from_text() so it returns -2 for "unknown parameter" isn't ideal but is at least consistent with other APIs.

levitte · 2020-02-10T03:27:54Z

Re names, would you do with OSSL_PARAM_parse_locate_const()? It's not reserved for ctrl_str specifically.

paulidale · 2020-02-10T03:35:27Z

What about #11049 ?

levitte · 2020-02-10T03:45:28Z

Damn it, I was warming up on the idea of a special locate function...

paulidale · 2020-02-10T04:00:57Z

It's only a suggestion.

levitte · 2020-02-10T09:20:00Z

I have now reworked this to have a separate locator function.

levitte · 2020-02-10T12:10:20Z

I think I've fixed the Travis failure.

levitte · 2020-02-10T19:38:09Z

Not WIP any more, I'm quite satisfied with the result by now. Please review (or if you prefer #11049, please say so)

paulidale

I wonder if this is over complicating a public API. For applications that don't care about the reason for failure OSSL_PARAM_allocate_from_text has become more complicated to use -- it's now two calls and an extra local variable is required.

I quite like that OSSL_PARAM_allocate_from_text did the search.

I prefer #11049 from a user's perspective.

crypto/params_from_text.c

paulidale · 2020-02-10T22:33:37Z

BTW: most internal uses of OSSL_PARAM_allocate_from_text don't seem to care about the reason for failure.

levitte · 2020-02-13T11:55:01Z

Fixed Travis failures (C++ builds with our headers! I haven't seen those fail in a while... and that, just because I dared call a parameter template)

levitte · 2020-02-13T11:55:48Z

As to the choice between this and #11049, don't ask me to make it. I'm obviously biased

paulidale · 2020-02-13T22:45:14Z

The more I think about it, the more I prefer the #11049 approach. I've a conflict of interest too.

levitte · 2020-02-13T22:47:19Z

So, er, would someone else please step in and make a judgment call? @openssl

t8m · 2020-02-14T16:03:07Z

Just my opinion: Although this PR is slightly cleaner architecturally from the callers POV I really prefer #11049 as being more simple to use.

slontis · 2020-02-16T08:26:47Z

I really prefer #11049 as being more simple to use.

I agree with Tomas. API simplicity should win.

paulidale · 2020-02-17T05:07:04Z

An option for future expansion would be to return an unsigned value which contains flags rather than a found/not found Boolean. I can't think of a use for this but it would be more future proof.

levitte · 2020-02-17T05:19:47Z

@paulidale, how does what you just wrote apply to this PR?

paulidale · 2020-02-17T05:23:17Z

It's for/about #11049 but the discussion is happening here...

levitte · 2020-02-17T05:33:18Z

Yeah, that's not confusing at all...

levitte · 2020-02-19T21:01:05Z

@t8m, @slontis, so go approve it? Or this one if you change your mind.

OSSL_PARAM_allocate_from_text() and OSSL_PARAM_construct_from_text() were "do-it-all" kind of functions, which didn't allow much flexibility in code that wants to adapt to situations such as the key not being found in the descriptor OSSL_PARAM array. We therefore split the functionality to make key lookup and value processing two different functions, thus resulting in the following functions: - OSSL_PARAM_parse_locate_const(), which parses the key and sets flags accordingly, but otherwise works like OSSL_PARAM_locate_const(). - OSSL_PARAM_construct_from_text(), which doesn't look up the key any more, but rather only parses the given value, with direction from a template OSSL_PARAM item, which could come from a call of OSSL_PARAM_parse_locate_const(). - OSSL_PARAM_allocate_from_text(), which is modified exactly like OSSL_PARAM_construct_from_text(). With this split, callers can also ignore OSSL_PARAM_parse_locate_const() completely, and work with an descriptor OSSL_PARAM item of their own choosing.

Now that locating the key in the settables is done with OSSL_PARAM_parse_locate_const(), legacy_ctrl_str_to_param() has enough information to return -2 when the given key isn't supported.

C++ (enable-buildtest-c++) didn't quite like a parameter named 'template' wonder why... ;-)

levitte · 2020-02-20T12:53:37Z

Choice made: #11049 (review)
Closing this.

levitte requested a review from romen February 9, 2020 12:42

levitte added approval: review pending This pull request needs review by a committer branch: master Applies to master branch labels Feb 9, 2020

levitte mentioned this pull request Feb 9, 2020

Decentralize legacy_ctrl_str_to_param() #10947

Closed

paulidale requested changes Feb 10, 2020

View reviewed changes

crypto/params_from_text.c Outdated Show resolved Hide resolved

crypto/params_from_text.c Outdated Show resolved Hide resolved

levitte changed the title ~~OSSL_PARAM_allocate_from_text(): Allow detection of unsupported param keys~~ [WIP] OSSL_PARAM_allocate_from_text(): Allow detection of unsupported param keys Feb 10, 2020

paulidale mentioned this pull request Feb 10, 2020

OSSL_PARAM_allocate_from_text(): Allow detection of unsupported param keys #11049

Closed

2 tasks

levitte changed the title ~~[WIP] OSSL_PARAM_allocate_from_text(): Allow detection of unsupported param keys~~ [WIP] Refactor OSSL_PARAM_allocate_from_text() for better flexibility Feb 10, 2020

levitte changed the title ~~[WIP] Refactor OSSL_PARAM_allocate_from_text() for better flexibility~~ Refactor OSSL_PARAM_allocate_from_text() for better flexibility Feb 10, 2020

paulidale reviewed Feb 10, 2020

View reviewed changes

crypto/params_from_text.c Outdated Show resolved Hide resolved

levitte force-pushed the fix-OSSL_PARAM_allocate_from_text-20200209 branch 2 times, most recently from 9580cdb to 4d1e70c Compare February 13, 2020 08:32

levitte added 7 commits February 19, 2020 22:03

EVP: legacy_ctrl_str_to_param() can now return -2 appropriately

7e786df

Now that locating the key in the settables is done with OSSL_PARAM_parse_locate_const(), legacy_ctrl_str_to_param() has enough information to return -2 when the given key isn't supported.

Adapt everything else to OSSL_PARAM_parse_locate_const()

852cbe6

fixup! Refactor OSSL_PARAM_allocate_from_text() for better flexibility

26d7de9

fixup! EVP: legacy_ctrl_str_to_param() can now return -2 appropriately

0e127dc

fixup! Adapt everything else to OSSL_PARAM_parse_locate_const()

b0e0791

fixup! Refactor OSSL_PARAM_allocate_from_text() for better flexibility

4c1bb78

C++ (enable-buildtest-c++) didn't quite like a parameter named 'template' wonder why... ;-)

levitte force-pushed the fix-OSSL_PARAM_allocate_from_text-20200209 branch from 90a6592 to 4c1bb78 Compare February 19, 2020 21:03

levitte closed this Feb 20, 2020

levitte deleted the fix-OSSL_PARAM_allocate_from_text-20200209 branch February 20, 2020 12:53

Uh oh!

Conversation

levitte commented Feb 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romen commented Feb 9, 2020

Uh oh!

levitte commented Feb 9, 2020

Uh oh!

paulidale left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

levitte commented Feb 10, 2020

Uh oh!

paulidale commented Feb 10, 2020

Uh oh!

paulidale commented Feb 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte commented Feb 10, 2020

Uh oh!

paulidale commented Feb 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte commented Feb 10, 2020

Uh oh!

paulidale commented Feb 10, 2020

Uh oh!

levitte commented Feb 10, 2020

Uh oh!

paulidale commented Feb 10, 2020

Uh oh!

levitte commented Feb 10, 2020

Uh oh!

levitte commented Feb 10, 2020

Uh oh!

levitte commented Feb 10, 2020

Uh oh!

paulidale left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

paulidale commented Feb 10, 2020

Uh oh!

levitte commented Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte commented Feb 13, 2020

Uh oh!

paulidale commented Feb 13, 2020

Uh oh!

levitte commented Feb 13, 2020

Uh oh!

t8m commented Feb 14, 2020

Uh oh!

slontis commented Feb 16, 2020

Uh oh!

paulidale commented Feb 17, 2020

Uh oh!

levitte commented Feb 17, 2020

Uh oh!

paulidale commented Feb 17, 2020

Uh oh!

levitte commented Feb 17, 2020

Uh oh!

levitte commented Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte commented Feb 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

levitte commented Feb 9, 2020 •

edited

Loading

paulidale commented Feb 10, 2020 •

edited

Loading

paulidale commented Feb 10, 2020 •

edited

Loading

levitte commented Feb 13, 2020 •

edited

Loading

levitte commented Feb 19, 2020 •

edited

Loading