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

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

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

Conversation

kosmas12
Copy link
Contributor

This PR fixes issue #381. While I had previously opened a PR for this (#382), it was getting bloated, so I am opening this new one after I improved upon all the suggestions in the other PR.

@kosmas12 kosmas12 force-pushed the unmountsamples branch 3 times, most recently from 2337f43 to d8e7826 Compare February 7, 2021 12:34
Copy link
Member

@thrimbor thrimbor left a comment

Choose a reason for hiding this comment

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

Left a few more review comments. When you make changes to your PR, I recommend to also leave a comment or respond to the feedback comments, it makes it easier to see that you changed something and which feedback you addressed. When you just quietly push changes, they might go unnoticed.

samples/winapi_filefind/main.c Outdated Show resolved Hide resolved
samples/winapi_drivelist/main.c Show resolved Hide resolved
samples/winapi_drivelist/main.c Outdated Show resolved Hide resolved
samples/winapi_drivelist/main.c Outdated Show resolved Hide resolved
samples/winapi_filefind/main.c Outdated Show resolved Hide resolved
samples/winapi_filefind/main.c Outdated Show resolved Hide resolved
samples/winapi_filefind/main.c Outdated Show resolved Hide resolved
@kosmas12
Copy link
Contributor Author

kosmas12 commented Mar 4, 2021

Fixed the indentation and added gotos and labels

Copy link
Member

@thrimbor thrimbor left a comment

Choose a reason for hiding this comment

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

You seem to have had a bit of trouble understanding how exactly goto works or how my review comment was meant, so let me try to explain a bit further.

goto is like a jump instruction, whenever it executes, it simply jumps to the place where the label is located, it's not like function call, there is no returning back to where the goto is.

My idea of handling the errors is something like this (pseudo-code):

    if(mount C fails) {
        print error;
        goto done;
    }

    if(mount E fails) {
        print error;
        goto unmount_c;
    }

    if(something else fails) {
        print error;
        goto unmount_e;
    }

    do whatever the sample is intended to do

unmount_e:
    unmount E here
unmount_c:
    unmount C here

    while(1) {
        Sleep(2000);
    }

   return 0;

Notice how C is mounted first, but unmounted last. This is important because a goto unmount_e; will first unmount E, and execution will then "fall through" to unmount_c, so both drives will get unmounted, but a goto unmount_c will only unmount C, not E.
When no error happens, execution will simply continue normally, never executing a goto, but still running the code after the labels, so the drives will get unmounted, too.

error = GetLastError();
debugPrint("\nCouldn't unmount E: drive! Error code: %x\n", error);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

You seem to have accidentally removed a newline at the end here.

Copy link
Member

@thrimbor thrimbor Feb 4, 2023

Choose a reason for hiding this comment

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

I'm almost certain this was fixed before, but now the newline is removed again.

samples/winapi_drivelist/main.c Outdated Show resolved Hide resolved
samples/winapi_drivelist/main.c Outdated Show resolved Hide resolved
samples/winapi_drivelist/main.c Outdated Show resolved Hide resolved
samples/winapi_drivelist/main.c Outdated Show resolved Hide resolved
samples/winapi_drivelist/main.c Outdated Show resolved Hide resolved
samples/winapi_drivelist/main.c Outdated Show resolved Hide resolved
samples/winapi_drivelist/main.c Outdated Show resolved Hide resolved
samples/winapi_filefind/main.c Outdated Show resolved Hide resolved
samples/winapi_filefind/main.c Outdated Show resolved Hide resolved
@kosmas12
Copy link
Contributor Author

Changes made, hope it's all good now

samples/winapi_drivelist/main.c Outdated Show resolved Hide resolved
samples/winapi_filefind/main.c Outdated Show resolved Hide resolved
samples/winapi_filefind/main.c Outdated Show resolved Hide resolved
samples/winapi_drivelist/main.c Outdated Show resolved Hide resolved
samples/winapi_drivelist/main.c Outdated Show resolved Hide resolved
samples/winapi_filefind/main.c Outdated Show resolved Hide resolved

