Skip to content

Conversation

maheeraeron
Copy link
Contributor

This PR simply refactors EfiDiagnostics into a module with multiple files that separate different responsibilities, since this started to bloat after some time

@maheeraeron maheeraeron requested a review from a team as a code owner October 14, 2025 21:36
@Copilot Copilot AI review requested due to automatic review settings October 14, 2025 21:36
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the EfiDiagnostics module from a single large file into a well-organized module structure with separate files for different responsibilities. The refactor improves code maintainability by separating parsing, processing, message accumulation, and formatting logic into distinct modules.

Key changes:

  • Split monolithic diagnostics.rs into a module with four specialized files
  • Created dedicated modules for processor, parser, message accumulator, and formatting logic
  • Maintained all existing functionality while improving code organization

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
vm/devices/firmware/firmware_uefi/src/service/diagnostics/processor.rs Contains core processing logic for EFI diagnostics buffer with error handling
vm/devices/firmware/firmware_uefi/src/service/diagnostics/parser.rs Handles entry parsing utilities for EFI diagnostics buffer
vm/devices/firmware/firmware_uefi/src/service/diagnostics/mod.rs Main module file with public API and re-exports from submodules
vm/devices/firmware/firmware_uefi/src/service/diagnostics/message_accumulator.rs Manages message accumulation state and processing logic
vm/devices/firmware/firmware_uefi/src/service/diagnostics/formatting.rs Contains log formatting and output utilities
vm/devices/firmware/firmware_uefi/src/service/diagnostics.rs Original file completely removed as part of the refactor
Comments suppressed due to low confidence (1)

vm/devices/firmware/firmware_uefi/src/service/diagnostics/processor.rs:1

  • The bytes_read field is being updated inside the entry processing loop, but it should be updated after successfully processing each entry. Currently, it's updated even if the entry processing fails later in the accumulator.
// Copyright (c) Microsoft Corporation.

Co-authored-by: Copilot <[email protected]>
@maheeraeron
Copy link
Contributor Author

I moved around a lot of things, but the core logic should still be the same and functionally there is no difference. If there are areas where I should optimize something or move things around, please LMK

@maheeraeron maheeraeron requested a review from jstarks October 14, 2025 21:50
//! Advanced Logger package, whose relevant types are defined in the Hyper-V
//! specification within the uefi_specs crate.

#![warn(missing_docs)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be needed, it should be the default workspace wide.

smalis-msft
smalis-msft previously approved these changes Oct 15, 2025
Copy link
Contributor

@smalis-msft smalis-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit

@maheeraeron maheeraeron merged commit cb80dff into microsoft:main Oct 15, 2025
49 checks passed
@maheeraeron maheeraeron deleted the user/maheeraeron/efidiagnostics-refactor branch October 15, 2025 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants