[PATCH crosvm v2] crosvm: support setting guest MAC from tap-fd
This adds a mac= option to crosvm's --tap-fd option. The virtio-net
driver in the guest will read the desired MAC from virtio
configuration space.
See the documentation for VIRTIO_NET_F_MAC in the Virtio spec[1].
[1]: https://docs.oasis-open.org/virtio/virtio/v1.1/virtio-v1.1.html
Thanks-to: Puck Meerburg
On Mon May 17, 2021 at 11:57 AM PDT, Alyssa Ross wrote:
This adds a mac= option to crosvm's --tap-fd option. The virtio-net driver in the guest will read the desired MAC from virtio configuration space.
See the documentation for VIRTIO_NET_F_MAC in the Virtio spec[1].
[1]: https://docs.oasis-open.org/virtio/virtio/v1.1/virtio-v1.1.html
Thanks-to: Puck Meerburg
--- devices/src/virtio/net.rs | 12 ++++++-- src/crosvm.rs | 8 +++-- src/linux.rs | 12 ++++---- src/main.rs | 63 ++++++++++++++++++++++++++++++--------- 4 files changed, 71 insertions(+), 24 deletions(-)
[snip]
diff --git a/src/main.rs b/src/main.rs index 5d02af02f..f8bc0d14e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -28,7 +28,7 @@ use base::{ }; use crosvm::{ argument::{self, print_help, set_arguments, Argument}, - platform, BindMount, Config, DiskOption, Executable, GidMap, SharedDir, TouchDeviceOption, + platform, BindMount, Config, DiskOption, Executable, GidMap, SharedDir, TapFdOptions, TouchDeviceOption, DISK_ID_LEN, }; #[cfg(feature = "gpu")] @@ -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.
+ "TAP FD already used: '{}'", + name + ))); + } + cfg.tap_fd.insert(fd, TapFdOptions { mac }); } #[cfg(feature = "gpu")] "gpu" => { @@ -1644,8 +1679,8 @@ writeback=BOOL - Indicates whether the VM can use writeback caching (default: fa Argument::value("plugin-gid-map-file", "PATH", "Path to the file listing supplemental GIDs that should be mapped in plugin jail. Can be given more than once."), Argument::flag("vhost-net", "Use vhost for networking."), Argument::value("tap-fd", - "fd", - "File descriptor for configured tap device. A different virtual network card will be added each time this argument is given."), + "FD[,mac=MAC]", + "File descriptor for configured tap device. A different virtual network card will be added each time this argument is given. MAC is the MAC address that will be set in the guest."), #[cfg(feature = "gpu")] Argument::flag_or_value("gpu", "[width=INT,height=INT]",
base-commit: f35d2c43ff19520855cffee761dc8899c5a439a1 -- 2.31.1
Else, LGTM.
Reviewed-by: Cole Helbling
"Cole Helbling"
@@ -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/+/f35d2c43ff195... The only other one I can see that might be applicable would be InvalidValue...
On Tue May 18, 2021 at 1:36 AM PDT, Alyssa Ross wrote:
"Cole Helbling"
writes: 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/+/f35d2c43ff195...
The only other one I can see that might be applicable would be InvalidValue...
Yeah, I think InvalidValue would be a better choice (IMHO). The user hasn't provided too many arguments; they just provided a invalid TAP FD because it's already in use.
"Cole Helbling"
On Tue May 18, 2021 at 1:36 AM PDT, Alyssa Ross wrote:
"Cole Helbling"
writes: 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/+/f35d2c43ff195...
The only other one I can see that might be applicable would be InvalidValue...
Yeah, I think InvalidValue would be a better choice (IMHO). The user hasn't provided too many arguments; they just provided a invalid TAP FD because it's already in use.
Hmm, I agree with you, but I just went to make this change, and noticed that everywhere else in the file, they use TooManyArguments for this sort of case. e.g. "wayland socket name already used". So I think it's probably best to stick with TooManyArguments for consistency (and that's probably why I did it like that in the first place, although I don't remember).
On Sat May 29, 2021 at 7:08 AM PDT, Alyssa Ross wrote:
"Cole Helbling"
writes: On Tue May 18, 2021 at 1:36 AM PDT, Alyssa Ross wrote:
"Cole Helbling"
writes: 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/+/f35d2c43ff195...
The only other one I can see that might be applicable would be InvalidValue...
Yeah, I think InvalidValue would be a better choice (IMHO). The user hasn't provided too many arguments; they just provided a invalid TAP FD because it's already in use.
Hmm, I agree with you, but I just went to make this change, and noticed that everywhere else in the file, they use TooManyArguments for this sort of case. e.g. "wayland socket name already used". So I think it's probably best to stick with TooManyArguments for consistency (and that's probably why I did it like that in the first place, although I don't remember).
OK, that's fair.
Reviewed-by: Cole Helbling
participants (2)
-
Alyssa Ross
-
Cole Helbling