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.

Sign up for GitHub

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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 37 additions & 20 deletions samples/winapi_drivelist/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,45 +9,46 @@ int main(void)

// Mount some drives for demonstration purposes
BOOL ret;
DWORD error;
ret = nxMountDrive('C', "\\Device\\Harddisk0\\Partition2\\");
if (!ret) {
debugPrint("Failed to mount C: drive!\n");
Sleep(5000);
return 1;
// Additional error info can be retrieved with GetLastError()
error = GetLastError();
debugPrint("Failed to mount C: drive! Error code: %x\n", error);
thrimbor marked this conversation as resolved.
Show resolved Hide resolved
thrimbor marked this conversation as resolved.
Show resolved Hide resolved
goto sleepForever;
}

ret = nxMountDrive('E', "\\Device\\Harddisk0\\Partition1\\");
if (!ret) {
debugPrint("Failed to mount E: drive!\n");
Sleep(5000);
return 1;
error = GetLastError();
debugPrint("Failed to mount E: drive! Error code: %x\n", error);
goto unmount_c;
Copy link
Member

@JayFoxRox JayFoxRox Apr 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This code makes little sense to me; mounting E failed, so it jumps to unmounting C?
The labels should be named more appropriately.

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 believe that this specific label should stay as unmount_c, it's a good enough name IMO.

Copy link
Member

Choose a reason for hiding this comment

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

@JayFoxRox What would you suggest? Personally, it doesn't bother me, imho it looks just like when a function is freeing a previous allocation before returning due to an error.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure; I'd probably cause a fatal error if anything fails to init (to avoid having to do conditional cleanup).

If conditional cleanup is mandatory I'd probably have a boolean which stores which drives were mounted correctly (mountedC / mountedE), I'd then have a generic cleanup function at the end of the scope which unmounts it (a single label called cleanup, which would conditionally deal with cleanup).

In a real application I'd probably have something like atexit / destructors which deal with this (= no labels); also ensures proper order of destruction without having to think about it.
Probably not practical for a sample because it adds too much complexity.

Copy link
Member

@JayFoxRox JayFoxRox Feb 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Especially with a generic label such as cleanup now, I think this should use booleans / keep track of it and handle everything in cleanup.

}

// Retrieve drive bitmaks. Every bit represents one drive letter
// Retrieve drive bitmasks. Every bit represents one drive letter
DWORD driveBits = GetLogicalDrives();
if (driveBits == 0 && GetLastError() != ERROR_SUCCESS) {
debugPrint("Failed to retrieve drive bitmask!\n");
Sleep(5000);
return 1;
error = GetLastError();
if (driveBits == 0 && error != ERROR_SUCCESS) {
debugPrint("Failed to retrieve drive bitmask! Error code: %x\n", error);
goto cleanup;
}
debugPrint("Drive bitmask: 0x%lx\n\n", driveBits);

debugPrint("Drive bitmask: 0xl%x\n\n", driveBits);

// Reserve buffer long enough for all possible drive strings plus null-terminator
char buffer[26 * 4 + 1];
// IMPORTANT: The size passed to GetLogicalDriveStringsA is WITHOUT the null-terminator, even though it gets written
DWORD charsWritten = GetLogicalDriveStringsA(sizeof(buffer)-1, buffer);
if (charsWritten == 0) {
// Additional error info can be retrieved with GetLastError()
debugPrint("Failed to retrieve drive strings!\n");
Sleep(5000);
return 1;
error = GetLastError();
debugPrint("Failed to retrieve drive strings! Error code: %x\n", error);
goto cleanup;
}

if (charsWritten > sizeof(buffer) - 1) {
// Can't happen here as our buffer is large enough to cover all possibilities
debugPrint("Buffer for GetLogicalDriveStringsA too small!\n");
Sleep(5000);
return 1;
goto cleanup;
}

debugPrint("Drives found:\n");
Expand All @@ -56,9 +57,25 @@ int main(void)
debugPrint("%s\n", drive);
while(*drive++);
}
debugPrint("\ndone");

while(1) {
debugPrint("\nDone!");

cleanup:
ret = nxUnmountDrive('E');
if (!ret) {
error = GetLastError();
debugPrint("\nFailed to unmount E: drive! Error code: %x\n", error);
}

unmount_c:
ret = nxUnmountDrive('C');
if (!ret) {
error = GetLastError();
debugPrint("\nFailed to unmount C: drive! Error code: %x\n", error);
}

sleepForever:
while (1) {
Sleep(2000);
}

Expand Down
29 changes: 20 additions & 9 deletions samples/winapi_filefind/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,16 @@ int main(void)
{
XVideoSetMode(640, 480, 32, REFRESH_DEFAULT);

DWORD error;
Copy link
Member

Choose a reason for hiding this comment

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

This can be a highly local variable, I don't see a benefit for putting it at function scope?
If you declare it where it's used it also won't require any additional comment.

If the variable is redeclared, you should use different names to differentiate them; in this particular sample this only seems to happen after FindNextFile, so you could probably do DWORD findFileError = GetLastError() in that instance.

Copy link
Member

Choose a reason for hiding this comment

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

This wasn't done everywhere


// Mount C:
BOOL ret = nxMountDrive('C', "\\Device\\Harddisk0\\Partition2\\");

if (!ret) {
debugPrint("Failed to mount C: drive!\n");
Sleep(5000);
return 1;
// There was an error. We can get more information about an error from WinAPI code using GetLastError()
error = GetLastError();
debugPrint("Failed to mount C: drive! Error code: %x\n", error);
goto sleepForever;
}

debugPrint("Content of C:\\\n");
Expand All @@ -27,9 +31,9 @@ int main(void)
// no matter whether they contain a dot or not
hFind = FindFirstFile("C:\\*.*", &findFileData);
if (hFind == INVALID_HANDLE_VALUE) {
debugPrint("FindFirstHandle() failed!\n");
Sleep(5000);
return 1;
error = GetLastError();
debugPrint("FindFirstHandle() failed! Error code: %x\n", error);
goto cleanup;
}

do {
Expand All @@ -38,21 +42,28 @@ int main(void)
} else {
debugPrint("File : ");
}

debugPrint("%s\n", findFileData.cFileName);
} while (FindNextFile(hFind, &findFileData) != 0);

debugPrint("\n");

DWORD error = GetLastError();
error = GetLastError();
if (error == ERROR_NO_MORE_FILES) {
debugPrint("Done!\n");
} else {
debugPrint("error: %lx\n", error);
debugPrint("Error: %lx\n", error);
}

FindClose(hFind);

cleanup:
ret = nxUnmountDrive('C');
// If there was an error while unmounting
if (!ret) {
error = GetLastError();
debugPrint("Failed to unmount C: drive! Error code: %x", error);
}
sleepForever:
while (1) {
Sleep(2000);
}
Expand Down