patches and low-level development discussion
 help / color / mirror / code / Atom feed
* [PATCH crosvm v2] crosvm: support setting guest MAC from tap-fd
@ 2021-05-17 18:57 Alyssa Ross
  2021-05-17 19:20 ` Cole Helbling
  0 siblings, 1 reply; 6+ messages in thread
From: Alyssa Ross @ 2021-05-17 18:57 UTC (permalink / raw)
  To: devel; +Cc: Cole Helbling

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 <puck@puckipedia.com>
---
v1: https://spectrum-os.org/lists/archives/spectrum-devel/20210409222026.13886-1-hi@alyssa.is/

This is a new version of the patch, updated for a much more recent
crosvm.  I'm posting it again because I had to change it enough that I
feel it could use a re-review.

The biggest change is that I gave up on a NetOptions struct, since
upstream has added a new option, and it seems that their approach is
going to be just passing ever more function arguments around.  No use
fighting that for my single addition.  Also, change from RawFd to
crosvm's RawDescriptor.  It would be nice if we could use
SafeDescriptor earlier here (which represents fd ownership), but I
don't think that's really easily doable.

 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(-)

diff --git a/devices/src/virtio/net.rs b/devices/src/virtio/net.rs
index 92368c440..4d0ea1560 100644
--- a/devices/src/virtio/net.rs
+++ b/devices/src/virtio/net.rs
@@ -419,6 +419,7 @@ where
 }
 
 pub struct Net<T: TapT> {
+    mac_address: Option<MacAddress>,
     queue_sizes: Box<[u16]>,
     workers_kill_evt: Vec<Event>,
     kill_evts: Vec<Event>,
@@ -439,6 +440,7 @@ where
         ip_addr: Ipv4Addr,
         netmask: Ipv4Addr,
         mac_addr: MacAddress,
+        guest_mac_addr: Option<MacAddress>,
         vq_pairs: u16,
     ) -> Result<Net<T>, NetError> {
         let multi_queue = if vq_pairs > 1 { true } else { false };
@@ -450,12 +452,12 @@ where
 
         tap.enable().map_err(NetError::TapEnable)?;
 
-        Net::from(base_features, tap, vq_pairs)
+        Net::with_tap(base_features, tap, vq_pairs, guest_mac_addr)
     }
 
     /// Creates a new virtio network device from a tap device that has already been
     /// configured.
-    pub fn from(base_features: u64, tap: T, vq_pairs: u16) -> Result<Net<T>, NetError> {
+    pub fn with_tap(base_features: u64, tap: T, vq_pairs: u16, mac_address: Option<MacAddress>) -> Result<Net<T>, NetError> {
         let taps = tap.into_mq_taps(vq_pairs).map_err(NetError::TapOpen)?;
 
         // This would also validate a tap created by Self::new(), but that's a good thing as it
@@ -488,7 +490,12 @@ where
             workers_kill_evt.push(worker_kill_evt);
         }
 
+        if mac_address.is_some() {
+            avail_features |= 1 << virtio_net::VIRTIO_NET_F_MAC;
+        }
+
         Ok(Net {
+            mac_address,
             queue_sizes: vec![QUEUE_SIZE; (vq_pairs * 2 + 1) as usize].into_boxed_slice(),
             workers_kill_evt,
             kill_evts,
@@ -503,6 +510,7 @@ where
         let vq_pairs = self.queue_sizes.len() as u16 / 2;
 
         VirtioNetConfig {
+            mac: self.mac_address.map(|m| m.octets()).unwrap_or_else(Default::default),
             max_vq_pairs: Le16::from(vq_pairs),
             // Other field has meaningful value when the corresponding feature
             // is enabled, but all these features aren't supported now.
diff --git a/src/crosvm.rs b/src/crosvm.rs
index 04e267d6c..1dde7d769 100644
--- a/src/crosvm.rs
+++ b/src/crosvm.rs
@@ -171,6 +171,10 @@ impl Default for SharedDir {
     }
 }
 
+pub struct TapFdOptions {
+    pub mac: Option<net_util::MacAddress>,
+}
+
 /// Aggregate of all configurable options for a running VM.
 pub struct Config {
     pub vcpu_count: Option<usize>,
@@ -194,7 +198,7 @@ pub struct Config {
     pub mac_address: Option<net_util::MacAddress>,
     pub net_vq_pairs: Option<u16>,
     pub vhost_net: bool,
-    pub tap_fd: Vec<RawFd>,
+    pub tap_fd: BTreeMap<RawFd, TapFdOptions>,
     pub cid: Option<u64>,
     pub wayland_socket_paths: BTreeMap<String, PathBuf>,
     pub wayland_dmabuf: bool,
@@ -253,7 +257,7 @@ impl Default for Config {
             mac_address: None,
             net_vq_pairs: None,
             vhost_net: false,
-            tap_fd: Vec::new(),
+            tap_fd: BTreeMap::new(),
             cid: None,
             #[cfg(feature = "gpu")]
             gpu_parameters: None,
diff --git a/src/linux.rs b/src/linux.rs
index f452fef3f..42c07df4f 100644
--- a/src/linux.rs
+++ b/src/linux.rs
@@ -75,7 +75,7 @@ use vm_memory::{GuestAddress, GuestMemory};
 
 #[cfg(all(target_arch = "x86_64", feature = "gdb"))]
 use crate::gdb::{gdb_thread, GdbStub};
-use crate::{Config, DiskOption, Executable, SharedDir, SharedDirKind, TouchDeviceOption};
+use crate::{Config, DiskOption, Executable, SharedDir, SharedDirKind, TapFdOptions, TouchDeviceOption};
 use arch::{
     self, LinuxArch, RunnableLinuxVm, SerialHardware, SerialParameters, VcpuAffinity,
     VirtioDeviceStub, VmComponents, VmImage,
@@ -675,7 +675,7 @@ fn create_balloon_device(cfg: &Config, socket: BalloonControlResponseSocket) ->
     })
 }
 
-fn create_tap_net_device(cfg: &Config, tap_fd: RawDescriptor) -> DeviceResult {
+fn create_tap_net_device(cfg: &Config, tap_fd: RawDescriptor, options: &TapFdOptions) -> DeviceResult {
     // Safe because we ensure that we get a unique handle to the fd.
     let tap = unsafe {
         Tap::from_raw_descriptor(
@@ -691,7 +691,7 @@ fn create_tap_net_device(cfg: &Config, tap_fd: RawDescriptor) -> DeviceResult {
         vq_pairs = 1;
     }
     let features = virtio::base_features(cfg.protected_vm);
-    let dev = virtio::Net::from(features, tap, vq_pairs).map_err(Error::NetDeviceNew)?;
+    let dev = virtio::Net::with_tap(features, tap, vq_pairs, options.mac).map_err(Error::NetDeviceNew)?;
 
     Ok(VirtioDeviceStub {
         dev: Box::new(dev),
@@ -725,7 +725,7 @@ fn create_net_device(
         .map_err(Error::VhostNetDeviceNew)?;
         Box::new(dev) as Box<dyn VirtioDevice>
     } else {
-        let dev = virtio::Net::<Tap>::new(features, host_ip, netmask, mac_address, vq_pairs)
+        let dev = virtio::Net::<Tap>::new(features, host_ip, netmask, mac_address, None, vq_pairs)
             .map_err(Error::NetDeviceNew)?;
         Box::new(dev) as Box<dyn VirtioDevice>
     };
@@ -1311,8 +1311,8 @@ fn create_virtio_devices(
     devs.push(create_balloon_device(cfg, balloon_device_socket)?);
 
     // We checked above that if the IP is defined, then the netmask is, too.
-    for tap_fd in &cfg.tap_fd {
-        devs.push(create_tap_net_device(cfg, *tap_fd)?);
+    for (tap_fd, options) in &cfg.tap_fd {
+        devs.push(create_tap_net_device(cfg, *tap_fd, options)?);
     }
 
     if let (Some(host_ip), Some(netmask), Some(mac_address)) =
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!(
+                    "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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH crosvm v2] crosvm: support setting guest MAC from tap-fd
  2021-05-17 18:57 [PATCH crosvm v2] crosvm: support setting guest MAC from tap-fd Alyssa Ross
@ 2021-05-17 19:20 ` Cole Helbling
  2021-05-18  8:36   ` Alyssa Ross
  0 siblings, 1 reply; 6+ messages in thread
From: Cole Helbling @ 2021-05-17 19:20 UTC (permalink / raw)
  To: Alyssa Ross, devel; +Cc: Cole Helbling

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 <puck@puckipedia.com>
> ---
>  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.e.helbling@outlook.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH crosvm v2] crosvm: support setting guest MAC from tap-fd
  2021-05-17 19:20 ` Cole Helbling
