Audit C enum for safety and use integer newtypes where needed (#55)

Also provides a general convenience macro for defining such newtypes and an extended discussion of safety contracts in comments.

Author: HadrienG2

src/data_types/enums.rs

@@ -0,0 +1,83 @@
+//! This module provides tooling that facilitates dealing with C-style enums
+//!
+//! C-style enums and Rust-style enums are quite different. There are things
+//! which one allows, but not the other, and vice versa. In an FFI context, two
+//! aspects of C-style enums are particularly bothersome to us:
+//!
+//! - They allow a caller to send back an unknown enum variant. In Rust, the
+//!   mere act of storing such a variant in a variable is undefined behavior.
+//! - They have an implicit conversion to integers, which is often used as a
+//!   more portable alternative to C bitfields or as a way to count the amount
+//!   of variants of an enumerated type. Rust enums do not model this well.
+//!
+//! Therefore, in many cases, C enums are best modeled as newtypes of integers
+//! featuring a large set of associated constants instead of as Rust enums. This
+//! module provides facilities to simplify this kind of FFI.
+
+/// Interface a C-style enum as an integer newtype.
+///
+/// This macro implements Debug for you, the way you would expect it to work on
+/// Rust enums (printing the variant name instead of its integer value). It also
+/// derives Clone, Copy, Eq and PartialEq, since that always makes sense for
+/// C-style enums and is used by the implementation. If you want anything else
+/// to be derived, you can ask for it by adding extra derives as shown in the
+/// example below.
+///
+/// One minor annoyance is that since variants will be translated into
+/// associated constants in a separate impl block, you need to discriminate
+/// which attributes should go on the type and which should go on the impl
+/// block. The latter should go on the right-hand side of the arrow operator.
+///
+/// Usage example:
+/// ```
+/// newtype_enum! {
+/// #[derive(Cmp, PartialCmp)]
+/// pub enum UnixBool: i32 => #[allow(missing_docs)] {
+///     FALSE          =  0,
+///     TRUE           =  1,
+///     /// Nobody expects the Unix inquisition!
+///     FILE_NOT_FOUND = -1,
+/// }}
+/// ```
+#[macro_export]
+macro_rules! newtype_enum {
+    (
+        $(#[$type_attrs:meta])*
+        pub enum $type:ident : $base_integer:ty => $(#[$impl_attrs:meta])* {
+            $(
+                $(#[$variant_attrs:meta])*
+                $variant:ident = $value:expr,
+            )*
+        }
+    ) => {
+        $(#[$type_attrs])*
+        #[repr(transparent)]
+        #[derive(Clone, Copy, Eq, PartialEq)]
+        pub struct $type($base_integer);
+
+        $(#[$impl_attrs])*
+        #[allow(unused)]
+        impl $type {
+            $(
+                $(#[$variant_attrs])*
+                pub const $variant: $type = $type($value);
+            )*
+        }
+
+        impl core::fmt::Debug for $type {
+            fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result {
+                match *self {
+                    // Display variants by their name, like Rust enums do
+                    $(
+                        $type::$variant => write!(f, stringify!($variant)),
+                    )*
+
+                    // Display unknown variants in tuple struct format
+                    $type(unknown) => {
+                        write!(f, "{}({})", stringify!($type), unknown)
+                    }
+                }
+            }
+        }
+    }
+}

src/data_types/mod.rs

@@ -12,3 +12,6 @@ pub struct Event(*mut c_void);
 
 mod guid;
 pub use self::guid::Guid;
+
+#[macro_use]
+mod enums;

src/error/status.rs

@@ -1,38 +1,23 @@
-// The error codes are unportable, but that's how the spec defines them.
-#![allow(clippy::enum_clike_unportable_variant)]
-
 use super::Result;
 use core::ops;
 use ucs2;
 
+/// Bit indicating that an UEFI status code is an error
+const ERROR_BIT: usize = 1 << (core::mem::size_of::<usize>() * 8 - 1);
+
+newtype_enum! {
 /// UEFI uses status codes in order to report successes, errors, and warnings.
 ///
 /// Unfortunately, the spec allows and encourages implementation-specific
 /// non-portable status codes. Therefore, these cannot be modeled as a Rust
 /// enum, as injecting an unknown value in a Rust enum is undefined behaviour.
 ///
 /// For lack of a better option, we therefore model them as a newtype of usize.
-#[derive(Debug, Copy, Clone, Eq, PartialEq)]
-#[repr(transparent)]
 #[must_use]
-pub struct Status(usize);
-
-/// Macro to make implementation of status codes easier
-macro_rules! status_codes {
-    (   $(  $(#[$attr:meta])*
-            $status:ident = $code:expr, )*
-    ) => {
-        #[allow(unused)]
-        impl Status {
-            $(  $(#[$attr])*
-                pub const $status: Status = Status($code); )*
-        }
-    }
-}
-//
-status_codes! {
+pub enum Status: usize => {
     /// The operation completed successfully.
     SUCCESS                 =  0,
+
     /// The string contained characters that could not be rendered and were skipped.
     WARN_UNKNOWN_GLYPH      =  1,
     /// The handle was closed, but the file was not deleted.
@@ -47,94 +32,77 @@ status_codes! {
     WARN_FILE_SYSTEM        =  6,
     /// The operation will be processed across a system reset.
     WARN_RESET_REQUIRED     =  7,
-}
-
-/// Bit indicating that a status code is an error
-const ERROR_BIT: usize = 1 << (core::mem::size_of::<usize>() * 8 - 1);
 
-/// Macro to make implementation of error codes easier
-macro_rules! error_codes {
-    (   $(  $(#[$attr:meta])*
-            $status:ident = $error_code:expr, )*
-    ) => {
-        status_codes! { $(
-            $(#[$attr])*
-            $status = $error_code | ERROR_BIT,
-        )* }
-    }
-}
-//
-error_codes! {
     /// The image failed to load.
-    LOAD_ERROR              =  1,
+    LOAD_ERROR              = ERROR_BIT |  1,
     /// A parameter was incorrect.
-    INVALID_PARAMETER       =  2,
+    INVALID_PARAMETER       = ERROR_BIT |  2,
     /// The operation is not supported.
-    UNSUPPORTED             =  3,
+    UNSUPPORTED             = ERROR_BIT |  3,
     /// The buffer was not the proper size for the request.
-    BAD_BUFFER_SIZE         =  4,
+    BAD_BUFFER_SIZE         = ERROR_BIT |  4,
     /// The buffer is not large enough to hold the requested data.
     /// The required buffer size is returned in the appropriate parameter.
-    BUFFER_TOO_SMALL        =  5,
+    BUFFER_TOO_SMALL        = ERROR_BIT |  5,
     /// There is no data pending upon return.
-    NOT_READY               =  6,
+    NOT_READY               = ERROR_BIT |  6,
     /// The physical device reported an error while attempting the operation.
-    DEVICE_ERROR            =  7,
+    DEVICE_ERROR            = ERROR_BIT |  7,
     /// The device cannot be written to.
-    WRITE_PROTECTED         =  8,
+    WRITE_PROTECTED         = ERROR_BIT |  8,
     /// A resource has run out.
-    OUT_OF_RESOURCES        =  9,
+    OUT_OF_RESOURCES        = ERROR_BIT |  9,
     /// An inconstency was detected on the file system.
-    VOLUME_CORRUPTED        = 10,
+    VOLUME_CORRUPTED        = ERROR_BIT | 10,
     /// There is no more space on the file system.
-    VOLUME_FULL             = 11,
+    VOLUME_FULL             = ERROR_BIT | 11,
     /// The device does not contain any medium to perform the operation.
-    NO_MEDIA                = 12,
+    NO_MEDIA                = ERROR_BIT | 12,
     /// The medium in the device has changed since the last access.
-    MEDIA_CHANGED           = 13,
+    MEDIA_CHANGED           = ERROR_BIT | 13,
     /// The item was not found.
-    NOT_FOUND               = 14,
+    NOT_FOUND               = ERROR_BIT | 14,
     /// Access was denied.
-    ACCESS_DENIED           = 15,
+    ACCESS_DENIED           = ERROR_BIT | 15,
     /// The server was not found or did not respond to the request.
-    NO_RESPONSE             = 16,
+    NO_RESPONSE             = ERROR_BIT | 16,
     /// A mapping to a device does not exist.
-    NO_MAPPING              = 17,
+    NO_MAPPING              = ERROR_BIT | 17,
     /// The timeout time expired.
-    TIMEOUT                 = 18,
+    TIMEOUT                 = ERROR_BIT | 18,
     /// The protocol has not been started.
-    NOT_STARTED             = 19,
+    NOT_STARTED             = ERROR_BIT | 19,
     /// The protocol has already been started.
-    ALREADY_STARTED         = 20,
+    ALREADY_STARTED         = ERROR_BIT | 20,
     /// The operation was aborted.
-    ABORTED                 = 21,
+    ABORTED                 = ERROR_BIT | 21,
     /// An ICMP error occurred during the network operation.
-    ICMP_ERROR              = 22,
+    ICMP_ERROR              = ERROR_BIT | 22,
     /// A TFTP error occurred during the network operation.
-    TFTP_ERROR              = 23,
+    TFTP_ERROR              = ERROR_BIT | 23,
     /// A protocol error occurred during the network operation.
-    PROTOCOL_ERROR          = 24,
+    PROTOCOL_ERROR          = ERROR_BIT | 24,
     /// The function encountered an internal version that was
     /// incompatible with a version requested by the caller.
-    INCOMPATIBLE_VERSION    = 25,
+    INCOMPATIBLE_VERSION    = ERROR_BIT | 25,
     /// The function was not performed due to a security violation.
-    SECURITY_VIOLATION      = 26,
+    SECURITY_VIOLATION      = ERROR_BIT | 26,
     /// A CRC error was detected.
-    CRC_ERROR               = 27,
+    CRC_ERROR               = ERROR_BIT | 27,
     /// Beginning or end of media was reached
-    END_OF_MEDIA            = 28,
+    END_OF_MEDIA            = ERROR_BIT | 28,
     /// The end of the file was reached.
-    END_OF_FILE             = 31,
+    END_OF_FILE             = ERROR_BIT | 31,
     /// The language specified was invalid.
-    INVALID_LANGUAGE        = 32,
+    INVALID_LANGUAGE        = ERROR_BIT | 32,
     /// The security status of the data is unknown or compromised and
     /// the data must be updated or replaced to restore a valid security status.
-    COMPROMISED_DATA        = 33,
+    COMPROMISED_DATA        = ERROR_BIT | 33,
     /// There is an address conflict address allocation
-    IP_ADDRESS_CONFLICT     = 34,
+    IP_ADDRESS_CONFLICT     = ERROR_BIT | 34,
     /// A HTTP error occurred during the network operation.
-    HTTP_ERROR              = 35,
-}
+    HTTP_ERROR              = ERROR_BIT | 35,
+}}
 
 impl Status {
     /// Returns true if status code indicates success.

src/lib.rs

@@ -32,12 +32,13 @@
 #![warn(missing_docs, unused)]
 #![deny(clippy::all)]
 
-mod error;
-pub use self::error::{Result, Status};
-
+#[macro_use]
 mod data_types;
 pub use self::data_types::{Event, Guid, Handle};
 
+mod error;
+pub use self::error::{Result, Status};
+
 pub mod table;
 
 pub mod proto;

src/proto/console/gop.rs

@@ -295,7 +295,7 @@ struct ModeData {
 #[repr(u32)]
 pub enum PixelFormat {
     /// Each pixel is 32-bit long, with 24-bit RGB, and the last byte is reserved.
-    RGB,
+    RGB = 0,
     /// Each pixel is 32-bit long, with 24-bit BGR, and the last byte is reserved.
     BGR,
     /// Custom pixel format, check the associated bitmask.
@@ -305,6 +305,10 @@ pub enum PixelFormat {
     /// This means you will have to use the `blt` function which will
     /// convert the graphics data to the device's internal pixel format.
     BltOnly,
+    // SAFETY: UEFI also defines a PixelFormatMax variant, and states that all
+    //         valid enum values are guaranteed to be smaller. Since that is the
+    //         case, adding a new enum variant would be a breaking change, so it
+    //         is safe to model this C enum as a Rust enum.
 }
 
 /// Bitmask used to indicate which bits of a pixel represent a given color.

src/proto/console/serial.rs

@@ -210,6 +210,9 @@ pub enum Parity {
     Mark,
     /// Space parity
     Space,
+    // SAFETY: The serial protocol is very old, and new parity modes are very
+    //         unlikely to be added at this point in time. Therefore, modeling
+    //         this C enum as a Rust enum seems safe.
 }
 
 /// Number of stop bits per character.
@@ -224,4 +227,7 @@ pub enum StopBits {
     OneFive,
     /// 2 stop bits
     Two,
+    // SAFETY: The serial protocol is very old, and new stop bit modes are very
+    //         unlikely to be added at this point in time. Therefore, modeling
+    //         this C enum as a Rust enum seems safe.
 }