ENCODER & DECODER: set params on all encoder/decoder instances, unconditionally

[levitte]

OSSL_DECODER_CTX_set_params() and OSSL_ENCODER_CTX_set_params() would
stop as soon as a decoder / encoder instance failed, which leaves the
rest of them with a possibly previous and different value.

Instead, these functions will now call them all, but will return 0 if
any of the instance calls failed.

[mattcaswell]

I'm wondering if it is correct to fail if any of the encoders/decoders fail. If some pass and others fail that might be ok. But I can't think what better behaviour might look like.

[levitte]

I'm wondering if it is correct to fail if any of the encoders/decoders fail. If some pass and others fail that might be ok. But I can't think what better behaviour might look like.

We'd have to have the concept of half failures... whatever that would mean. (30% failed, 50% failed, 74.3% failed... maybe we should return a float 😆)

[mattcaswell]

You could have 0 == all failed, 1 == some success, 2 == all success. That way anything greater than 0 is just "success", and if you want to know that they all succeeded then you could get that information too.

[mattcaswell]

Mind you, what does "fail" actually mean for an encoder/decoder? Does it mean I don't understand this, or does it mean I do understand this, tried to process it, but couldn't. If the latter then perhaps it is ok to just say if any fail then the overall result is 0.

[h-vetinari]

You could have 0 == all failed, 1 == some success, 2 == all success.

I'm aware openssl has an idiosyncratic relationship with error codes, but seeing this, I have to ask: why not

0 == all passed
1,2,3,... == number of failures

?

[mattcaswell]

I'm aware openssl has an idiosyncratic relationship with error codes, but seeing this

Because virtually all functions return 0 for fail and 1 for success (including all the other *_set_params functions). This would reverse the logic, which would be hugely confusing.

[levitte]

Should we really re-invent yet another return code structure? We already have a couple of fairly common ones:

• the binary (0 or 1)
• positive vs negative (1 = success; 0, -1, -2, ... = diverse "levels" of failure)

These both allow using fn(...) > 0 to find out if a call was successful. Now you're inventing new return code structures with wildly different patterns!

If there is to be some kind of count of failures, I'd suggest going for the negative, that would at least be some sort of compatible with already existing patterns. 1 = success, 0 = all failed, -n = negated count of failures.

Seriously, though, I'm not sure why the count of failures is relevant. What is the caller going to do with that? That any of the en-/decoder instances has failed should tell the caller that they shouldn't expect any well defined result if they continue to use the OSSL_ENCODER_CTX where setting the params failed.

[h-vetinari]

All good, was just asking because "1 == some success, 2 == all success" IMO sounds like a footgun waiting to happen.

• positive vs negative (1 = success; 0, -1, -2, ... = diverse "levels" of failure)
• 1 = success, 0 = all failed, -n = negated count of failures.

Both reasonable.

[levitte]

Either way, this PR is approved... I guess we can come back to failure rates at another time, if that turns out to be relevant

[mattcaswell]

Either way, this PR is approved... I guess we can come back to failure rates at another time, if that turns out to be relevant

Yes, its fine. Just me thinking out aloud.

[openssl-machine]

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

[levitte]

Merged

9096809 ENCODER & DECODER: set params on all encoder/decoder instances, unconditionally