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

[RDY] The return of ISO support in CorsixTH #1548

Merged
merged 10 commits into from Jun 12, 2019

Conversation

Projects
None yet
2 participants
@TheCycoONE
Copy link
Member

commented May 29, 2019

Fixes all the bit rot that happened in the ISO code over the years, and possibly some bugs that have always been there.

Everything is functional except movies.

Commits need to be cleaned up and squashed as appropriate.

@TheCycoONE TheCycoONE force-pushed the TheCycoONE:iso-fs-fix branch 3 times, most recently from 935fa2e to 6d99746 May 31, 2019

@TheCycoONE TheCycoONE changed the title [WIP] The return of ISO support in CorsixTH [RDY] The return of ISO support in CorsixTH May 31, 2019

using value_type = const iso_file_entry;
using difference_type = ptrdiff_t;
using pointer = const iso_file_entry*;
using reference = const iso_file_entry&;

This comment has been minimized.

Copy link
@Alberth289346

Alberth289346 May 31, 2019

Contributor

new spiffy type syntax, apparently :)

This comment has been minimized.

Copy link
@TheCycoONE

TheCycoONE May 31, 2019

Author Member

C++ 11 version of a typedef yes. This is the standard C++ 17 (and higher) way of declaring an iterator, since std::iterator is deprecated. Works in C++14 so why not.

if(file_count == 0)
{
if(std::memcmp(aBuffer + 1, "CD001\x01", 6) == 0) {
if(aBuffer[0] == vdt_primary_volume) {

This comment has been minimized.

Copy link
@Alberth289346

Alberth289346 May 31, 2019

Contributor

At other places you change it to if (..., ie with a space behind the keyword. You may want to look for if( and a the usual others (for( and while(). Wiki code style probably also needs adapting then.

This comment has been minimized.

Copy link
@TheCycoONE

TheCycoONE May 31, 2019

Author Member

Not a big fan that I missed these, but I will do a format patch later (soon) to fix the C++ format consistency project wide. Skip for now.

}

void iso_filesystem::trim_identifier_version(const uint8_t* sIdent, uint8_t& iLength)
bool iso_filesystem::filename_compare(const file_metadata& lhs, const file_metadata& rhs)

This comment has been minimized.

Copy link
@Alberth289346

Alberth289346 May 31, 2019

Contributor

compare is a ambiguous, eg equality checking is also comparing. Perhaps filename_sort_compare or filename_orderer ?

This comment has been minimized.

Copy link
@TheCycoONE

TheCycoONE May 31, 2019

Author Member

hmm, existing name, but the functionality was changed. Don't like confusing what it does with where it's used. otoh it's simple enough it could be a lambda. file_path_less?

This comment has been minimized.

Copy link
@TheCycoONE

TheCycoONE May 31, 2019

Author Member

It parallels std::less

Show resolved Hide resolved CorsixTH/Src/iso_fs.cpp Outdated
@Alberth289346

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

Very good work, my comments are just stuff that gets missing when you switch to fixing and understanding code.

@TheCycoONE TheCycoONE changed the title [RDY] The return of ISO support in CorsixTH [RFC] The return of ISO support in CorsixTH May 31, 2019

@TheCycoONE TheCycoONE force-pushed the TheCycoONE:iso-fs-fix branch 7 times, most recently from 57fc81c to 90a8a93 Jun 1, 2019

*/
iso_directory_iterator& operator++() {
directory_ptr += *directory_ptr;
while (*directory_ptr == 0 && directory_ptr < end_ptr) {

This comment has been minimized.

Copy link
@Alberth289346

Alberth289346 Jun 4, 2019

Contributor

Swap the conditions to avoid accessing memory that you're not supposed to access

* returning a copy of the old iterator.
*/
iso_directory_iterator operator++(int) {
iso_directory_iterator o(*this);

This comment has been minimized.

Copy link
@Alberth289346

Alberth289346 Jun 4, 2019

Contributor

o is a not so good variable name, maybe change to old ?

while (*directory_ptr == 0 && directory_ptr < end_ptr) {
++directory_ptr;
}
if (directory_ptr == end_ptr) {

This comment has been minimized.

Copy link
@Alberth289346

Alberth289346 Jun 4, 2019

Contributor

For safety, compare with >= ?

@TheCycoONE TheCycoONE force-pushed the TheCycoONE:iso-fs-fix branch from 193bd6d to 7dca3a6 Jun 5, 2019

@TheCycoONE

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

Added paranoia around the iso_directory_iterator. Force push now simplifies the exception handling in the advance pointer, so no state is changed until past the last fail point instead of trying to recover.

@TheCycoONE

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

Some more documentation and another mistake I noticed.

throw std::runtime_error("The last directory entry was larger than the defined table region.");
}

if (new_dir_ptr < end_ptr) {

This comment has been minimized.

Copy link
@Alberth289346

Alberth289346 Jun 7, 2019

Contributor

Since the first check at line 294 is the same, you can move the comments, the second check, and the throw into here.

This comment has been minimized.

Copy link
@TheCycoONE

TheCycoONE Jun 9, 2019

Author Member

I felt it was clearer this way.

@TheCycoONE TheCycoONE changed the title [RFC] The return of ISO support in CorsixTH [RDY] The return of ISO support in CorsixTH Jun 9, 2019

TheCycoONE added some commits May 1, 2019

Fix ISO handling
* Return the expected output for listFiles

* Do not do the file size sanity check

* Properly return the contents of iso files

* Fix file list on multisector directories
  Was losing the first file of each sector after the first because the loop
  condition was skipping it after the continue.
iso filesystem code cleanup
Use std vector instead of custom implementation for files list
Use c++ strings instead of C string manipulation
Reduce repetitive code by moving into common constructor
Move next directory entry logic to iterator
Formatting
Safer error assignment
Use std:: prefix on FILEs
Fix undefined behaviour in iso-fs
Cannot reinterpret cast bytes to ints due to potential alignment problems.
Allow selecting iso from install dir browser
Also document InstallDirectoryBrowser
Better interface for filesystem.lua
Add documentation to filesystem.lua

Expose fileExists and fileSize functions to lua so that we do not
have to read the contents of an iso file  when we only care if the
file exists.

Doesn't leak into bare lfs calls when checking corruption, so that
function works for ISO as well now.

Explicit function for checking file's existence which doesn't leak
implementation details.
Separate lua bindings for iso_fs into new file.
Use th_lua_* convention that other classes bound to lua use.
Extra paranoia reading the ISO
File entry sizes are now validated.
The iso_directory_iterator now validates thatit does not access outside
of the defined directory table.
More documentation of iso_fs.cpp internals functions
Renames trim_identifier_version to trim_file_id as a more descriptive
name.

@TheCycoONE TheCycoONE force-pushed the TheCycoONE:iso-fs-fix branch from 943ea99 to 17167cc Jun 11, 2019

@TheCycoONE

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2019

Squashed up about half the commits. OK to merge as is now.

@Alberth289346 Alberth289346 merged commit 5452961 into CorsixTH:master Jun 12, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.