Fix TTRPC proto format and style issues - #4480
Hamza El-Saawy (helsaawy) wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 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-formatconfiguration.
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. Usefilter(|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:48still documentsAddVpciDeviceRequest.instance_id, while this schema now exposesAddVmVpciDeviceRequest. 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.
| 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; |
| 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; |
| enum PropertiesType { | ||
| Memory = 0; | ||
| Processor = 1; | ||
| PROPERTIES_TYPE_UNSPECIFIED = 0; | ||
| PROPERTIES_TYPE_MEMORY = 1; | ||
| PROPERTIES_TYPE_PROCESSOR = 2; |
| 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; |
| enum ResourceModifyType { | ||
| RESOURCE_MODIFY_TYPE_UNSPECIFIED = 0; | ||
| RESOURCE_MODIFY_TYPE_ADD = 1; | ||
| RESOURCE_MODIFY_TYPE_REMOVE = 2; | ||
| RESOURCE_MODIFY_TYPE_UPDATE = 3; |
| 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; |
f647321 to
d1584b3
Compare
There was a problem hiding this comment.
🟡 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 identifiesvmservice.protoas 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
TeardownVMandWaitVMidentifiers, while the declarations below areTeardownVmandWaitVm. 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
| optional string nic_name = 5; | ||
| NicBackend backend = 6; | ||
| } |
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>
d1584b3 to
3654b00
Compare
There was a problem hiding this comment.
🟡 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 anInitialVariableswhose enum field is zero. Because the old zero value wasNONEandinitial_variablesis optional, a UEFI request that omits this block (or omitssecure_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 rejectingUNSPECIFIED).
(_, vmservice::uefi::initial_variables::SecureBootTemplate::Unspecified) => {
bail!("unspecified uefi secure boot template");
}
openvmm/openvmm_entry/src/ttrpc/mod.rs:1065
guest_power_actionsis optional andGuestPowerActionsfields default to zero when omitted. After introducingUNSPECIFIED = 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 usingdefaults. TreatUnspecifiedlikeDefaultfor omitted fields, or validate presence separately.
Some(ProtoAction::Unspecified) => bail!("unspecified guest power action"),
openvmm/openvmm_ttrpc_vmservice/src/vmservice.nic.proto:17
NicConfig.backendused 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
ModifyResourceRPC. The corresponding API is nowModifyVmResource, 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
SecureBootTemplatevalue by insertingUNSPECIFIED = 0. A client that sends the previously valid value 0 (NONE) will now be decoded asUNSPECIFIEDand 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, andHALTfrom 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
VmStatewire values, so an old client interpreting 0 asUninitializedwill see the newUNSPECIFIEDvalue 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
ResourceModifyTypevalue by inserting a new zero member. An older client sendingADD = 0will now be treated asUNSPECIFIEDand fail to add the resource, whileREMOVE/UPDATEare 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
UNSPECIFIEDis inserted at zero. Existing clients will misread advertised resources and guest OSes (for example, oldSCSI = 1becomesVPMEM, and oldLINUX = 1becomesWINDOWS). 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
| enum DiskType { | ||
| DISK_TYPE_UNSPECIFIED = 0; | ||
| DISK_TYPE_VHD1 = 1; | ||
| DISK_TYPE_VHDX = 2; | ||
| DISK_TYPE_PHYSICAL = 3; |
| enum IpProtocol { | ||
| IP_PROTOCOL_UNSPECIFIED = 0; | ||
| IP_PROTOCOL_TCP = 1; | ||
| IP_PROTOCOL_UDP = 2; |
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>
3654b00 to
3d24ae3
Compare
There was a problem hiding this comment.
🟡 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
GuestPowerActionsmessage is still zero when omitted, so addingGUEST_POWER_ACTION_UNSPECIFIED = 0makes partial requests fail here. Previously an omitted action decoded asDEFAULTand inherited the host default; please handleUnspecifiedthe same way asDefault(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::TCPwas 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 sendingtap(7) orconsomme(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/UDPwere 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 asUNSPECIFIED(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 = 0entries renumber every existing enum value (and the same pattern is used forGuestPowerAction,VmState,ResourceModifyType,IpProtocol, andDiskType). Enum numbers are the protobuf wire values, so existing clients will have their oldADD/TCP/RUNNINGetc. 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
| (_, vmservice::uefi::initial_variables::SecureBootTemplate::Unspecified) => { | ||
| bail!("unspecified uefi secure boot template"); | ||
| } |
Follow protobuff style guide and best practices:
Vm,Scsi,Pcie) in message and RPC call names._UNSPECIFIEDenum valueAdd
Vmto RPC names inVMservice that operate on VMs, and renameCapabilitiesVMtoCapabilities, since it, along withQuit, operate on the virtstack level.Breakout out
vmservice.protointo smaller files to reduce file size and group together related definitions:vmservice.disk.protovmservice.nic.protovmservice.numa.protovmservice.nvme.protovmservice.pci.protovmservice.vfio.protovmservice.virtio.protoUpdate
openvmm\openvmm_ttrpc_vmservice\build.rsto emitcargo::rerun-if-changeddirectives for all*.protofiles insrc/.Add
.clang-formatto enable consistent formatting for*.protofiles.There is currently no
xtaskor flowey integration forclang-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, andvmm_tests\vmm_tests\tests\tests\ttrpc\fd_passing.rs.