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

Remove SQL-FS #1490

Merged
merged 1 commit into from
Apr 27, 2017
Merged

Remove SQL-FS #1490

merged 1 commit into from
Apr 27, 2017

Conversation

jenswi-linaro
Copy link
Contributor

With recent developments in REE-FS SQL-FS has become redundant. This
patch removes SQL-FS.

Tested-by: Jens Wiklander jens.wiklander@linaro.org (QEMU)
Signed-off-by: Jens Wiklander jens.wiklander@linaro.org

@@ -86,8 +86,6 @@
#define TEE_STORAGE_PRIVATE_REE 0x80000000
/* Storage is the Replay Protected Memory Block partition of an eMMC device */
#define TEE_STORAGE_PRIVATE_RPMB 0x80000100
/* Storage is provided by a SQLite database in the normal world filesystem */
#define TEE_STORAGE_PRIVATE_SQL 0x80000200
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should reserve the value so it is not re-used for some other FS later, while still allowing older TAs to be compiled with a newer dev kit. It is easy enough to do: how about:

#define TEE_STORAGE_PRIVATE_SQL \
        _Pragma("GCC warning \"'TEE_STORAGE_PRIVATE_SQL' is deprecated \
(not supported by OP-TEE > 2.4.x)\"") 0x80000200

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reserving the value makes sense, but using a pragma to emit a warning instead of compile error seems a bit over engineered. Lets assume a TA is using this define, when the TA is compiled there will be a warning which is turned into an error since the TA is compiled with -Werror.

How about:

/* Was TEE_STORAGE_PRIVATE_SQL, which isn't supported any longer */
#define TEE_STORAGE_PRIVATE_SQL_RESERVED  0x80000200

Copy link
Contributor

Choose a reason for hiding this comment

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

OK let's leave the burden on TA developers ;-) it's quite unlikely that many people are using SQL FS anyways (hence this PR to begin with).

@@ -48,14 +48,8 @@
*/

/*
* SQLite file system access
*/
#define OPTEE_MSG_RPC_CMD_SQL_FS 8
Copy link
Contributor

Choose a reason for hiding this comment

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

Also keep the SQL IDs as reserved/deprecated? It is API IDs shared with REE.

Copy link
Contributor

Choose a reason for hiding this comment

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

The IDs are used by tee-supplicant and OP-TEE OS only, and you're not supposed to mix different versions of supplicant and OP-TEE OS (or build an older tee-supplicant with include files from a newer OP-TEE os), so I don't think it is really useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reserving still makes sense, this number shouldn't be reused in the nearest future. How about:

/* Was OPTEE_MSG_RPC_CMD_SQL_FS, which isn't supported any longer */
#define OPTEE_MSG_RPC_CMD_SQL_FS_RESERVED	8

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good too.

@jbech-linaro
Copy link
Contributor

jbech-linaro commented Apr 27, 2017

LGTM (I agree with keeping / reserving the SQL FS numbers).
Reviewed-by: Joakim Bech <joakim.bech@linaro.org>

@jforissier
Copy link
Contributor

As Joakim said, assuming we're keeping (reserving) the SQL numbers:

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

With recent developments in REE-FS SQL-FS has become redundant. This
patch removes SQL-FS.

Reviewed-by: Joakim Bech <joakim.bech@linaro.org>
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Tested-by: Jens Wiklander <jens.wiklander@linaro.org> (QEMU)
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
@jenswi-linaro
Copy link
Contributor Author

Comments addressed, rebased, tags applied.

@jforissier jforissier merged commit 455856d into OP-TEE:master Apr 27, 2017
@jenswi-linaro jenswi-linaro deleted the rem_sqlfs branch April 27, 2017 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants