-
Notifications
You must be signed in to change notification settings - Fork 378
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
introduce file io routines with unit tests #213
introduce file io routines with unit tests #213
Conversation
Changing the file layout on disk would be a major version bump, as old files would no longer be compatible. Tools themselves could detect this as an invalid file and fix it up on behalf of the user as a possibility, or we could make it a flag. |
3859fc4
to
eda39cc
Compare
We need to think through on when and how we want to version file formats. Many of the files, notably the ones using SaveDataToFile() are just outputting raw single tpm structures that are just raw bytes. I think things like this don't need any format versioning as they are coupled directly to TPM formats and cannot change. I think any other file format, where we save things like algorithm with the key (SaveEccKeyToFile in tpm2_akparse) or writeCrtSecToFile() or saveTpmContextToFile() would need to place a header on it. In summary, raw tpm byte arrays, no header and continue raw output. Complex structures of our own design need the header. |
I think we would also want to kill off ChangeEndian.h during this PR as a separate commit as well. |
src/files.c
Outdated
* version number. Tools can define their own, individual file | ||
* formats as they make sense, but they should always have the header. | ||
*/ | ||
static const UINT32 MAGIC = 0xBADC0DE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing nibble, should be: 0xBADCC0DE
* @return | ||
* True on success, False otherwise. | ||
*/ | ||
static bool readx(FILE *f, UINT8 *data, size_t size) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newline
This isn't ready yet, outside of the code comments I left, and someone else may leave, we should take the time to kill of ChangeEndian.h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, LGTM too. But expect to see changes applied per self-comments and also my comments before merging.
#define FILE_READ(size) \ | ||
bool files_read_##size(FILE *out, UINT##size *data) { \ | ||
BAIL_ON_NULL("FILE", out); \ | ||
BAIL_ON_NULL("data", data); \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong indent in above two lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its declaring a function, the lines are part of the body, so should be indented.
|
||
return test_byte[0] == 0xFF; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe you shall implement string_bytes_endian_convert_x via Micros like what you did for files_read/write_x.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The routine you posted the comment closest to is a run time test to determine endianess. As far as the others, they could be declared as macros if I did a loop, and the loop counter should be small enough for the loop-unroller to unroll them (thus preserving lack of branches). But I am not too worried about those being macro'd up right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I did it anyways +1
Update Cmoka to the version on Xenial. Signed-off-by: William Roberts <william.c.roberts@intel.com>
eda39cc
to
c4a23fc
Compare
Introduce routines in string-bytes.h for detecting endianess and converting endianess. Introduce generic file-io routines that save multibyte 16,32 and 64 bit values as big endian to a FILE stream as well as raw byte read/write routines. Additionally, have routines for reading and writing a file specific header that includes a magic and version field. Include unit tests for these routines. Signed-off-by: William Roberts <william.c.roberts@intel.com>
Drop ChangeEndian.h and introduce proper endianess routines that handle both big and little endian architectures dynamically. Fixes tpm2-software#220 Signed-off-by: William Roberts <william.c.roberts@intel.com>
c4a23fc
to
2533baf
Compare
Introduce routines in string-bytes.h for detecting endianess
and converting endianess.
Introduce generic file-io routines that save multibyte 16,32 and 64
bit values as big endian to a FILE stream as well as raw byte
read/write routines. Additionally, have routines for reading and
writing a file specific header that includes a magic and version
field.
Include unit tests for these routines.
Signed-off-by: William Roberts william.c.roberts@intel.com