Skip to content

Fix TTRPC proto format and style issues - #4480

Open
Hamza El-Saawy (helsaawy) wants to merge 2 commits into
microsoft:mainfrom
helsaawy:proto-fmt
Open

Hamza El-Saawy (helsaawy) wants to merge 2 commits into
microsoft:mainfrom
helsaawy:proto-fmt

Conversation

@helsaawy

Copy link
Copy Markdown

Follow protobuff style guide and best practices:

Add Vm to RPC names in VM service that operate on VMs, and rename CapabilitiesVM to Capabilities, since it, along with Quit, operate on the virtstack level.

Breakout out vmservice.proto into smaller files to reduce file size and group together related definitions:

  • vmservice.disk.proto
  • vmservice.nic.proto
  • vmservice.numa.proto
  • vmservice.nvme.proto
  • vmservice.pci.proto
  • vmservice.vfio.proto
  • vmservice.virtio.proto

Update openvmm\openvmm_ttrpc_vmservice\build.rs to emit cargo::rerun-if-changed directives for all *.proto files in src/.

Add .clang-format to enable consistent formatting for *.proto files.
There is currently no xtask or flowey integration for clang-format, but there are editor integrations available to run locally.

Second commit is updates to openvmm\openvmm_entry\src\ttrpc\mod.rs, vmm_tests\vmm_tests\tests\tests\ttrpc.rs, and vmm_tests\vmm_tests\tests\tests\ttrpc\fd_passing.rs.

@helsaawy
Hamza El-Saawy (helsaawy) requested a review from a team as a code owner September 18, 2026 18:05
Copilot AI lite review requested due to automatic review settings September 18, 2026 18:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Critical protobuf wire-value compatibility breaks and a CIDR handling regression remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Updates the TTRPC VM service protobuf schema, Rust callers, tests, build tracking, and formatting configuration.

Changes:

  • Renames RPCs and messages to follow protobuf style guidance.
  • Splits device definitions into focused proto files.
  • Updates service implementation, tests, and protobuf rebuild directives.
  • Adds .clang-format configuration.
File summaries
File Summary
vmm_tests/vmm_tests/tests/tests/ttrpc/fd_passing.rs Updates FD-passing test requests.
vmm_tests/vmm_tests/tests/tests/ttrpc.rs Updates TTRPC integration tests.
openvmm/openvmm_ttrpc_vmservice/src/vmservice.virtio.proto Defines Virtio schema changes.
openvmm/openvmm_ttrpc_vmservice/src/vmservice.vfio.proto Defines VFIO schema changes.
openvmm/openvmm_ttrpc_vmservice/src/vmservice.proto Updates core service and VM definitions.
openvmm/openvmm_ttrpc_vmservice/src/vmservice.pci.proto Defines PCI schema changes.
openvmm/openvmm_ttrpc_vmservice/src/vmservice.nvme.proto Defines NVMe schema changes.
openvmm/openvmm_ttrpc_vmservice/src/vmservice.numa.proto Defines NUMA schema changes.
openvmm/openvmm_ttrpc_vmservice/src/vmservice.nic.proto Defines NIC schema changes.
openvmm/openvmm_ttrpc_vmservice/src/vmservice.disk.proto Defines disk schema changes.
openvmm/openvmm_ttrpc_vmservice/src/lib.rs Updates generated binding documentation.
openvmm/openvmm_ttrpc_vmservice/build.rs Tracks protobuf file changes.
openvmm/openvmm_entry/src/ttrpc/mod.rs Updates service implementation and configuration parsing.
.clang-format Adds protobuf formatting rules.
Review details

Suppressed comments (2)

openvmm/openvmm_entry/src/ttrpc/mod.rs:2201

  • This predicate is reversed: it returns Some("") for an explicitly empty CIDR and drops every non-empty CIDR. That changes the previous behavior (empty means use the default; non-empty must be passed through), so configured Consomme networks lose their CIDR and empty values are treated as invalid/configured. Use filter(|s| !s.is_empty()).
                cidr: cidr.filter(|s| s.is_empty()), // user explicitly passed in an empty string

openvmm/openvmm_ttrpc_vmservice/src/vmservice.proto:455

  • This API rename leaves the Guide stale: Guide/src/reference/openvmm/management/grpc.md:48 still documents AddVpciDeviceRequest.instance_id, while this schema now exposes AddVmVpciDeviceRequest. Update the user-facing API reference in the same change (and search for the other renamed RPC/message identifiers).
