Merge pull request #92 from rust-osdev/stricter-code-quality-ci

Code style improvements + optional CI job (clippy, rustfmt, rustdoc)

Author: phip1611

.github/workflows/rust.yml

@@ -20,3 +20,18 @@ jobs:
       run: cargo build --verbose
     - name: Run tests
       run: cargo test --verbose
+
+
+  # As discussed, these tasks are optional for PRs.
+  style_checks:
+
+    runs-on: ubuntu-latest
+
+    steps:
+    - uses: actions/checkout@v2
+    - name: Rustfmt
+      run: cargo fmt -- --check
+    - name: Clippy
+      run: cargo clippy
+    - name: Rustdoc
+      run: cargo doc

.travis.yml

@@ -11,11 +11,10 @@ use core::marker::PhantomData;
 pub const MULTIBOOT2_BOOTLOADER_MAGIC: u32 = 0x36d76289;
 
 /// Possible types of a Tag in the Multiboot2 Information Structure (MBI), therefore the value
-/// of the the `typ` field in [`Tag`]. The names and values are taken from the example C code
+/// of the the `typ` property. The names and values are taken from the example C code
 /// at the bottom of the Multiboot2 specification.
 #[repr(u32)]
 #[derive(Copy, Clone, Debug)]
-#[allow(missing_docs)]
 pub enum TagType {
     /// Marks the end of the tags.
     End = 0,

src/header.rs

@@ -1,6 +1,14 @@
 #![no_std]
 #![deny(missing_debug_implementations)]
-#![deny(missing_docs)]
+// --- BEGIN STYLE CHECKS ---
+// These checks are optional in CI for PRs, as discussed in
+// https://github.com/rust-osdev/multiboot2/pull/92
+#![deny(clippy::all)]
+#![deny(rustdoc::all)]
+// Forcing this would be a little bit ridiculous, because it would require code examples for
+// each getter and each trivial trait implementation (like Debug).
+#![allow(rustdoc::missing_doc_code_examples)]
+// --- END STYLE CHECKS ---
 
 //! Library that helps you to parse the multiboot information structure (mbi) from
 //! Multiboot2-compliant bootloaders, like GRUB. It supports all tags from the specification
@@ -9,15 +17,15 @@
 //!
 //! The GNU Multiboot(2) specification aims to provide a standardised
 //! method of sharing commonly used information about the host machine at
-//! boot time and give the payload, i.e. a kernel, a well defined machien
+//! boot time and give the payload, i.e. a kernel, a well defined machine
 //! state.
 //!
 //! ## Example
 //!
-//! ```ignore
-//! use multiboot::load;
+//! ```rust
+//! use multiboot2::load;
 //! fn kmain(multiboot_info_ptr: u32) {
-//!     let boot_info = unsafe { load(ptr as usize).unwrap() };
+//!     let boot_info = unsafe { load(multiboot_info_ptr as usize).unwrap() };
 //!     println!("{:?}", boot_info);
 //! }
 //! ```
@@ -26,23 +34,21 @@ use core::fmt;
 
 pub use boot_loader_name::BootLoaderNameTag;
 pub use command_line::CommandLineTag;
+pub use efi::{EFIImageHandle32, EFIImageHandle64, EFISdt32, EFISdt64};
 pub use elf_sections::{
     ElfSection, ElfSectionFlags, ElfSectionIter, ElfSectionType, ElfSectionsTag,
 };
 pub use framebuffer::{FramebufferColor, FramebufferField, FramebufferTag, FramebufferType};
 pub use header::TagType;
 pub use header::MULTIBOOT2_BOOTLOADER_MAGIC;
 use header::{Tag, TagIter};
+pub use image_load_addr::ImageLoadPhysAddr;
 pub use memory_map::{
     EFIMemoryAreaType, EFIMemoryDesc, EFIMemoryMapTag, MemoryArea, MemoryAreaIter, MemoryAreaType,
     MemoryMapTag,
 };
 pub use module::{ModuleIter, ModuleTag};
-pub use rsdp::{
-    RsdpV1Tag, RsdpV2Tag,
-};
-pub use image_load_addr::ImageLoadPhysAddr;
-pub use efi::{EFIImageHandle32, EFIImageHandle64, EFISdt32, EFISdt64};
+pub use rsdp::{RsdpV1Tag, RsdpV2Tag};
 pub use vbe_info::{
     VBECapabilities, VBEControlInfo, VBEDirectColorAttributes, VBEField, VBEInfoTag,
     VBEMemoryModel, VBEModeAttributes, VBEModeInfo, VBEWindowAttributes,
@@ -53,46 +59,56 @@ extern crate bitflags;
 
 mod boot_loader_name;
 mod command_line;
+mod efi;
 mod elf_sections;
 mod framebuffer;
 mod header;
+mod image_load_addr;
 mod memory_map;
 mod module;
 mod rsdp;
 mod vbe_info;
-mod efi;
-mod image_load_addr;
 
 /// Load the multiboot boot information struct from an address.
 ///
 /// This is the same as `load_with_offset` but the offset is omitted and set
 /// to zero.
 ///
-/// Examples
+/// ## Example
 ///
-/// ```ignore
-/// use multiboot::load;
+/// ```rust
+/// use multiboot2::load;
 ///
 /// fn kmain(multiboot_info_ptr: u32) {
-///     let boot_info = unsafe { load(ptr as usize).unwrap() };
+///     let boot_info = unsafe { load(multiboot_info_ptr as usize).unwrap() };
 ///     println!("{:?}", boot_info);
 /// }
 /// ```
+///
+/// ## Safety
+/// This function might terminate the program, if the address is invalid. This can be the case in
+/// environments with standard environment (segfault) but also in UEFI-applications,
+/// where the referenced memory is not (identity) mapped (UEFI does only identity mapping).
 pub unsafe fn load(address: usize) -> Result<BootInformation, MbiLoadError> {
     load_with_offset(address, 0)
 }
 
 /// Load the multiboot boot information struct from an address at an offset.
 ///
-/// Examples
+/// ## Example
 ///
 /// ```ignore
-/// use multiboot::load_with_offset;
+/// use multiboot2::load_with_offset;
 ///
-/// let ptr = 0xDEADBEEF as *const _;
+/// let ptr = 0xDEADBEEF as *const u32;
 /// let boot_info = unsafe { load_with_offset(ptr as usize, 0xCAFEBABE).unwrap() };
 /// println!("{:?}", boot_info);
 /// ```
+///
+/// ## Safety
+/// This function might terminate the program, if the address is invalid. This can be the case in
+/// environments with standard environment (segfault) but also in UEFI-applications,
+/// where the referenced memory is not (identity) mapped (UEFI does only identity mapping).
 pub unsafe fn load_with_offset(
     address: usize,
     offset: usize,

src/lib.rs

@@ -1,5 +1,5 @@
-use core::marker::PhantomData;
 use crate::TagType;
+use core::marker::PhantomData;
 
 /// This Tag provides an initial host memory map.
 ///
@@ -24,7 +24,8 @@ pub struct MemoryMapTag {
 impl MemoryMapTag {
     /// Return an iterator over all AVAILABLE marked memory areas.
     pub fn memory_areas(&self) -> impl Iterator<Item = &MemoryArea> {
-        self.all_memory_areas().filter(|entry| matches!(entry.typ, MemoryAreaType::Available))
+        self.all_memory_areas()
+            .filter(|entry| matches!(entry.typ, MemoryAreaType::Available))
     }
 
     /// Return an iterator over all marked memory areas.

src/memory_map.rs

@@ -24,7 +24,10 @@ impl ModuleTag {
         use core::{mem, slice, str};
         let strlen = self.size as usize - mem::size_of::<ModuleTag>();
         unsafe {
-            str::from_utf8_unchecked(slice::from_raw_parts(&self.cmdline_str as *const u8, strlen))
+            str::from_utf8_unchecked(slice::from_raw_parts(
+                &self.cmdline_str as *const u8,
+                strlen,
+            ))
         }
     }
 
@@ -47,9 +50,9 @@ impl ModuleTag {
 impl Debug for ModuleTag {
     fn fmt(&self, f: &mut Formatter<'_>) -> core::fmt::Result {
         f.debug_struct("ModuleTag")
-            .field("type", &self.typ)
-            .field("size (tag)", &self.size)
-            .field("size (module)", &(self.module_size()))
+            .field("type", &{ self.typ })
+            .field("size (tag)", &{ self.size })
+            .field("size (module)", &self.module_size())
             .field("mod_start", &(self.mod_start as *const usize))
             .field("mod_end", &(self.mod_end as *const usize))
             .field("cmdline", &self.cmdline())

src/module.rs

@@ -8,9 +8,9 @@
 //!
 //! Even though the bootloader should give the address of the real RSDP/XSDT, the checksum and
 //! signature should be manually verified.
+use crate::TagType;
 use core::slice;
 use core::str;
-use crate::TagType;
 
 const RSDPV1_LENGTH: usize = 20;
 

src/rsdp.rs

@@ -1,5 +1,5 @@
-use core::fmt;
 use crate::TagType;
+use core::fmt;
 
 /// This tag contains VBE metadata, VBE controller information returned by the
 /// VBE Function 00h and VBE mode information returned by the VBE Function 01h.