Re: [PATCH crosvm v2] crosvm: support setting guest MAC from tap-fd

Alyssa Ross
2021-05-18T08:37:08Z

[-- Attachment #1: Type: text/plain, Size: 3106 bytes --]

"Cole Helbling" <cole.e.helbling@outlook.com> writes:

>> @@ -1319,17 +1319,52 @@ fn set_argument(cfg: &mut Config, name: &str, value: Option<&str>) -> argument::
>> }
>> "vhost-net" => cfg.vhost_net = true,
>> "tap-fd" => {
>> - cfg.tap_fd.push(
>> - value
>> - .unwrap()
>> - .parse()
>> - .map_err(|_| argument::Error::InvalidValue {
>> - value: value.unwrap().to_owned(),
>> - expected: String::from(
>> - "this value for `tap-fd` must be an unsigned integer",
>> - ),
>> - })?,
>> - );
>> + let mut components = value.unwrap().split(',');
>> +
>> + let fd: RawDescriptor = components
>> + .next()
>> + .and_then(|x| x.parse().ok())
>> + .ok_or_else(|| argument::Error::InvalidValue {
>> + value: value.unwrap().to_owned(),
>> + expected: String::from("this value for `tap-fd` must be an unsigned integer"),
>> + })?;
>> +
>> + let mut mac = None;
>> + for c in components {
>> + let mut kv = c.splitn(2, '=');
>> + let (kind, value) = match (kv.next(), kv.next()) {
>> + (Some(kind), Some(value)) => (kind, value),
>> + _ => {
>> + return Err(argument::Error::InvalidValue {
>> + value: c.to_owned(),
>> + expected: String::from("option must be of the form `kind=value`"),
>> + })
>> + }
>> + };
>> + match kind {
>> + "mac" => {
>> + mac = Some(value.parse().map_err(|_| argument::Error::InvalidValue {
>> + value: value.to_owned(),
>> + expected: String::from(
>> + "`mac` needs to be in the form \"XX:XX:XX:XX:XX:XX\"",
>> + ),
>> + })?)
>> + }
>> + _ => {
>> + return Err(argument::Error::InvalidValue {
>> + value: kind.to_owned(),
>> + expected: String::from("unrecognized option"),
>> + })
>> + }
>> + }
>> + }
>> + if cfg.tap_fd.contains_key(&fd) {
>> + return Err(argument::Error::TooManyArguments(format!(
>
> Is there a better Error variant for this? `TooManyArguments` seems not-
> completely-accurate when specifying an already-in-use FD.

Here are all the variants. Do you think there's a better one?

https://chromium.googlesource.com/chromiumos/platform/crosvm/+/f35d2c43ff19520855cffee761dc8899c5a439a1/src/argument.rs#49

The only other one I can see that might be applicable would be
InvalidValue...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]