@ 2021-05-18  8:36   ` Alyssa Ross
  2021-05-18 15:43     ` Cole Helbling
  0 siblings, 1 reply; 6+ messages in thread
From: Alyssa Ross @ 2021-05-18  8:36 UTC (permalink / raw)
  To: Cole Helbling; +Cc: devel

[-- 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 --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH crosvm v2] crosvm: support setting guest MAC from tap-fd
  2021-05-18  8:36   ` Alyssa Ross
@ 2021-05-18 15:43     ` Cole Helbling
  2021-05-29 14:08       ` Alyssa Ross
  0 siblings, 1 reply; 6+ messages in thread
From: Cole Helbling @ 2021-05-18 15:43 UTC (permalink / raw)
  To: Alyssa Ross; +Cc: devel

On Tue May 18, 2021 at 1:36 AM PDT, Alyssa Ross wrote:
> "Cole Helbling" <cole.e.helbling@outlook.com> 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/+/f35d2c43ff19520855cffee761dc8899c5a439a1/src/argument.rs#49
>
> 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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH crosvm v2] crosvm: support setting guest MAC from tap-fd
  2021-05-18 15:43     ` Cole Helbling
@ 2021-05-29 14:08       ` Alyssa Ross
  2021-05-30  2:20         ` Cole Helbling
  0 siblings, 1 reply; 6+ messages in thread
