std.os.uefi.protocol: ziggify function signatures #23214

dotcarmen · 2025-03-12T13:03:15Z

A lot of the function signatures in std.os.uefi.protocol aren't very ziggy. This PR modifies a significant number of the namespace's functions to align more with verbose Zig code.

Some parameters changed from * to *const and vice-versa as appropriate
Some pairs of parameters that represent slices were consolidated into a single slice parameter
Status errors associated with each function according to the UEFI Specification are handled specifically, resulting in narrow error unions being returned from each function
- unexpected status codes result in a new call to uefi.unexpectedError, which is akin to posix.unexpectedError
As appropriate, return values from various functions are returned directly when the underlying function's return value is .success, rather than requiring an output pointer parameter

note: most of std.os.uefi is untested, and that's not really changing here. I'm mostly focused on making sure there are no compiler errors. Any further problems will require a future PR to address.

dotcarmen · 2025-03-12T13:16:07Z

one problem I'm struggling to address in this PR is the potential for the underlying API to return status codes - most of std.os.uefi already assumes non-.success return values are errors, but I wonder if warnings (high bit clear) should be stored somewhere?

lib/std/os/uefi.zig

lib/std/os/uefi/protocol/absolute_pointer.zig

lib/std/os/uefi/protocol/device_path.zig

lib/std/os/uefi/protocol/edid.zig

linusg · 2025-03-13T16:10:24Z

lib/std/os/uefi/protocol/file.zig

