-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
base: master
Are you sure you want to change the base?
Conversation
4f65107
to
c5ad247
Compare
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- |
c5ad247
to
3c4cfa8
Compare
3c4cfa8
to
29c91b7
Compare
pub fn delete(self: *File) bool { | ||
switch (self._delete(self)) { | ||
.success => return true, | ||
.warn_delete_failure => return false, |
There was a problem hiding this comment.
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.
data_type: DataType, | ||
data_size: usize, | ||
data: *const anyopaque, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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? |
There was a problem hiding this comment.
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...
// .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); |
There was a problem hiding this comment.
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
for warnings, i'm considering a global alternatively, I can make a |
if (size_of_info != @sizeOf(Mode.Info)) | ||
return error.Unexpected |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
.
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) |
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 was a problem hiding this comment.
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...
lib/std/os/uefi/protocol.zig
Outdated
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); | ||
}, | ||
} | ||
} |
There was a problem hiding this comment.
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
lib/std/os/uefi/protocol.zig
Outdated
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); | ||
}, | ||
} | ||
} |
There was a problem hiding this comment.
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
pub fn getStatus( | ||
self: *SimpleNetwork, | ||
interrupt_status: ?*InterruptStatus, | ||
tx_buf: ?*?[*]u8, | ||
) GetStatusError!void { |
There was a problem hiding this comment.
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...
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, |
There was a problem hiding this comment.
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
There was a problem hiding this 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.
93d664a
to
0b2eb4c
Compare
in addition to the specific requests for feedback i pinged you on, i think i'm ready for a full review :) @linusg |
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 |
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(.{ |
There was a problem hiding this comment.
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? |
There was a problem hiding this comment.
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.
lib/std/os/uefi/protocol.zig
Outdated
// .invalid_parameter => error.InvalidParameter, | ||
// .out_of_resources => error.OutOfResources, |
There was a problem hiding this comment.
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??? |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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.
.read_only = true, | ||
.system = true, | ||
._flag = true, |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop efi_file_
(see https://ziglang.org/documentation/master/#Avoid-Redundancy-in-Names)
if (size_of_info != @sizeOf(Mode.Info)) | ||
return error.Unexpected |
There was a problem hiding this comment.
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
.
blt_buffer: ?[*]BltPixel, | ||
blt_operation: BltOperation, | ||
source_x: usize, | ||
source_y: usize, | ||
destination_x: usize, | ||
destination_y: usize, | ||
width: usize, | ||
height: usize, | ||
delta: usize, |
There was a problem hiding this comment.
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.
// TODO: according to the spec, "Other" errors are also possible for some of these fns, | ||
// but i think it's referring to warnings? |
There was a problem hiding this comment.
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.
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.*
to*const
and vice-versa as appropriateuefi.unexpectedError
, which is akin toposix.unexpectedError
.success
, rather than requiring an output pointer parameternote: 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.