-
Notifications
You must be signed in to change notification settings - Fork 33
feat: Support multi-format integer parsing for CLI arguments #119
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
Conversation
* add clparser module (including multi-format integer parser) * support multi-format CLI arguments in key subcommand * refactor KeyArgs to use typed defaults and streamline validation logic * update README.md accordingly Signed-off-by: Takuma IMAMURA <[email protected]>
| pub fn parse_int_auto_radix<T>(s: &str) -> Result<T, ParseIntError> | ||
| where | ||
| T: FromStrRadix, | ||
| { | ||
| if let Some(hex) = s.strip_prefix("0x") { | ||
| T::from_str_radix(hex, 16) | ||
| } else if let Some(bin) = s.strip_prefix("0b") { | ||
| T::from_str_radix(bin, 2) | ||
| } else { | ||
| T::from_str_radix(s, 10) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only other format that we might need to consider is Base64. Qemu takes in a lot of base 64 encoded values. That is why in some of the other commands we support that format. Besides that everything else looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion! I agree that support for Base64 is worthwhile. The main challenge is that it is not possible to unambiguously detect Base64 purely from the input string (e.g., a hex literal like 0x00 can also be parsed as valid Base64 bits: 0x00 → 110100 110001 110100 110100).
One approach I would propose is to assign an appropriate default format for each argument type (e.g., binary for bitfields, decimal for plain integers, hex for general byte strings) and then allow overriding via explicit flags such as --lmv-format base64 or --gfs-format hex. This way we can consistently support decimal, hex, binary, Base64 (and potentially Base64URL).
In the case of the key subcommand, the default formats are set as follows:
--vmpl: decimal--guest_field_select: binary--launch_mit_vector: hex
Does that approach make sense to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A point @tylerfanelli brought up is that maybe user's won't be inputting Base64 as much as they would require the output, so maybe we support Base64 outputs, but not necessarily inputs.
@tylerfanelli any thoughts on this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, i suggest that because of different padding formats, etc, its best to just have users input hex. If QEMU depends on base64 inputs, then this tool's outputs can be in base64. But I think it's best to standardize around hex (and thus not needing a 0x prefix to inputs) and not worry about different encoding formats.
DGonzalezVillal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally like that we can support different types of inputs by prefixing the input type with 0x or 0b for hex and binary. It gives the users flexibility. It also helps standarize inputs to the tool. people can use what makes the most sense or is most convenient to them. Unless you just want to do hex all through out the tool @tylerfanelli .
|
@tylerfanelli thoughts? |
I have a hard time believing anyone will be inputting binary. IMO that should ultimately be removed.
Sure, fine with me. |
|
I think for the binary input we were thinking of fields like Guest field select in the key derivation were we have several different inputs values |MIT_VECTOR, TCB VERSION, SVN, ...| were 1 is include and 0 is not include. And so binary is an easier way of inputting that value. |
This PR introduces a custom integer parser able to accept decimal, prefixed hexadecimal (
0x…) and prefixed binary (0b…) strings in thesnpguestCLI, and wires it into thekeysubcommand.This was discussed and proposed in PR #115.
What's changed
src/clparser/mod.rsimplementing the multi-format integer parserparse_int_auto_radix::<T>keysubcommand ― argumentsgfs,gsvn,tcbvandlmvnow useclparser::parse_int_auto_radixas theirvalue_parserKeyArgsto use typed defaults and streamline validation logicREADME.mdto document the new multi-format parsing in CLI examplesTest Environment
All affected CLI arguments (
gfs,gsvn,tcbv,lmv) were tested in this environment and worked as expected. Note:lmvis not supported on GCE SNP VMs (SEV-SNP FW ABI pre-1.58), so the expected behavior is to emit an error.