When crypto_newsession() is given a request for an unsupported
capability, raise a more specific error than EINVAL.
This allows cryptotest.py to skip some HMAC tests that a driver does not
support.
Details
Details
Ran cryptotest.py with a SHA driver that only supports SHA1 and 256. SHA1 and
256 test cases are executed and pass. Others are skipped.
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
I was going to say that you needed to update crypto(4) but it doesn't have an ERRORS section yet. :(
Note that using this should also let you remove the hardcoded lists of supported "cipher groups" for device names and all the 'skipif' stuff. Do the tests report as FAIL or SKIP when you raise an exception like this?
| sys/opencrypto/crypto.c | ||
|---|---|---|
| 466 ↗ | (On Diff #33298) | Why not change this instead? I think it will be more consistent and less confusing to have 'EOPNOTSUPP' mean "driver doesn't support algorithm or could not find a driver that does" than to use EINVAL still for the case that 'crid' was a wildcard and we couldn't find a driver that supports the algorithm. |
Comment Actions
Potentially, although I wouldn't do that. The goal is to allow running any supported NIST-KAT HMAC tests against an HMAC-supporting driver. For example, sha_sse only supports SHA1 and SHA256, while the NIST-KAT suite includes SHA224, 384, and 512 vectors as well. It seems wasteful to pummel an AES driver with a whole suite of unsupported HMAC vectors.
Do the tests report as FAIL or SKIP when you raise an exception like this?
Uncaught exceptions result in failure. This change catches only EOPNOTSUPP error exceptions.
| sys/opencrypto/crypto.c | ||
|---|---|---|
| 466 ↗ | (On Diff #33298) | I think it might be useful to distinguish the two cases with different error codes. The "could not find a driver" case could be ENOENT or ENXIO or something. |
Comment Actions
It would be good to get a proper errors section added to the documentation.
| tests/sys/opencrypto/cryptotest.py | ||
|---|---|---|
| 290 ↗ | (On Diff #33298) | I'd recommend moving this comment after the except so that it has sane indenting. |
Comment Actions
Do you consider that an essential part of this change, or could that go in as a separate revision?
| tests/sys/opencrypto/cryptotest.py | ||
|---|---|---|
| 290 ↗ | (On Diff #33298) | Seems sane and clear to me. Comments generally go before the line(s) they describe. |
Comment Actions
Any change that changes behavior, IMO, requires documentation updates as part of the change. W/o documentation, the chance is incomplete.
Comment Actions
The existing behavior is already undocumented. This isn't reducing the accuracy of the documentation.
Comment Actions
I still prefer a single errno value (it's also a smaller code patch and is a bit simpler to read post-patch). I don't think adding the errors section is a hard requirement for this change though, that is an old bug.
| tests/sys/opencrypto/cryptotest.py | ||
|---|---|---|
| 290 ↗ | (On Diff #33298) | I think it's a little clearer if it's inside the except as it is describing what the except clause does. Either that or before the try (as it is describing the reason for having a try/except at all), but of those two I prefer it inside the except. |
Comment Actions
Ok.
| tests/sys/opencrypto/cryptotest.py | ||
|---|---|---|
| 290 ↗ | (On Diff #33298) | Yeah, I think that's backwards, but it seems to be consensus — will do. |
Comment Actions
- Return NOTSUPP for any missed capability
- Move a comment below the line it describes by popular demand
Comment Actions
The change itself seems fine to me. I'll defer to others with respect to whether this is the right direction and whether the documentation needs to be in this revision.
I don't think it is necessarily inconsistent that the various cryptodev_newsession implementations return EINVAL for the corresponding cases where they don't support an algorithm. It may be worth a comment somewhere though.
I guess an alternative might have been to make a more fine-grained capability map for each driver in the test. This would be more of a pain to maintain, but on the positive side it would not require (undocumented) error interpretation, and it would allow checking for erroneous lack of support (as in, we want to test that $driver supports $algo and if it returns EINVAL/EOPNOTSUPP instead then we want to fail the test).
Comment Actions
I think those could maybe even be turned into ASSERTs. It seems like crypto(4) should prevent drivers from being invoked with capabilities they do not advertise.