Skip to content

tool: separate tree parsing and xml parsing #588

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.

Sign up for GitHub

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

Open
sfulham wants to merge 1 commit into
base: main
Choose a base branch
from
+26 −14
Open

tool: separate tree parsing and xml parsing #588

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions tool/microkit/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
use microkit_tool::argparse;
use microkit_tool::argparse::{Args, ArgsError};
use microkit_tool::build::build_system;
use microkit_tool::sdf::parse;
use microkit_tool::sdf::parse_xml;
use microkit_tool::sdk::Sdk;
use microkit_tool::sel4::Config;
use microkit_tool::util::bail_if_not_exists;
Expand Down Expand Up @@ -54,7 +54,7 @@ fn main() -> Result<(), String> {

let xml: String = fs::read_to_string(system_path).unwrap();

let mut system = match parse(
let mut system = match parse_xml(
system_path.as_path(),
&xml,
&kernel_config,
Expand Down
32 changes: 22 additions & 10 deletions tool/microkit/src/sdf.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ pub struct SdfLocation {
pub col: u32,
}

#[derive(Clone, Copy)]
pub struct SdfAttribute<'a> {
pub name: &'a str,
pub value: &'a str,
Expand Down Expand Up @@ -125,7 +126,7 @@ impl<'a> SdfNode<'a> for roxmltree::Node<'a, '_> {
}
}

pub(crate) struct SystemDescriptionFile<'a> {
pub struct SystemDescriptionFile<'a> {
filename: &'a Path,
}

Expand All @@ -138,7 +139,7 @@ pub struct SystemDescription {
pub domains: Domains,
}

pub fn parse(
pub fn parse_xml(
filename: &Path,
xml: &str,
config: &Config,
Expand All @@ -151,14 +152,6 @@ pub fn parse(

let xml_sdf = SystemDescriptionFile { filename };

let mut root_pds = vec![];
let mut mrs = vec![];
let mut iomaps = vec![];
let mut io_address_space_names = HashSet::new();
let mut iommu_domain_ids = HashSet::new();
let mut iommu_device_identifiers = Vec::new();
let mut channels = vec![];
let mut domains = Domains::default();
let system = doc
.root()
.children()
Expand All @@ -170,6 +163,25 @@ pub fn parse(

let system: &dyn SdfNode = &system;

parse(xml_sdf, system, config, search_paths)
}

pub fn parse(
xml_sdf: SystemDescriptionFile,

@midnightveil midnightveil Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

hm, I think I'd rather than this takes a &Path and constructs XmlSystemDescription internally. not much point exposing that in an external interface if it's pointless.

then can leave off the pub(crate) change.

Speaking of I should probably go through and audit what needs to be public vs not..

@sfulham sfulham Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My main thinking with this was that parse_xml uses it for check_no_text (which to be fair may not actually need it) and didn't really want to have to construct it twice if we don't have to, but ultimately it doesn't really matter either way.

@midnightveil midnightveil Aug 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nah, should be fine to construct again.

It's a struct with one member. The in-memory-layout will (almost certainly) be the same as the member itself.

system: &dyn SdfNode,
config: &Config,
search_paths: &Vec<PathBuf>,
) -> Result<SystemDescription, String> {
let mut root_pds = vec![];
let mut mrs = vec![];
let mut iomaps = vec![];
let mut io_address_space_names = HashSet::new();
let mut iommu_domain_ids = HashSet::new();
let mut iommu_device_identifiers = Vec::new();
let mut channels = vec![];
let mut domains = Domains::default();
let filename = xml_sdf.filename;

// Channels cannot be parsed immediately as they refer to a particular protection domain
// via an index in the list of PDs. This means that we have to parse all PDs first and
// then parse the channels.
Expand Down
4 changes: 2 additions & 2 deletions tool/microkit/tests/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ fn check_success(kernel_config: &sel4::Config, test_name: &str) {
path.push("tests/sdf/");
path.push(test_name);
let sdf = std::fs::read_to_string(path).unwrap();
let parse = sdf::parse(
let parse = sdf::parse_xml(
Path::new(test_name),
&sdf,
kernel_config,
Expand All @@ -131,7 +131,7 @@ fn check_error(kernel_config: &sel4::Config, test_name: &str, expected_err: &str
sdf_path.push("tests/sdf/");
sdf_path.push(test_name);
let sdf = std::fs::read_to_string(sdf_path).unwrap();
let parse_err = sdf::parse(
let parse_err = sdf::parse_xml(
Path::new(test_name),
&sdf,
kernel_config,
Expand Down
Loading