message AddVmVpciDeviceRequest {
    // The device to attach. VTL assignment is not modeled; this targets VTL0.
    PciDeviceKind device = 1;
    // Guest-visible VPCI instance ID. Pass this to RemoveVpciDevice to remove
    // the device.
    string instance_id = 2;
  • Files reviewed: 14/14 changed files
  • Comments generated: 11
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +103 to +109
SECURE_BOOT_TEMPLATE_UNSPECIFIED = 0;
// Do not initialize Secure Boot-related variables from a template.
NONE = 0;
SECURE_BOOT_TEMPLATE_NONE = 1;
// Initialize variables with the Microsoft Windows certificate set.
MICROSOFT_WINDOWS = 1;
SECURE_BOOT_TEMPLATE_MICROSOFT_WINDOWS = 2;
// Initialize variables with the Microsoft UEFI CA certificate set.
MICROSOFT_UEFI_CERTIFICATE_AUTHORITY = 2;
SECURE_BOOT_TEMPLATE_MICROSOFT_UEFI_CERTIFICATE_AUTHORITY = 3;
Comment on lines 205 to +212
enum GuestPowerAction {
GUEST_POWER_ACTION_UNSPECIFIED = 0;
// Use the default for this event.
DEFAULT = 0;
GUEST_POWER_ACTION_DEFAULT = 1;
// Restart the guest.
RESTART = 1;
GUEST_POWER_ACTION_RESTART = 2;
// Leave the virtual machine halted.
HALT = 2;
GUEST_POWER_ACTION_HALT = 3;
Comment on lines 346 to +349
enum PropertiesType {
Memory = 0;
Processor = 1;
PROPERTIES_TYPE_UNSPECIFIED = 0;
PROPERTIES_TYPE_MEMORY = 1;
PROPERTIES_TYPE_PROCESSOR = 2;
Comment on lines 355 to +361
enum VmState {
Uninitialized = 0;
Paused = 1;
Running = 2;
Halted = 3;
VM_STATE_UNSPECIFIED = 0;
VM_STATE_UNINITIALIZED = 1;
VM_STATE_PAUSED = 2;
VM_STATE_RUNNING = 3;
VM_STATE_HALTED = 4; // Powered off.
VM_STATE_TORN_DOWN = 5;
Comment on lines +376 to +380
enum ResourceModifyType {
RESOURCE_MODIFY_TYPE_UNSPECIFIED = 0;
RESOURCE_MODIFY_TYPE_ADD = 1;
RESOURCE_MODIFY_TYPE_REMOVE = 2;
RESOURCE_MODIFY_TYPE_UPDATE = 3;
Comment on lines +468 to +490
message CapabilitiesResponse {
enum Resource {
RESOURCE_UNSPECIFIED = 0;
RESOURCE_VPMEM = 1;
RESOURCE_SCSI = 2;
RESOURCE_VPCI = 3;
RESOURCE_PLAN9 = 4;
RESOURCE_VM_NIC = 5;
RESOURCE_MEMORY = 6;
RESOURCE_PROCESSOR = 7;
}

message SupportedResource {
bool Add = 1;
bool Remove = 2;
bool Update = 3;
Resource resource = 4;
}

enum SupportedGuestOs {
SUPPORTED_GUEST_OS_UNSPECIFIED = 0;
SUPPORTED_GUEST_OS_WINDOWS = 1;
SUPPORTED_GUEST_OS_LINUX = 2;
Comment thread openvmm/openvmm_entry/src/ttrpc/mod.rs Outdated
Comment thread openvmm/openvmm_entry/src/ttrpc/mod.rs Outdated
Comment thread openvmm/openvmm_ttrpc_vmservice/src/vmservice.nic.proto
Comment thread openvmm/openvmm_ttrpc_vmservice/src/vmservice.proto

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Critical wire-compatibility breakage and enum value renumbering must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

openvmm/openvmm_ttrpc_vmservice/src/vmservice.proto:106

  • These additions shift every existing SecureBootTemplate value (old NONE=0, MICROSOFT_WINDOWS=1, and MICROSOFT_UEFI_CERTIFICATE_AUTHORITY=2) to new wire values 1–3. The same renumbering occurs for GuestPowerAction, PropertiesType, VmState, ResourceModifyType, DiskType, and SupportedGuestOs, so an older client and this server will decode valid messages as different options. Preserve the established numeric assignments (or introduce an explicitly versioned/aliased compatibility scheme) rather than inserting UNSPECIFIED and shifting them.
            SECURE_BOOT_TEMPLATE_UNSPECIFIED = 0;
            // Do not initialize Secure Boot-related variables from a template.
            SECURE_BOOT_TEMPLATE_NONE = 1;
            // Initialize variables with the Microsoft Windows certificate set.
            SECURE_BOOT_TEMPLATE_MICROSOFT_WINDOWS = 2;
            // Initialize variables with the Microsoft UEFI CA certificate set.
            SECURE_BOOT_TEMPLATE_MICROSOFT_UEFI_CERTIFICATE_AUTHORITY = 3;

openvmm/openvmm_ttrpc_vmservice/src/vmservice.proto:26

  • The public Guide still lists the pre-change RPC names (CreateVM, CapabilitiesVM, PropertiesVM, ModifyResource, etc.) even though this service now exposes the renamed methods. Since that page identifies vmservice.proto as the authoritative API and is the user-facing RPC list, update it in the same change so clients are not directed to obsolete names.
service VM {
    // VM lifecycle & management

    // CreateVm will create the virtual machine with the configuration in the
    // CreateVmRequest. The virtual machine will be in a paused state, power wise,
    // after CreateVm. ResumeVm can be called to transition the VM into a running
    // state.
    rpc CreateVm(CreateVmRequest) returns (google.protobuf.Empty);

openvmm/openvmm_ttrpc_vmservice/src/vmservice.proto:30

  • This updated RPC comment still refers to the old TeardownVM and WaitVM identifiers, while the declarations below are TeardownVm and WaitVm. Keeping the old spellings in the API comments makes generated/API documentation inconsistent with the actual service names.
    // TeardownVM will release all associated resources from the VM and unblock
    // the WaitVM call.
    rpc TeardownVm(google.protobuf.Empty) returns (google.protobuf.Empty);
  • Files reviewed: 14/14 changed files
  • Comments generated: 3
  • Review effort level: Lite

Comment on lines +16 to +18
optional string nic_name = 5;
NicBackend backend = 6;
}
Comment thread openvmm/openvmm_ttrpc_vmservice/src/vmservice.nic.proto
Comment thread openvmm/openvmm_ttrpc_vmservice/build.rs Outdated
Add `.clang-format` to enable formatting for `*.proto` files.
There is currently no `xtask` or flowey integration for `clang-format`,
but there are [editor integrations][clang-fmt] available to run locally.

[Use TitleCase][id_case] for acronyms (e.g., `Vm`, `Scsi`, `Pcie`)
in message and RPC call names.

Add `_UNSPECIFIED` [enum value][enum_unspec] and rename enum values to
be [UPPER_SNAKE_CASE][enum_case], prefixed by enum name.

Start breaking out `vmservice.proto` into smaller files.

Update docs (`Guide\src\reference\openvmm\management\grpc.md`) with
proto changes.

[clang-fmt]: https://clang.llvm.org/docs/ClangFormat.html
[enum_case]: https://protobuf.dev/programming-guides/style/#enums
[enum_unspec]: https://protobuf.dev/best-practices/dos-donts/#unspecified-enum
[id_case]: https://protobuf.dev/programming-guides/style/#identifier

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Wire compatibility regressions and default-value handling issues must be addressed.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (9)

openvmm/openvmm_entry/src/ttrpc/mod.rs:874

  • unwrap_or_default() produces an InitialVariables whose enum field is zero. Because the old zero value was NONE and initial_variables is optional, a UEFI request that omits this block (or omits secure_boot_template) previously selected no template; this new arm now rejects that otherwise valid/default configuration. Preserve the no-template default (or otherwise distinguish field presence before rejecting UNSPECIFIED).
                    (_, vmservice::uefi::initial_variables::SecureBootTemplate::Unspecified) => {
                        bail!("unspecified uefi secure boot template");
                    }

openvmm/openvmm_entry/src/ttrpc/mod.rs:1065

  • guest_power_actions is optional and GuestPowerActions fields default to zero when omitted. After introducing UNSPECIFIED = 0, requests that rely on the documented/default power actions (including the updated test, which leaves shutdown and crash unset) now hit this error instead of using defaults. Treat Unspecified like Default for omitted fields, or validate presence separately.
                    Some(ProtoAction::Unspecified) => bail!("unspecified guest power action"),

openvmm/openvmm_ttrpc_vmservice/src/vmservice.nic.proto:17

  • NicConfig.backend used to be a oneof whose concrete backends occupied fields 2, 6, 7, and 8; it is now a wrapper message at field 6 with new inner fields 1–4. That changes the wire shape, so old clients' NIC configurations will be ignored or decoded incorrectly rather than merely receiving renamed generated types. Preserve the original field numbers/types or introduce a versioned migration.
message NicConfig {
    reserved 2, 4;
    reserved "legacy_port_id", "legacy_switch_id";

    string nic_id      = 1; // GUID
    string mac_address = 3; // 12-34-56-78-9A-BC
    // Optional friendly name for the adapter. Might be useful to show up in logs.
    optional string nic_name = 5;
    NicBackend backend       = 6;

openvmm/openvmm_ttrpc_vmservice/src/vmservice.nic.proto:70

  • This comment still names the removed ModifyResource RPC. The corresponding API is now ModifyVmResource, so update the reference to avoid misleading users of the split proto definitions.
    // Used during initial creation and for dynamic add/remove via ModifyResource.

openvmm/openvmm_ttrpc_vmservice/src/vmservice.proto:106

  • This renumbers every existing SecureBootTemplate value by inserting UNSPECIFIED = 0. A client that sends the previously valid value 0 (NONE) will now be decoded as UNSPECIFIED and rejected, while values 1 and 2 select different templates. Preserve the existing wire numbers for the old values (or version/migrate the protocol explicitly) rather than changing them as part of a style-only update.
            SECURE_BOOT_TEMPLATE_UNSPECIFIED = 0;
            // Do not initialize Secure Boot-related variables from a template.
            SECURE_BOOT_TEMPLATE_NONE = 1;
            // Initialize variables with the Microsoft Windows certificate set.
            SECURE_BOOT_TEMPLATE_MICROSOFT_WINDOWS = 2;
            // Initialize variables with the Microsoft UEFI CA certificate set.
            SECURE_BOOT_TEMPLATE_MICROSOFT_UEFI_CERTIFICATE_AUTHORITY = 3;

openvmm/openvmm_ttrpc_vmservice/src/vmservice.proto:209

  • Adding the unspecified value here shifts the wire values of GuestPowerAction::DEFAULT, RESTART, and HALT from 0/1/2 to 1/2/3. Existing clients' VM creation requests will therefore be interpreted as different power actions; enum names can be prefixed without changing their numeric assignments.
        GUEST_POWER_ACTION_UNSPECIFIED = 0;
        // Use the default for this event.
        GUEST_POWER_ACTION_DEFAULT = 1;
        // Restart the guest.
        GUEST_POWER_ACTION_RESTART = 2;
        // Leave the virtual machine halted.
        GUEST_POWER_ACTION_HALT = 3;

openvmm/openvmm_ttrpc_vmservice/src/vmservice.proto:363

  • The new zero value shifts the existing VmState wire values, so an old client interpreting 0 as Uninitialized will see the new UNSPECIFIED value and all nonzero states are off by one. Keep the existing numeric values and assign only the newly added state a fresh number (with a deliberate policy for unspecified), or version the protocol.
    VM_STATE_UNSPECIFIED   = 0;
    VM_STATE_UNINITIALIZED = 1;
    VM_STATE_PAUSED        = 2;
    VM_STATE_RUNNING       = 3;
    VM_STATE_HALTED        = 4; // Powered off.
    VM_STATE_TORN_DOWN     = 5;

openvmm/openvmm_ttrpc_vmservice/src/vmservice.proto:382

  • This changes the numeric meaning of every existing ResourceModifyType value by inserting a new zero member. An older client sending ADD = 0 will now be treated as UNSPECIFIED and fail to add the resource, while REMOVE/UPDATE are also shifted. Preserve the old numbers or make this an explicitly versioned protocol change.
enum ResourceModifyType {
    RESOURCE_MODIFY_TYPE_UNSPECIFIED = 0;
    RESOURCE_MODIFY_TYPE_ADD         = 1;
    RESOURCE_MODIFY_TYPE_REMOVE      = 2;
    RESOURCE_MODIFY_TYPE_UPDATE      = 3;

openvmm/openvmm_ttrpc_vmservice/src/vmservice.proto:471

  • Both capability enums shift the numeric values used on the wire when UNSPECIFIED is inserted at zero. Existing clients will misread advertised resources and guest OSes (for example, old SCSI = 1 becomes VPMEM, and old LINUX = 1 becomes WINDOWS). Preserve the old numeric assignments and add only new values, or version the API.
        RESOURCE_UNSPECIFIED = 0;
        RESOURCE_VPMEM       = 1;
        RESOURCE_SCSI        = 2;
        RESOURCE_VPCI        = 3;
        RESOURCE_PLAN9       = 4;
        RESOURCE_VM_NIC      = 5;
        RESOURCE_MEMORY      = 6;
        RESOURCE_PROCESSOR   = 7;
    }

    message SupportedResource {
        bool add          = 1;
        bool remove       = 2;
        bool update       = 3;
        Resource resource = 4;
    }

    enum SupportedGuestOs {
        SUPPORTED_GUEST_OS_UNSPECIFIED = 0;
        SUPPORTED_GUEST_OS_WINDOWS     = 1;
        SUPPORTED_GUEST_OS_LINUX       = 2;
  • Files reviewed: 15/15 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment on lines +27 to +31
enum DiskType {
DISK_TYPE_UNSPECIFIED = 0;
DISK_TYPE_VHD1 = 1;
DISK_TYPE_VHDX = 2;
DISK_TYPE_PHYSICAL = 3;
Comment on lines +49 to +52
enum IpProtocol {
IP_PROTOCOL_UNSPECIFIED = 0;
IP_PROTOCOL_TCP = 1;
IP_PROTOCOL_UDP = 2;
@github-actions

Copy link
Copy Markdown

Update `openvmm\openvmm_entry\src\ttrpc\mod.rs`,
`vmm_tests\vmm_tests\tests\tests\ttrpc.rs`, and any other relevant code
that uses the `openvmm_ttrpc_vmservice` crate.

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Moderate protobuf wire-compatibility and legacy-default issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (5)

openvmm/openvmm_entry/src/ttrpc/mod.rs:1073

  • An individual action in a present GuestPowerActions message is still zero when omitted, so adding GUEST_POWER_ACTION_UNSPECIFIED = 0 makes partial requests fail here. Previously an omitted action decoded as DEFAULT and inherited the host default; please handle Unspecified the same way as Default (or use presence-aware fields) to retain that behavior.
                    Some(ProtoAction::Unspecified) => bail!("unspecified guest power action"),

openvmm/openvmm_entry/src/ttrpc/mod.rs:1683

  • IpProtocol::TCP was previously numeric value 0, so omitted protocol fields and messages from older clients decode as TCP. With the new UNSPECIFIED value and this rejection, those requests now fail instead of retaining the existing default behavior. Preserve the old default (or introduce presence/versioning before changing the wire meaning of 0).
    let protocol = match vmservice::IpProtocol::from_i32(protocol) {
        Some(vmservice::IpProtocol::Unspecified) => bail!("unspecified ip protocol"),
        Some(vmservice::IpProtocol::Tcp) => HostPortProtocol::Tcp,
        Some(vmservice::IpProtocol::Udp) => HostPortProtocol::Udp,
        None => anyhow::bail!("invalid protocol {protocol}"),

openvmm/openvmm_ttrpc_vmservice/src/vmservice.nic.proto:17

  • This changes the wire layout of NicConfig, not just its generated names. Previously the backend oneof arms were encoded directly as fields 2/6/7/8; after this change an old client sending tap (7) or consomme (8) is ignored by the new server, and old clients cannot decode the new wrapper at field 6. Preserve the existing field numbers/oneof shape (or explicitly version the RPC) before shipping this schema change.
    NicBackend backend       = 6;

openvmm/openvmm_ttrpc_vmservice/src/vmservice.nic.proto:52

  • These changes renumber existing wire enum values (TCP/UDP were 0/1 and are now 1/2). The same renumbering occurs for the other existing enums in this PR, so an older client sending a previously valid value will now decode as a different value or as UNSPECIFIED (for example, old TCP=0 is rejected). Preserve the established numeric assignments, or version/migrate the protocol explicitly; identifier renames do not require changing wire numbers.
enum IpProtocol {
    IP_PROTOCOL_UNSPECIFIED = 0;
    IP_PROTOCOL_TCP         = 1;
    IP_PROTOCOL_UDP         = 2;

openvmm/openvmm_ttrpc_vmservice/src/vmservice.proto:106

  • The new UNSPECIFIED = 0 entries renumber every existing enum value (and the same pattern is used for GuestPowerAction, VmState, ResourceModifyType, IpProtocol, and DiskType). Enum numbers are the protobuf wire values, so existing clients will have their old ADD/TCP/RUNNING etc. values decoded as different values or rejected. Preserve the published numeric assignments and add a compatible migration/versioning strategy instead of shifting them.
            SECURE_BOOT_TEMPLATE_UNSPECIFIED = 0;
            // Do not initialize Secure Boot-related variables from a template.
            SECURE_BOOT_TEMPLATE_NONE = 1;
            // Initialize variables with the Microsoft Windows certificate set.
            SECURE_BOOT_TEMPLATE_MICROSOFT_WINDOWS = 2;
            // Initialize variables with the Microsoft UEFI CA certificate set.
            SECURE_BOOT_TEMPLATE_MICROSOFT_UEFI_CERTIFICATE_AUTHORITY = 3;
  • Files reviewed: 15/15 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment on lines +872 to +874
(_, vmservice::uefi::initial_variables::SecureBootTemplate::Unspecified) => {
bail!("unspecified uefi secure boot template");
}
@github-actions

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants