Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

dotcarmen
Copy link
Contributor

@dotcarmen dotcarmen commented Mar 12, 2025

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
Copy link
Contributor Author

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?

@dotcarmen dotcarmen force-pushed the uefi-fn-signatures branch from c5ad247 to 3c4cfa8 Compare March 12, 2025 15:39
@dotcarmen dotcarmen force-pushed the uefi-fn-signatures branch from 3c4cfa8 to 29c91b7 Compare March 13, 2025 15:42
pub fn delete(self: *File) bool {
switch (self._delete(self)) {
.success => return true,
.warn_delete_failure => return false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Comment on lines +44 to +46
data_type: DataType,
data_size: usize,
data: *const anyopaque,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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...

Comment on lines +91 to +97
// .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);
Copy link
Contributor Author

@dotcarmen dotcarmen Mar 15, 2025

Choose a reason for hiding this comment

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

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

@dotcarmen
Copy link
Contributor Author

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?

Comment on lines +38 to +39
if (size_of_info != @sizeOf(Mode.Info))
return error.Unexpected
Copy link
Contributor Author

@dotcarmen dotcarmen Mar 16, 2025

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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 dotcarmen marked this pull request as ready for review March 17, 2025 01:59
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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

Comment on lines 65 to 87
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);
},
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

#23214 (comment) moved to the code here

Comment on lines 75 to 100
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);
},
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Comment on lines 290 to 294
pub fn getStatus(
self: *SimpleNetwork,
interrupt_status: ?*InterruptStatus,
tx_buf: ?*?[*]u8,
) GetStatusError!void {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Comment on lines +60 to +63
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,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

getEdid should return all values via return value

nitty mcpicky

more enum Flag types

oops didnt update the signature

IpAddress union
Copy link
Collaborator

@linusg linusg left a comment

Choose a reason for hiding this comment

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

Great work, I really like where this is going 🙂

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

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

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

@dotcarmen
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@@ -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(.{
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Comment on lines 80 to 81
// .invalid_parameter => error.InvalidParameter,
// .out_of_resources => error.OutOfResources,
Copy link
Collaborator

Choose a reason for hiding this comment

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

No commmented out code

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

Choose a reason for hiding this comment

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

Unactionable TODO

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

Choose a reason for hiding this comment

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

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.

Comment on lines +281 to +283
.read_only = true,
.system = true,
._flag = true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.


pub const efi_file_position_end_of_file: u64 = 0xffffffffffffffff;
const efi_file_position_end_of_file: u64 = 0xffffffffffffffff;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment on lines +38 to +39
if (size_of_info != @sizeOf(Mode.Info))
return error.Unexpected
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Comment on lines +57 to +65
blt_buffer: ?[*]BltPixel,
blt_operation: BltOperation,
source_x: usize,
source_y: usize,
destination_x: usize,
destination_y: usize,
width: usize,
height: usize,
delta: usize,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Comment on lines +23 to +24
// TODO: according to the spec, "Other" errors are also possible for some of these fns,
// but i think it's referring to warnings?
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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