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

Derive Default for Time #38

Merged
merged 1 commit into from Jul 8, 2022
Merged

Derive Default for Time #38

merged 1 commit into from Jul 8, 2022

Conversation

Ayush1325
Copy link
Contributor

Time cannot be a null pointer when passing to GetTime. Thus there needs
to be a default initialization.

Signed-off-by: Ayush Singh ayushsingh1325@gmail.com

@Ayush1325 Ayush1325 changed the title Derive Debug for Time Derive Default for Time Jul 7, 2022
Time cannot be a null pointer when passing to GetTime. Thus there needs
to be a default initialization.

Signed-off-by: Ayush Singh <ayushsingh1325@gmail.com>
@dvdhrm
Copy link
Member

dvdhrm commented Jul 8, 2022

Time cannot be a null pointer when passing to GetTime. Thus there needs to be a default initialization.

Can you elaborate? How does a NULL-Pointer relate to default initialization?

I would imagine using GetTime() with MaybeUninit() and calling assume_init() once GetTime() returns with success?

Generally, I don't mind the Default implementation. However, did you check whether the spec references zero-initializing Time anywhere? Does it apply any meaning to it, like being Invalid?

@Ayush1325
Copy link
Contributor Author

Under the Status Code Returned in GetTime(), this is preset:
EFI_INVALID_PARAMETER: Time is NULL.

This basically means that unless the Time parameter is initialized before being passed to the function, it well will just return an error. I also tried using it (Basically passing an uninitialized Time) and well, I got an error until I tried initializing a Time with everything 0 beforehand.

@dvdhrm
Copy link
Member

dvdhrm commented Jul 8, 2022

Under the Status Code Returned in GetTime(), this is preset: EFI_INVALID_PARAMETER: Time is NULL.

This basically means that unless the Time parameter is initialized before being passed to the function, it well will just return an error.

Sorry, but this is not true. Your quote from the specification states that if you pass NULL, then EFI_INVALID_PARAMETER is returned. It does not talk about passing valid pointers to non-zeroed memory. The specification even has examples that call into GetTime() without clearing the memory. See 4.7.1 for instance. So according to the spec, this should work fine without clearing the Time structure.
Also, have a look at the 8.3 SetTime() definition. There the spec explicitly states that the function performs error-checking on the Time parameter ("Full error checking is performed on the different fields of the EFI_TIME structure")

Now this does not mean that implementations follow this. But it is a crucial difference whether the spec mandates something, or whether implementations violate the spec and we try to adapt.

I also tried using it (Basically passing an uninitialized Time) and well, I got an error until I tried initializing a Time with everything 0 beforehand.

This is really weird. So you are saying passing the same structure to GetTime() twice in a row will yield an error? What error? Why would the GetTime() implementation read the structure at all?

Anyway, if you happen to run across an UEFI firmware implementation that fails in GetTime() unless the output structure is cleared to all zero, I think this is a crucial thing to document. The spec seems to be clear that all-zero Time is invalid, so I am fine with merging this as it is.

@Ayush1325
Copy link
Contributor Author

Sorry, my bad. Using MaybeUninit::uninit() works. It's just that the NULL pointer does not work.

Of course, I am only testing on qemu with OVMF currently.

@dvdhrm
Copy link
Member

dvdhrm commented Jul 8, 2022

Sorry, my bad. Using MaybeUninit::uninit() works. It's just that the NULL pointer does not work.

No worries! I am just glad this is not a weird implementation quirk.

Of course, I am only testing on qemu with OVMF currently.

We all are :) But sooner or later people will report the weirdest behavior of their firmware...

I will merge this nonetheless, as I think it is quite useful if you want to avoid the MaybeUninit dance. Thanks, and sorry for the long discussion!

@dvdhrm dvdhrm merged commit d038260 into r-efi:main Jul 8, 2022
@Ayush1325 Ayush1325 deleted the time branch July 11, 2022 08:45
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.

None yet

2 participants