samples: Make winapi samples unmount drives when exiting #382

kosmas12 · 2020-07-16T18:03:12Z

Fixes #381

JayFoxRox · 2020-07-16T18:04:21Z

samples/winapi_drivelist/main.c

+    Sleep(20000);
+
+    ret = nxUnmountDrive('C');
+    if (!ret) { // if there was an error while unmounting (nxUnmountDrive() returns false)


We don't do these sort of inline-comments. Check surrounding code for how it should be done.

(Other instances, too)

JayFoxRox · 2020-07-16T18:05:05Z

samples/winapi_drivelist/main.c

+    if (!ret) { // if there was an error while unmounting (nxUnmountDrive() returns false)
+      debugPrint("Couldn't unmount C: drive. Exiting in 5 seconds...");
+      Sleep (5000);
+      return 1;


nit: I'd consider this bad practice, because we are already trying to exit, and this just skips unmounting E drive, too.
Instead, set error flag instead which can be returned from main?

(Other instances, too)

JayFoxRox · 2020-07-16T20:55:20Z

samples/winapi_drivelist/main.c

-        debugPrint("Failed to retrieve drive strings!\n");
+        errorCount++;
+        error = GetLastError();
+        debugPrint("Failed to retrieve drive strings! Reason: %s\n", error);


error is not a char*; so %s is wrong.

JayFoxRox · 2020-07-16T20:55:37Z

samples/winapi_drivelist/main.c

@@ -7,17 +7,27 @@ int main(void)
 {
    XVideoSetMode(640, 480, 32, REFRESH_DEFAULT);

+    // Create a variable for WinAPI error checking
+    DWORD error;


nit: This doesn't have to be at this scope.

JayFoxRox · 2020-07-16T20:56:08Z

samples/winapi_drivelist/main.c

+    // Create a variable for WinAPI error checking
+    DWORD error;
+    // Make a variable to keep track of how many errors we have
+    int errorCount = 0;


nit: This can be unsigned int.

JayFoxRox · 2020-07-16T20:56:58Z

samples/winapi_drivelist/main.c

+        // Additional error info can be retrieved with GetLastError()
+        error = GetLastError();
+        debugPrint("Failed to mount C: drive! Reason: %s\n", error);
+        // Give the user some time to read the message
        Sleep(5000);
        return 1;


This could be considered optional now.

JayFoxRox · 2020-07-16T20:58:43Z

samples/winapi_drivelist/main.c

+      errorCount++;
+      debugPrint("Couldn't unmount C: drive Reason: %s\n. Trying to unmount E: drive...\n", error);
+      // Leave some time for the user to read the error
+      Sleep(5000);


You already wait 10 seconds if any error happened. Waiting an additional 5 seconds per error is not necessary in my opinion.

(Other instances, too)

JayFoxRox · 2020-07-16T20:58:58Z

samples/winapi_drivelist/main.c

@@ -56,10 +67,32 @@ int main(void)
        debugPrint("%s\n", drive);
        while(*drive++);
    }
-    debugPrint("\ndone");
+    debugPrint("\nDone! Exiting in 10 seconds...");


This is not what's happening.

JayFoxRox · 2020-07-16T20:59:27Z

samples/winapi_drivelist/main.c

-        debugPrint("Failed to mount E: drive!\n");
+        errorCount++;
+        error = GetLastError();
+        debugPrint("Failed to mount E: drive! Reason: %s\n", error);
        Sleep(5000);
        return 1;


This is actually worse now, because we don't unmount in this case.

JayFoxRox · 2020-07-16T21:00:14Z

samples/winapi_drivelist/main.c

+    ret = nxUnmountDrive('E');
+    if (!ret) {
+      error = GetLastError();
+      errorCount++;


I feel like you should do this before or after printing the message, but not inbetween getting the message and printing it.

JayFoxRox · 2020-07-16T21:01:35Z

samples/winapi_filefind/main.c

    if (error == ERROR_NO_MORE_FILES) {
-        debugPrint("Done!\n");
+        debugPrint("Done! Exiting in 10 seconds...\n");


This is not what's happening.

JayFoxRox · 2020-07-16T21:01:57Z

samples/winapi_filefind/main.c

    } else {
        debugPrint("error: %x\n", error);
    }

    FindClose(hFind);

-    while (1) {
-        Sleep(2000);
+    Sleep(10000);


This wait doesn't make any sense to me.

JayFoxRox · 2020-08-08T15:57:59Z

samples/winapi_drivelist/main.c

-    debugPrint("\ndone");
+    debugPrint("\nDone! Re-running in 10 seconds...");
+
+    // Give some time to the user to read the drives


The user would be reading the output, but not the "drives"

JayFoxRox · 2020-08-08T15:58:20Z

samples/winapi_drivelist/main.c

@@ -7,27 +7,30 @@ int main(void)
 {
    XVideoSetMode(640, 480, 32, REFRESH_DEFAULT);

+    // Create a variable to check if there was any error in the program
+    BOOL errored = false;


nit: This is a poor variable name in my opinion.

JayFoxRox · 2020-08-08T15:58:39Z

samples/winapi_drivelist/main.c

-        Sleep(5000);
-        return 1;
+        errored = true;
+        // Additional error info can be retrieved with GetLastError()


This comment doesn't add anything

JayFoxRox · 2020-08-08T15:59:58Z

samples/winapi_drivelist/main.c

-        debugPrint("Failed to mount C: drive!\n");
-        Sleep(5000);
-        return 1;
+        errored = true;


This should probably be done after handling the error, to not disturb the readability of the important bits.

Also if this is later turned into a mark_error(); (for whatever reason) then that function might actually change the GetLastError() value.

JayFoxRox · 2020-08-08T16:00:51Z

samples/winapi_drivelist/main.c

@@ -56,10 +58,30 @@ int main(void)
        debugPrint("%s\n", drive);
        while(*drive++);
    }
-    debugPrint("\ndone");
+    debugPrint("\nDone! Re-running in 10 seconds...");


Where does this "re-running" happen?

JayFoxRox · 2020-08-08T16:01:43Z

samples/winapi_filefind/main.c

+    Sleep(10000);
+
+    ret = nxUnmountDrive('C');
+    // If there was an error while unmounting (nxUnmountDrive() returns false)


This claims that it will ~"return false". This isn't actually happening anywhere.

JayFoxRox · 2020-08-08T16:03:21Z

Due to the large amount of issues and backlog, I'm leaning towards closing this PR.
I feel that some changes during review actually made some of the code worse than the initial proposal.

kosmas12 · 2020-08-08T16:09:08Z

I understand that there are certain issues in the code, and I will prioritize fixing them. I will do some changes here soon, but first I want to go through them in detail myself to find what might be bad and what might be better to change. I will ping when I believe this will be ready for further review, on a different PR if necessary, since, as stated, this one is starting to become very inefficient.

mborgerson · 2020-10-12T22:23:52Z

@kosmas12 Do you still intend to fix this PR per feedback? If not, I will close this PR within the next couple of days.

mborgerson · 2020-10-14T22:07:44Z

Closed due to lack of activity. If you wish to resume this work, we can re-open.

JayFoxRox reviewed Jul 16, 2020

View reviewed changes

kosmas12 force-pushed the unmountsamples branch 3 times, most recently from b66d362 to bc0186c Compare July 16, 2020 20:48

JayFoxRox reviewed Jul 16, 2020

View reviewed changes

kosmas12 force-pushed the unmountsamples branch from bc0186c to 24fc42b Compare July 16, 2020 20:57

JayFoxRox reviewed Jul 16, 2020

View reviewed changes

samples: Make winapi samples unmount drives when exiting

348de3f

kosmas12 force-pushed the unmountsamples branch from 24fc42b to 348de3f Compare July 16, 2020 21:56

JayFoxRox reviewed Aug 8, 2020

View reviewed changes

mborgerson closed this Oct 14, 2020

kosmas12 mentioned this pull request Jan 14, 2021

samples: Make winapi samples unmount drives before finishing execution #421

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

samples: Make winapi samples unmount drives when exiting #382

samples: Make winapi samples unmount drives when exiting #382

kosmas12 commented Jul 16, 2020

JayFoxRox Jul 16, 2020 •

edited

Loading

JayFoxRox Jul 16, 2020 •

edited

Loading

JayFoxRox Jul 16, 2020

JayFoxRox Jul 16, 2020

JayFoxRox Jul 16, 2020

JayFoxRox Jul 16, 2020

JayFoxRox Jul 16, 2020

JayFoxRox Jul 16, 2020

JayFoxRox Jul 16, 2020

JayFoxRox Jul 16, 2020

JayFoxRox Jul 16, 2020

JayFoxRox Jul 16, 2020

JayFoxRox Aug 8, 2020

JayFoxRox Aug 8, 2020

JayFoxRox Aug 8, 2020

JayFoxRox Aug 8, 2020

JayFoxRox Aug 8, 2020

JayFoxRox Aug 8, 2020

JayFoxRox commented Aug 8, 2020

kosmas12 commented Aug 8, 2020 •

edited

Loading

mborgerson commented Oct 12, 2020

mborgerson commented Oct 14, 2020

samples: Make winapi samples unmount drives when exiting #382

samples: Make winapi samples unmount drives when exiting #382

Conversation

kosmas12 commented Jul 16, 2020

JayFoxRox Jul 16, 2020 • edited Loading

Choose a reason for hiding this comment

JayFoxRox Jul 16, 2020 • edited Loading

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

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

JayFoxRox commented Aug 8, 2020

kosmas12 commented Aug 8, 2020 • edited Loading

mborgerson commented Oct 12, 2020

mborgerson commented Oct 14, 2020

JayFoxRox Jul 16, 2020 •

edited

Loading

JayFoxRox Jul 16, 2020 •

edited

Loading

kosmas12 commented Aug 8, 2020 •

edited

Loading