+    pub fn delete(self: *File) bool {
+        switch (self._delete(self)) {
+            .success => return true,
+            .warn_delete_failure => return false,


That seems like a good solution. Once we find an API that can return both a value and a warning it'll get awkward though.

lib/std/os/uefi/protocol/edid.zig

lib/std/os/uefi/protocol/file.zig

lib/std/os/uefi/protocol/graphics_output.zig

lib/std/os/uefi/protocol/absolute_pointer.zig

lib/std/os/uefi/protocol/device_path.zig

lib/std/os/uefi/protocol/ip6.zig

dotcarmen · 2025-03-15T17:11:58Z

lib/std/os/uefi/protocol/ip6_config.zig

+        data_type: DataType,
+        data_size: usize,
+        data: *const anyopaque,


the data type should probably be a union (enum) eventually, but i'm concerned about how that would affect getData where the out pointer is the field defined by data_type from the union

if #23214 (comment) is good to go, then i can do something similar here...

dotcarmen · 2025-03-15T17:15:01Z

lib/std/os/uefi/protocol/ip6_service_binding.zig

+    pub const CreateChildError = uefi.UnexpectedError || error{
+        InvalidParameter,
+        OutOfResources,
+    } || Error; // TODO: according to the spec, _any other_ status is returnable?


i only did a little bit of google, but couldn't figure out what Other means in the spec. i have a feeling it's referring to warnings, but i'm wary of assuming such and i think i'd rather let somebody who encounters this file their own bug report/pr to fix...

dotcarmen · 2025-03-15T17:16:10Z

lib/std/os/uefi/protocol/managed_network.zig

+            // .invalid_parameter => return Error.InvalidParameter,
+            // .unsupported => return Error.Unsupported,
+            // .not_started => return Error.NotStarted,
+            else => |status| {
+                try status.err();
+                // TODO: only warnings get here I think?
+                return uefi.unexpectedStatus(status);


depending on how we decide on approaching Other above, i'll change the logic here and below

dotcarmen · 2025-03-15T17:21:47Z

for warnings, i'm considering a global pub var warning: Status = .success; in uefi.zig, which the user can check for a value. then, uefi.unexpectedStatus can return UnexpectedError!void and callers can return .success value.

alternatively, I can make a pub fn MaybeWarned(T: type) type which has an optional warning: ?Status field in addition to result: T. thoughts?

dotcarmen · 2025-03-16T09:17:25Z

lib/std/os/uefi/protocol/graphics_output.zig

+        if (size_of_info != @sizeOf(Mode.Info))
+            return error.Unexpected


also thoughts on this pattern? the 3rd parameter is for informing the caller of the implementation's sizeOf(Mode.Info) even though the struct is well-defined according to the spec...

this also applies to SimpleNetwork.statistics

even though the struct is well-defined according to the spec...

I assume it's expected to change across spec versions:

The size of the Info structure should never be assumed and the value of SizeOfInfo is the only valid way to know the size of Info.

Assuming that they would only add fields over time I think we can disregard the size we're told and simply return our definition of Mode.Info.

dotcarmen · 2025-03-17T01:59:33Z

as mentioned in the last commit message, i've finished the initial refactorings :D I don't have time for a self-review right now, I'll get to it later today (I'm traveling in Japan for another couple days)

dotcarmen · 2025-03-17T07:34:02Z

lib/std/os/uefi/protocol.zig

 pub const Udp6 = @import("protocol/udp6.zig").Udp6;

 pub const HiiDatabase = @import("protocol/hii_database.zig").HiiDatabase;
 pub const HiiPopup = @import("protocol/hii_popup.zig").HiiPopup;

+pub fn ServiceBinding(service_guid: uefi.Guid) type {


there's more ServiceBinding types, and none of them provide more specific info about errors, nor do I see any of them have more functionality...

dotcarmen · 2025-03-17T07:35:13Z

lib/std/os/uefi/protocol.zig

+        pub const CreateChildError = uefi.UnexpectedError || error{
+            InvalidParameter,
+            OutOfResources,
+        } || Error; // TODO: according to the spec, _any other_ status is returnable?
+        pub const DestroyChildError = uefi.UnexpectedError || error{
+            Unsupported,
+            InvalidParameter,
+            AccessDenied,
+        } || Error; // TODO: according to the spec, _any other_ status is returnable?
+
+        pub fn createChild(self: *Self) CreateChildError!Handle {
+            var handle: Handle = null;
+            switch (self._create_child(self, &handle)) {
+                .success => handle,
+                // .invalid_parameter => error.InvalidParameter,
+                // .out_of_resources => error.OutOfResources,
+                else => |status| {
+                    try status.err();
+                    // TODO: only warnings get here???
+                    return uefi.unexpectedStatus(status);
+                },
+            }
+        }


#23214 (comment) moved to the code here

dotcarmen · 2025-03-17T07:36:55Z

lib/std/os/uefi/protocol.zig

+        pub fn createChild(self: *Self) CreateChildError!Handle {
+            var handle: Handle = null;
+            switch (self._create_child(self, &handle)) {
+                .success => handle,
+                // .invalid_parameter => error.InvalidParameter,
+                // .out_of_resources => error.OutOfResources,
+                else => |status| {
+                    try status.err();
+                    // TODO: only warnings get here???
+                    return uefi.unexpectedStatus(status);
+                },
+            }
+        }
+
+        pub fn addToHandle(self: *Self, handle: Handle) CreateChildError!void {
+            switch (self._create_child(self, &handle)) {
+                .success => {},
+                .invalid_parameter => error.InvalidParameter,
+                .out_of_resources => error.OutOfResources,
+                else => |status| {
+                    try status.err();
+                    // TODO: only warnings get here???
+                    return uefi.unexpectedStatus(status);
+                },
+            }
+        }


i took the liberty of defining 2 different methods where the spec defines 1, i did that in a couple other places too, if that's undesired i can revert

lib/std/os/uefi/protocol/edid.zig

lib/std/os/uefi/protocol/file.zig

lib/std/os/uefi/protocol/ip6.zig

dotcarmen · 2025-03-17T09:15:26Z

lib/std/os/uefi/protocol/simple_network.zig

+    pub fn getStatus(
+        self: *SimpleNetwork,
+        interrupt_status: ?*InterruptStatus,
+        tx_buf: ?*?[*]u8,
+    ) GetStatusError!void {


lol there's 4 different ways to return values here, so i just left it as is...

lib/std/os/uefi/protocol/simple_text_input.zig

dotcarmen · 2025-03-17T09:29:49Z

lib/std/os/uefi/protocol/simple_text_output.zig

+    pub fn outputString(self: *SimpleTextOutput, msg: [*:0]const u16) OutputStringError!bool {
+        switch (self._output_string(self, msg)) {
+            .success => return true,
+            .warn_unknown_glyph => return false,


another instance of a defined warning being possible, luckily this one is also easy...

lib/std/os/uefi/protocol/simple_text_output.zig

getEdid should return all values via return value nitty mcpicky more enum Flag types oops didnt update the signature IpAddress union

linusg

Great work, I really like where this is going 🙂

Please ping me when you consider this done and want a full review.

dotcarmen · 2025-03-17T13:40:52Z

in addition to the specific requests for feedback i pinged you on, i think i'm ready for a full review :) @linusg

dotcarmen · 2025-03-17T13:55:27Z

ah dang, i'm getting some compiler errors as i'm integrating this into my code 😅 sorry if i push while you're reviewing, i'll try to push only once (after i get regular zig build and the zig test artifact built with refAllDeclsRecursive(std.os.uefi))

dotcarmen · 2025-03-17T14:27:16Z

ok fixed the compiler errors :) done pushing for the (japanese) night!

linusg · 2025-03-17T13:50:52Z

lib/std/os/uefi/protocol.zig

@@ -23,16 +26,98 @@ pub const edid = @import("protocol/edid.zig");
 pub const SimpleNetwork = @import("protocol/simple_network.zig").SimpleNetwork;
 pub const ManagedNetwork = @import("protocol/managed_network.zig").ManagedNetwork;

-pub const Ip6ServiceBinding = @import("protocol/ip6_service_binding.zig").Ip6ServiceBinding;
+pub const Ip6ServiceBinding = ServiceBinding(.{


let's move these and the ServiceBinding function to protocol/service_binding.zig please

linusg · 2025-03-17T13:53:16Z

lib/std/os/uefi/protocol.zig

+        pub const CreateChildError = uefi.UnexpectedError || error{
+            InvalidParameter,
+            OutOfResources,
+        } || Error; // TODO: according to the spec, _any other_ status is returnable?


Please make sure all TODO comments are actionable. If the spec says this is the case there's nothing to do here.

linusg · 2025-03-17T14:01:18Z

lib/std/os/uefi/protocol.zig

+                // .invalid_parameter => error.InvalidParameter,
+                // .out_of_resources => error.OutOfResources,


No commmented out code

linusg · 2025-03-17T14:01:26Z

lib/std/os/uefi/protocol.zig

+                // .out_of_resources => error.OutOfResources,
+                else => |status| {
+                    try status.err();
+                    // TODO: only warnings get here???


Unactionable TODO

linusg · 2025-03-17T14:09:17Z

lib/std/os/uefi/protocol/edid.zig

-        edid: *?[*]u8,
-    ) Status {
-        return self._get_edid(self, handle, attributes, edid_size, edid);
+    pub fn getEdid(self: *const Override, handle: Handle) GetEdidError!Edid {


I believe this should take a *const Handle:

Note: the (EFI_HANDLE ) type of the “ChildHandle” parameter is an historical typing error in the UEFI specification. To match existent practice however, implementors and callers of the protocol are now expected to conform to the declaration of the parameter as written. That is, callers must pass the address of an EFI_HANDLE object as “ChildHandle”, and implementors must dereference “ChildHandle” for finding the EFI_HANDLE object.

linusg · 2025-03-17T14:22:06Z

lib/std/os/uefi/protocol/file.zig

+            .read_only = true,
+            .system = true,
+            ._flag = true,


This doesn't look correct, 0x37 is 0b110111, i.e. all known fields but reserved set to true. But as mentioned I think this is unnecessary, this only seems to be used by UEFI implementations to check if the incoming flag matches this bitmask.

linusg · 2025-03-17T14:23:06Z

lib/std/os/uefi/protocol/file.zig


-    pub const efi_file_position_end_of_file: u64 = 0xffffffffffffffff;
+    const efi_file_position_end_of_file: u64 = 0xffffffffffffffff;


Drop efi_file_ (see https://ziglang.org/documentation/master/#Avoid-Redundancy-in-Names)

linusg · 2025-03-17T14:32:18Z

lib/std/os/uefi/protocol/graphics_output.zig

+        if (size_of_info != @sizeOf(Mode.Info))
+            return error.Unexpected


even though the struct is well-defined according to the spec...

I assume it's expected to change across spec versions:

The size of the Info structure should never be assumed and the value of SizeOfInfo is the only valid way to know the size of Info.

Assuming that they would only add fields over time I think we can disregard the size we're told and simply return our definition of Mode.Info.

linusg · 2025-03-17T14:33:07Z

lib/std/os/uefi/protocol/graphics_output.zig

+        blt_buffer: ?[*]BltPixel,
+        blt_operation: BltOperation,
+        source_x: usize,
+        source_y: usize,
+        destination_x: usize,
+        destination_y: usize,
+        width: usize,
+        height: usize,
+        delta: usize,


Seems fine, not necessary to diverge from the original API too much IMO.

linusg · 2025-03-17T14:48:14Z

lib/std/os/uefi/protocol/managed_network.zig

+    // TODO: according to the spec, "Other" errors are also possible for some of these fns,
+    // but i think it's referring to warnings?


I don't think they refer to warnings, this seems to be dependent on the "MNP child driver" (whatever that is) - so we'll have to expect any UEFI error here.

dotcarmen mentioned this pull request Mar 12, 2025

lib/std/os/uefi/protocol/file.zig: change io adapter fns to const ptr #23160

Closed

dotcarmen force-pushed the uefi-fn-signatures branch from 4f65107 to c5ad247 Compare March 12, 2025 13:06

dotcarmen force-pushed the uefi-fn-signatures branch from c5ad247 to 3c4cfa8 Compare March 12, 2025 15:39

std.os.uefi.protocol: ziggify function signatures

29c91b7

dotcarmen force-pushed the uefi-fn-signatures branch from 3c4cfa8 to 29c91b7 Compare March 13, 2025 15:42

linusg reviewed Mar 13, 2025

View reviewed changes

dotcarmen added 2 commits March 14, 2025 01:49

address some feedback

da26164

explicit error unions

7c21d60

linusg reviewed Mar 14, 2025

View reviewed changes

lib/std/os/uefi/protocol/absolute_pointer.zig Outdated Show resolved Hide resolved

lib/std/os/uefi/protocol/device_path.zig Outdated Show resolved Hide resolved

dotcarmen added 3 commits March 15, 2025 12:02

finish unexpectedErr -> unexpectedStatus

3e1aea5

use undefined and null-unwrap in getEdid

5938b73

some more signature updates

65b98d3

dotcarmen commented Mar 15, 2025

View reviewed changes

lib/std/os/uefi/protocol/ip6.zig Outdated Show resolved Hide resolved

dotcarmen commented Mar 15, 2025

View reviewed changes

a few fixes

6fc4b2b

dotcarmen commented Mar 16, 2025

View reviewed changes

dotcarmen added 2 commits March 16, 2025 23:49

almost there...

abf2b0c

finished initial implementations

f535ed9

dotcarmen marked this pull request as ready for review March 17, 2025 01:59

dotcarmen commented Mar 17, 2025

View reviewed changes

dotcarmen added 2 commits March 17, 2025 18:43

various changes from self-review

c2dadaf

getEdid should return all values via return value nitty mcpicky more enum Flag types oops didnt update the signature IpAddress union

File.open - typed mode and create_attributes args

a7b3533

linusg reviewed Mar 17, 2025

View reviewed changes

dotcarmen added 2 commits March 17, 2025 21:19

CreateAttributes => Attributes

08653d2

more updates to ip6

3a558f8

dotcarmen added 2 commits March 17, 2025 22:11

proper simple_text_output attribute type

60f6778

more nitty gritty mcpicky

0b2eb4c

dotcarmen force-pushed the uefi-fn-signatures branch from 93d664a to 0b2eb4c Compare March 17, 2025 13:23

oops, foreground bright and darkgray are the same

5221eec

compiler error fixes

a40d2bf

linusg reviewed Mar 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

std.os.uefi.protocol: ziggify function signatures #23214

std.os.uefi.protocol: ziggify function signatures #23214

dotcarmen commented Mar 12, 2025 •

edited

Loading

dotcarmen commented Mar 12, 2025

linusg Mar 13, 2025

dotcarmen Mar 15, 2025

dotcarmen Mar 17, 2025

dotcarmen Mar 15, 2025

dotcarmen Mar 15, 2025 •

edited

Loading

dotcarmen commented Mar 15, 2025

dotcarmen Mar 16, 2025 •

edited

Loading

linusg Mar 17, 2025

dotcarmen commented Mar 17, 2025

dotcarmen Mar 17, 2025

dotcarmen Mar 17, 2025

dotcarmen Mar 17, 2025

dotcarmen Mar 17, 2025

dotcarmen Mar 17, 2025

linusg left a comment

dotcarmen commented Mar 17, 2025

dotcarmen commented Mar 17, 2025

dotcarmen commented Mar 17, 2025

linusg Mar 17, 2025

linusg Mar 17, 2025

linusg Mar 17, 2025

linusg Mar 17, 2025

linusg Mar 17, 2025

linusg Mar 17, 2025

linusg Mar 17, 2025

linusg Mar 17, 2025

linusg Mar 17, 2025

linusg Mar 17, 2025

		if (size_of_info != @sizeOf(Mode.Info))
		return error.Unexpected

		// .invalid_parameter => error.InvalidParameter,
		// .out_of_resources => error.OutOfResources,


		pub const efi_file_position_end_of_file: u64 = 0xffffffffffffffff;
		const efi_file_position_end_of_file: u64 = 0xffffffffffffffff;

		// TODO: according to the spec, "Other" errors are also possible for some of these fns,
		// but i think it's referring to warnings?

std.os.uefi.protocol: ziggify function signatures #23214

Are you sure you want to change the base?

std.os.uefi.protocol: ziggify function signatures #23214

Conversation

dotcarmen commented Mar 12, 2025 • edited Loading

dotcarmen commented Mar 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotcarmen Mar 15, 2025 • edited Loading

Choose a reason for hiding this comment

dotcarmen commented Mar 15, 2025

dotcarmen Mar 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotcarmen commented Mar 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linusg left a comment

Choose a reason for hiding this comment

dotcarmen commented Mar 17, 2025

dotcarmen commented Mar 17, 2025

dotcarmen commented Mar 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotcarmen commented Mar 12, 2025 •

edited

Loading

dotcarmen Mar 15, 2025 •

edited

Loading

dotcarmen Mar 16, 2025 •

edited

Loading