From: Alyssa Ross @ 2021-05-29 14:08 UTC (permalink / raw)
  To: Cole Helbling; +Cc: devel

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

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

> On Tue May 18, 2021 at 1:36 AM PDT, Alyssa Ross wrote:
>> "Cole Helbling" <cole.e.helbling@outlook.com> 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/+/f35d2c43ff19520855cffee761dc8899c5a439a1/src/argument.rs#49
>>
>> 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).

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH crosvm v2] crosvm: support setting guest MAC from tap-fd
  2021-05-29 14:08       ` Alyssa Ross
@ 2021-05-30  2:20         ` Cole Helbling
  0 siblings, 0 replies; 6+ messages in thread
From: Cole Helbling @ 2021-05-30  2:20 UTC (permalink / raw)
  To: Alyssa Ross; +Cc: devel

On Sat May 29, 2021 at 7:08 AM PDT, Alyssa Ross wrote:
> "Cole Helbling" <cole.e.helbling@outlook.com> writes:
>
> > On Tue May 18, 2021 at 1:36 AM PDT, Alyssa Ross wrote:
> >> "Cole Helbling" <cole.e.helbling@outlook.com> 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/+/f35d2c43ff19520855cffee761dc8899c5a439a1/src/argument.rs#49
> >>
> >> 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 <cole.e.helbling@outlook.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-05-30  2:21 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-17 18:57 [PATCH crosvm v2] crosvm: support setting guest MAC from tap-fd Alyssa Ross
2021-05-17 19:20 ` Cole Helbling
2021-05-18  8:36   ` Alyssa Ross
2021-05-18 15:43     ` Cole Helbling
2021-05-29 14:08       ` Alyssa Ross
2021-05-30  2:20         ` Cole Helbling

Code repositories for project(s) associated with this public inbox

	https://spectrum-os.org/git/crosvm
	https://spectrum-os.org/git/doc
	https://spectrum-os.org/git/mktuntap
	https://spectrum-os.org/git/nixpkgs
	https://spectrum-os.org/git/spectrum
	https://spectrum-os.org/git/ucspi-vsock
	https://spectrum-os.org/git/www

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).