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

partition: add repair capabilities to GPT #187

Merged
merged 1 commit into from
Oct 30, 2023

Conversation

adamqqqplay
Copy link
Contributor

Based on #112, and addressing review comments. Thanks to the original author! @thebsdbox

  1. add Repair and Verify interfaces in type table
  2. add test to test opening the GPT image

@deitch
Copy link
Collaborator

deitch commented Oct 30, 2023

@thebsdbox is great, isn't he? Did he do work elsewhere that you are moving in here?

Copy link
Collaborator

@deitch deitch left a comment

Choose a reason for hiding this comment

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

Just 2 style comments.

// potential protective MBR is at LBA0
hasProtectiveMBR := readProtectiveMBR(b[:logicalBlockSize], uint32(header.secondaryHeader))

table := Table{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than copying the fields, since readGptHeader() returns *Table, wouldn't it be easier to just add the data to it and return it? Something like:

table, err := readGptHeader(gpt)
// .. lots of other stuff
table.LogicalSectorSize = logicalBlockSize
table.PhysicalSectorSize = physicalBlockSize
table.ProtectiveMBR = hasProtectiveMBR
table.initialized = true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your suggestion, I updated code.

// GPT starts at LBA1
gpt := b[logicalBlockSize:]
// readGptHeader reads the GPT header from the given byte slice
func readGptHeader(b []byte) (*Table, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suspect lint will complain that this should be readGPTHeader()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

1. add Repair and Verify interfaces in type table
2. add test to test opening the GPT image

Signed-off-by: thebsdbox <[email protected]>
Signed-off-by: Qinqi Qu <[email protected]>
Copy link
Collaborator

@deitch deitch left a comment

Choose a reason for hiding this comment

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

And CI is happy too.

Thank for this @adamqqqplay !

@deitch deitch merged commit 5817922 into diskfs:master Oct 30, 2023
19 checks passed
@adamqqqplay
Copy link
Contributor Author

@deitch Thanks for your review!

I just discovered a new improvement point. Maybe we could merge this two function in the future: tableFromBytes and tableHeaderFromBytes.

@deitch
Copy link
Collaborator

deitch commented Oct 30, 2023

If it is easier, sure. These are private functions, so no worry about impact other than where they exist, and ability to read clearly.

@thebsdbox
Copy link

Apologies, I was always meant to circle back and finish this off :-(

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.

3 participants