foucault / nvfancontrol

NVidia dynamic fan control for Linux and Windows
GNU General Public License v3.0
206 stars 20 forks source link

Replace mem::uninitialized with mem::MaybeUninit #37

Closed rHermes closed 2 years ago

rHermes commented 3 years ago

Hey!

Not really a rust developer, so I hope I did this right. Got some warnings when building the binary, so decided to try fixing them.

While developing I ran cargo fmt, which reformated a bunch of the code, but I've split it into two commits. If the changes are not wanted, I'll of course just revert that commit.

Thanks for creating this software, it's been a god send!

foucault commented 3 years ago

Hi and thanks for the contribution. I'm not a Rust developer either, so don't worry about it! I'm a bit reluctant to do automatic rust formatting at this point as it generates a big commit for no added benefit. The uninitialised patch looks good but there is an instance of mem::uninitialized in the Windows code as well (windows.rs), can you fix that too?

rHermes commented 3 years ago

Hey! I will replace the windows instance too, but I'm not sure how I'm going to do the replacement without getting the automatic formatting, my vim does it by default.

I removed the overall formatting to other files and changed the windows one too. I am not able to build the windows version, so I cannot test that it works or compiles.

Also, I want to point out that there was some comment about it always being wrong: https://doc.rust-lang.org/std/mem/fn.uninitialized.html or undefined behavior. I don't know enough about the setup to be able to see if there is a better way to do this, but maybe there is a way to remove the unsafe parts when switching to MaybeUninit?

kdave commented 2 years ago

FWIW below is minimal fix that works on recent rust (1.58+)

--- a/src/nvctrl/os/unix.rs
+++ b/src/nvctrl/os/unix.rs
@@ -239,7 +239,7 @@ impl NvidiaControl {

             for i in 0..gpu_count {
                 let mut len = -1 as i32;
-                let v: *mut c_uchar = unsafe { mem::uninitialized() };
+                let v: *mut c_uchar = unsafe { mem::MaybeUninit::uninit().assume_init() };

                 match unsafe {
                     XNVCTRLQueryTargetBinaryData(dpy, CTRL_TARGET::GPU, i, 0,
@@ -442,7 +442,7 @@ impl NvFanController for NvidiaControl {
             return Err("XNVCTRLIsNvScreen failed; no screens assigned to the NVidia driver".to_string());
         }

-        let v: *mut c_char = unsafe { mem::uninitialized() };
+        let v: *mut c_char = unsafe { mem::MaybeUninit::uninit().assume_init() };
         match unsafe {
             XNVCTRLQueryStringAttribute(self.dpy, nv_screen, 0, CTRL_ATTR::NVIDIA_DRIVER_VERSION, &v)
         } {
@@ -458,7 +458,7 @@ impl NvFanController for NvidiaControl {

         self.check_gpu_id(id)?;

-        let v: *mut c_char = unsafe { mem::uninitialized() };
+        let v: *mut c_char = unsafe { mem::MaybeUninit::uninit().assume_init() };
         match unsafe {
             XNVCTRLQueryTargetStringAttribute(self.dpy, CTRL_TARGET::GPU, id as i32,
                                               0, CTRL_ATTR::PRODUCT_NAME, &v)
@@ -475,7 +475,7 @@ impl NvFanController for NvidiaControl {

         self.check_gpu_id(id)?;

-        let v: *mut c_char = unsafe { mem::uninitialized() };
+        let v: *mut c_char = unsafe { mem::MaybeUninit::uninit().assume_init() };
         match unsafe {
             XNVCTRLQueryTargetStringAttribute(self.dpy, CTRL_TARGET::GPU, 0, 0,
                                               CTRL_ATTR::UTILIZATION, &v)
foucault commented 2 years ago

This has now been addressed.