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

Add exception vector type #303

Merged
merged 2 commits into from
Sep 13, 2021
Merged

Add exception vector type #303

merged 2 commits into from
Sep 13, 2021

Conversation

npmccallum
Copy link
Member

These embeddable types can be used to parse the various errors.

cc @haraldh

Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request! I think we already have some of these types, but adding the Exception enum sounds good to me (probably under a different name such as ExceptionCode or ExceptionKind).

src/errors.rs Outdated Show resolved Hide resolved
src/errors.rs Outdated Show resolved Hide resolved
src/errors.rs Outdated Show resolved Hide resolved
@npmccallum
Copy link
Member Author

@phil-opp Any chance you could review this promptly? I have a large pile of pull requests for other crates that now depend on this change.

Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Looks good to me overall!

I think we should move the InterruptVector struct to the structures::idt module, to keep the root module clean.

src/structures/idt.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@josephlr josephlr left a comment

Choose a reason for hiding this comment

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

LGTM modulo my and @phil-opp's comments

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@npmccallum
Copy link
Member Author

I think we should move the InterruptVector struct to the structures::idt module, to keep the root module clean.

I'm all for keeping the root module clean. But this type has far-ranging use and isn't idt related. That's why I put it in root. Is there another place it should go?

@npmccallum
Copy link
Member Author

@phil-opp @josephlr I think we're really close to merging this with a minor amount of attention.

Copy link
Contributor

@josephlr josephlr left a comment

Choose a reason for hiding this comment

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

@npmccallum sorry for the delay in review. This now looks good to me.

First thing, after trying to write a little code (mostly in relation to #285), I think that this should be called ExceptionVector (sorry for flip-flopping on this). This is what Linux uses and avoids confusion with external interrupts (32-355) which are explicitly not covered by this type. Then the following code makes more sense:

fn common_exception_handler(ist: InterruptStackFrame, vector: ExceptionVector, error_code: Option<u64>) {
    // Common interrupt handling logic
}

I think we should move the InterruptVector struct to the structures::idt module, to keep the root module clean.

I'm all for keeping the root module clean. But this type has far-ranging use and isn't idt related. That's why I put it in root. Is there another place it should go?

You're correct that this type can be used outside of the IDT; however, that's certainly it's most common use, and where other users would expect to find it. Furthermore, as mentioned above, we might use this type in a common exception handling function, which would be used directly with the IDT.

For these reasons, lets move it to structures::idt. I'm happy to make these changes. After I merge this, we can release 0.14.5.

@josephlr josephlr changed the title Add exception, page fault and selector error types Add exception vector type Sep 12, 2021
@josephlr
Copy link
Contributor

@npmccallum I made the changes, but I can't push them unless you enable the "allow edits from maintainers" option.

Signed-off-by: Joe Richey <joerichey@google.com>
@npmccallum
Copy link
Member Author

@npmccallum I made the changes, but I can't push them unless you enable the "allow edits from maintainers" option.

done

@josephlr josephlr merged commit 79ce148 into rust-osdev:master Sep 13, 2021
@josephlr josephlr mentioned this pull request Sep 13, 2021
josephlr pushed a commit that referenced this pull request Sep 13, 2021
Signed-off-by: Joe Richey <joerichey@google.com>
@josephlr
Copy link
Contributor

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