Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix driver schema json default type requirements #9674

Merged
merged 1 commit into from
Oct 10, 2024

Conversation

gergelykarm
Copy link
Contributor

@gergelykarm gergelykarm commented Oct 9, 2024

Description

Fix the const default value for the driver schema jsons.

PR checklist

Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.

@gilles-peskine-arm gilles-peskine-arm added bug needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review component-psa PSA keystore/dispatch layer (storage, drivers, …) size-xs Estimated task size: extra small (a few hours at most) labels Oct 9, 2024
@gilles-peskine-arm
Copy link
Contributor

Thanks! Since you discovered this as a user, I think this should have a changelog entry. Incidentally, how did you discover it? Because the script throws an error on your valid JSON file? (We should have better tests, but I'm afraid this won't happen for a while, and we don't even have the infrastructure to add a non-regression test.)

@gilles-peskine-arm gilles-peskine-arm added needs-backports Backports are missing or are pending review and approval. priority-high High priority - will be reviewed soon labels Oct 9, 2024
@gergelykarm
Copy link
Contributor Author

@gilles-peskine-arm yes I noticed it, while I was using the schemah, for creating a json file. It throws an error if the previous suggested const value is between bracket, because it is supposed to be string by the schema. If it is just between the quotes, it will show a warning, that it should be the predefined const value between brackets.

@gergelykarm
Copy link
Contributor Author

In both of the examples it is only shown as a simple string, and not as an array.

@gergelykarm
Copy link
Contributor Author

I added a changelog.

Comment on lines 2 to 3
* Fix type definition's default value in driver_*_schema.json. Also removes
warning, if the right string definition is used.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The audience of changelog entries is users. The entry currently describes the content of the patch, but not what it means from a user's perspective. Something like this:

Fix a warning about an invalid JSON schema when running generate_driver_wrappers.py.

Although I can't reproduce the warning here, even though running scripts/generate_driver_wrappers.py with default options should load our bundled drivers (test and p256-m) which do have a "type" property that is incorrectly described in the schema.

Note that I agree that your patch is correct. I'm just wondering why you're getting an error due to the current invalid schema and I'm not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure about how generate_driver_wrappers.py works, and I wasn't talking about seeing a warning at compile time.

I meant, when I'm editing my_driver_tansparent/opaque_driver.json file, and using the cshema in it for help, and autocorrect with LSP (specifying "$schema": "https://github.com/Mbed-TLS/mbedtls/blob/development/ scripts/data_files/driver_jsons/driver_transparent_schema.json" at the begining) with vscode-langservers-extracted, it will automatically set the "type": as ["transparent"], but show an error about the type being string not an array. After deleting the brackets and setting "type": "transparent" it will show a warning about type should be ["transparent"].

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see, thanks. The information I was missing was that some other tool complained. Our Python script just calls Python's jsonschema package to load the schema, and I guess that package doesn't validate the schema itself. I've filed a backlog issue about checking the validity of the schemas.

So can you please change the changelog entry to something that just says the schema was invalid? E.g.

Fix invalid JSON schemas for driver descriptions used by generate_driver_wrappers.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@tom-cosgrove-arm
Copy link
Contributor

I've kicked the CI

Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update!

Can you please submit the same patch to the mbedtls-3.6 branch?

@gilles-peskine-arm gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Oct 10, 2024
@gergelykarm
Copy link
Contributor Author

Fix the const default value for the driver schema jsons.

I created a new PR on that branch #9679

@gergelykarm
Copy link
Contributor Author

As I see the CI is failing on the changelog check. I will push a word wrapped version to be in the 79 columns.

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update!

@gilles-peskine-arm gilles-peskine-arm removed the approved Design and code approved - may be waiting for CI or backports label Oct 10, 2024
@gilles-peskine-arm gilles-peskine-arm added the needs-review Every commit must be reviewed by at least two team members, label Oct 10, 2024
@tom-cosgrove-arm tom-cosgrove-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Oct 10, 2024
@tom-cosgrove-arm tom-cosgrove-arm added this pull request to the merge queue Oct 10, 2024
Merged via the queue into Mbed-TLS:development with commit 0b4ccdd Oct 10, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports bug component-psa PSA keystore/dispatch layer (storage, drivers, …) needs-backports Backports are missing or are pending review and approval. priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most)
Development

Successfully merging this pull request may close these issues.

3 participants