unmount_e:
ret = nxUnmountDrive('E');
// If there was an error while unmounting
Copy link
Member

Choose a reason for hiding this comment

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

This comment adds nothing in my opinion.
The "GetLastError" in combination with !ret strongly implies that this is error-checking.

@@ -10,10 +10,15 @@ int main(void)
{
XVideoSetMode(640, 480, 32, REFRESH_DEFAULT);

// Create a variable for WinAPI error checking
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

@kosmas12
Copy link
Contributor Author

I just applied all the suggested changes, ready to make more if needed

@kosmas12
Copy link
Contributor Author

Building failed previously, tired me forgot to test before pushing haha.

Errors fixed and building of both samples tested to work fine.

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

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

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.

@JayFoxRox
Copy link
Member

JayFoxRox commented Apr 10, 2021

I just applied all the suggested changes, ready to make more if needed

No, you didn't. And it's very hard to keep track of what you did or how you resolved things.

I suggest to leave a comment on each review comment, telling the reviewer what you did to resolve it.
That way it's much easier to confirm if you understood the feedback + to spot potential logic errors in the new solution.
This is especially important for beginners like you, but should also be expected of experienced coders.


This is also taking up a lot of reviewer time and too many iterations for such a small change.

Building failed previously, tired me forgot to test before pushing haha.

This also confirms why it's taking so much time: to little testing locally, and too little reflection / review on your own work.
You should review your own changes to make sure you only send a finalized version to reviewers, otherwise you are wasting everyones time (yours included).

Because this PR discussion is getting hard to follow (too much review feedback), I'd even consider starting a new PR if this doesn't reach an acceptable state in the next iteration.

@kosmas12
Copy link
Contributor Author

Changed what I could and tested. If anything more is left I'd also gladly move it to a new PR (even though I know that I should have got it right a looooong time ago and that opening even this 2nd PR is a bit absurd)

@kosmas12
Copy link
Contributor Author

I believe this is ready for a review and hopefully it's (at least one of the) final iteration(s).

samples/winapi_drivelist/main.c Outdated Show resolved Hide resolved
samples/winapi_filefind/main.c Outdated Show resolved Hide resolved
@kosmas12
Copy link
Contributor Author

Removal of return was indeed stupid of me, added it back now and ready for a new round of reviews if needed

@kosmas12
Copy link
Contributor Author

I believe I have solved the merge conflicts, so this might be in the clear.

@kosmas12 kosmas12 force-pushed the unmountsamples branch 8 times, most recently from 64d0dec to 4db54c3 Compare September 26, 2021 15:42
@kosmas12 kosmas12 force-pushed the unmountsamples branch 3 times, most recently from 1ccc6cb to 6d7deb7 Compare February 3, 2023 13:23
Copy link
Member

@thrimbor thrimbor left a comment

Choose a reason for hiding this comment

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

There are some review comments still unaddressed, and a couple of issues that seem to be new - newlines are getting removed again, the commit author is incorrect, and somehow f2c634f got mixed in.

This is also branching off a rather old version of nxdk now and imho should be rebased onto current master to make sure there aren't any conflicts.

Comment on lines 65 to 76
static void free_xid_device(xid_dev_t *xid_dev) {
//Find the device head in the linked list
xid_dev_t *head = pxid_list;

//If the xid is at the head of the list, remove now.
if (xid_dev == head)
{
pxid_list = head->next;
head = NULL;
}

//Find the device head in the linked list
while (head != NULL && head->next != xid_dev)
Copy link
Member

Choose a reason for hiding this comment

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

What is this change doing here? (f2c634f)

error = GetLastError();
debugPrint("\nCouldn't unmount E: drive! Error code: %x\n", error);
}
}
Copy link
Member

@thrimbor thrimbor Feb 4, 2023

Choose a reason for hiding this comment

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

I'm almost certain this was fixed before, but now the newline is removed again.

Comment on lines 61 to 72
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This newline should not get removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants