search

rust-vmm / vm-fdt

Apache License 2.0
14 stars 16 forks source link

Add `FdtWriter::property_cstring` #72

Open nuta opened 5 months ago

nuta commented 5 months ago

Summary of the PR

Since FdtWriter only accept &str, it does a validation to make sure it's NULL-terminated and copies the whole string in Cstring::new().

This is unnecessary in some circumstances. For example, for "bootargs" property, linux_loader can create CString [1]. However, library users need to convert it into String [2].

This patches adds another property helper property_cstring, which accepts a &CStr value directly to eliminate the overhead.

[1] https://docs.rs/linux-loader/latest/linux_loader/cmdline/struct.Cmdline.html#method.as_cstring [2] https://github.com/firecracker-microvm/firecracker/blob/3853362520b81efc8ce6559148d023379a5a4da4/src/vmm/src/arch/aarch64/fdt.rs#L240-L246

Requirements

Before submitting your PR, please make sure you addressed the following requirements:

nuta commented 5 months ago

All added/changed functionality has a corresponding unit/integration test.

I've skipped adding tests because the new method is covered through property_string(&mut self, &str), but lmk if we should add another one 😃

nuta commented 5 months ago

@danielverkamp Good point I was wondering that too. The problem is convenient traits such as TryInto for &CStr/&str conversion are not implemented in the standard library.

BTW, the review requires at least 2 approvers. Whom should I ask for another review? 🙏

danielverkamp commented 4 months ago

BTW, the review requires at least 2 approvers. Whom should I ask for another review? 🙏

Sorry, I missed this notification - I think it needs someone from https://github.com/rust-vmm/community/blob/main/MAINTAINERS.md#gatekeepers (I haven't been involved much lately, but it seems like there is supposed to be some automation for this, possibly not enabled for this repo though?)

nuta commented 4 months ago

Thank you for pointing it out! I'll ask for another review in a community channel.