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

Refactor - Make FunctionRegistry accept dense indexes #617

Closed
wants to merge 2 commits into from

Conversation

LucasSte
Copy link

@LucasSte LucasSte commented Oct 17, 2024

The idea behind this PR is to allow the registration of syscalls with integers.

}
}

impl<T: Copy + PartialEq + ContextObject> FunctionRegistry<BuiltinFunction<T>> {
Copy link
Author

@LucasSte LucasSte Oct 17, 2024

Choose a reason for hiding this comment

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

I still want to add a few unit tests for these functions.

src/program.rs Outdated Show resolved Hide resolved
pub fn keys(&self) -> Box<dyn Iterator<Item = u32> + '_> {
match self {
FunctionRegistry::Sparse(map) => Box::new(map.keys().copied()),
FunctionRegistry::Dense(vec) => Box::new(1..=(vec.len() as u32)),
Copy link

Choose a reason for hiding this comment

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

Needs to filter out the tombstones

.map(|(key, (name, value))| (*key, (name.as_slice(), *value))),
),
FunctionRegistry::Dense(vec) => {
Box::new(vec.iter().enumerate().map(|(idx, value)| {
Copy link

Choose a reason for hiding this comment

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

Needs to filter out the tombstones

@@ -350,6 +420,41 @@ macro_rules! declare_builtin_function {
};
}

/// Tombstone function for when we call an invalid syscall (e.g. a syscall deactivated through a
Copy link

Choose a reason for hiding this comment

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

"when we call" makes it sound like runtime when it is about verification instead. Maybe "for leaving gaps in a sparse function registry"

vm.context_object_pointer
.consume(vm.previous_instruction_meter - vm.due_insn_count);
}
vm.program_result = ProgramResult::Err(EbpfError::SyscallError(Box::new(
Copy link

Choose a reason for hiding this comment

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

could also add a debug_assert!(false); to indicate that this is never supposed to actually run.

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.

2 participants