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

macOS '.DS_Store' files #131

Merged
merged 26 commits into from
Apr 18, 2019
Merged

macOS '.DS_Store' files #131

merged 26 commits into from
Apr 18, 2019

Conversation

SkypLabs
Copy link
Contributor

Hello,

This PR aims to add a description for the macOS .DS_Store files generated by the Finder. You can test the description on these files: ds_store_samples.zip

There are two things I would like to improve though:

Could you please give me a hand on these two issues?

Thank you in advance.

@SkypLabs SkypLabs changed the title macOS '.DS_Store' files [WIP] macOS '.DS_Store' files Mar 28, 2019
@GreyCat
Copy link
Member

GreyCat commented Mar 28, 2019

Thanks for this PR! I haven't yet done a through review, but I've noticed that you use ofs_xxx, which follows style guide, but other IDs are not very consistent — i.e. you use xxx_size, xxx_len, length, etc, instead of len_xxx, xxx_count, counter, counter, etc, instead of num_xxxs. Could you consider using consistent names, as per style guide?

The position of certain instances need to be calculated according to alignment_header but I didn't succeed to make it work. I had to manually add 4 to the result as in this example: buddy_allocator_header.ofs_bookkeeping_info_block + 4

Um, sorry, I don't quite follow what you mean here. You want some offsets to be automatically adjusted by size of alignment_header, which is a fixed 4 bytes? If yes, probably you can benefit from kaitai-io/kaitai_struct#84, when it will be available.

When I try to use _index lines 65 and 66...

Try splitting it into separate type, i.e.:

    instances:
      directories:
        io: _root._io
        type: master_block_ref(_index)
        repeat: expr
        repeat-expr: directory_count
        doc: Master blocks of the different B-trees
types:
  master_block_ref:
    params:
      - id: idx
        type: u8
    instances:
      master_block:
        pos: (block_addresses[directory_entries[idx].block_id] >> 0x05 << 0x05) + 4
        size: 1 << block_addresses[directory_entries[idx].block_id] & 0x1f
        type: master_block

@SkypLabs
Copy link
Contributor Author

I haven't yet done a through review, but I've noticed that you use ofs_xxx, which follows style guide, but other IDs are not very consistent — i.e. you use xxx_size, xxx_len, length, etc, instead of len_xxx, xxx_count, counter, counter, etc, instead of num_xxxs. Could you consider using consistent names, as per style guide?

Of course, thanks for catching that. Actually, I read the style guide and renamed some of the fields accordingly. For example, I searched for occurrences of length and renamed xxx_length to len_xxx. Same for offset. I missed the xxx_size ones and similar fields. It should be fixed now.

Um, sorry, I don't quite follow what you mean here. You want some offsets to be automatically adjusted by size of alignment_header, which is a fixed 4 bytes? If yes, probably you can benefit from kaitai-io/kaitai_struct#84, when it will be available.

Actually, alignment_header has a static size. This is why it is fine to use + 4 in my calculus. However, I was wondering if there is a more elegant way to explicitly refer to this field in pos.

Try splitting it into separate type

I will try and let you know. Thanks.

@GreyCat
Copy link
Member

GreyCat commented Mar 29, 2019

if there is a more elegant way

There will be, once we'll get sizeof-style things done.

@SkypLabs
Copy link
Contributor Author

SkypLabs commented Apr 7, 2019

Try splitting it into separate type

It works well now 👍

@GreyCat
Copy link
Member

GreyCat commented Apr 7, 2019

Ready for to be merged?

@SkypLabs
Copy link
Contributor Author

SkypLabs commented Apr 7, 2019

If you're satisfied of my work, yes ☺️

@SkypLabs SkypLabs changed the title [WIP] macOS '.DS_Store' files macOS '.DS_Store' files Apr 7, 2019
macos/ds_store.ksy Outdated Show resolved Hide resolved
macos/ds_store.ksy Outdated Show resolved Hide resolved
macos/ds_store.ksy Outdated Show resolved Hide resolved
macos/ds_store.ksy Outdated Show resolved Hide resolved
macos/ds_store.ksy Outdated Show resolved Hide resolved
macos/ds_store.ksy Outdated Show resolved Hide resolved
macos/ds_store.ksy Outdated Show resolved Hide resolved
macos/ds_store.ksy Outdated Show resolved Hide resolved
macos/ds_store.ksy Outdated Show resolved Hide resolved
macos/ds_store.ksy Outdated Show resolved Hide resolved
macos/ds_store.ksy Outdated Show resolved Hide resolved
@SkypLabs
Copy link
Contributor Author

Thanks @KOLANICH and @GreyCat for your awesome review! It was also really instructive. I've learnt a lot about your project.

@KOLANICH
Copy link
Contributor

Thank you for your contribution ;)

@GreyCat GreyCat merged commit d831d97 into kaitai-io:master Apr 18, 2019
@GreyCat
Copy link
Member

GreyCat commented Apr 18, 2019

Just merged in, thanks for this awesome work!

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.

None yet

3 participants