Skip to content

Add timeout to wdi-simple installers#140

Closed
karelbilek wants to merge 1 commit intopbatard:masterfrom
karelbilek:master
Closed

Add timeout to wdi-simple installers#140
karelbilek wants to merge 1 commit intopbatard:masterfrom
karelbilek:master

Conversation

@karelbilek
Copy link
Contributor

@karelbilek karelbilek commented Sep 13, 2018

We solved the issue #131 with our user

It actually had nothing (much) to do with HP Driver. However, the user had somehow misconfigured PC that kept re-installing some (different!) faulty device ad-infinitum in a loop.

Which caused the wcid-simple inside the NSIS installer to always fail. Fixing it as in the PR fixed the issue. (The HP driver does not actually override the libwdi driver, because the libwdi driver was not installed in the first place.)

I have added the timeouts to the example NSIS and ISS scripts, maybe it will help someone. I know it is an extreme edge-case but it actually helped at least 2 users.

(I have no idea what timeout zadig uses, as I cannot find pending_install_timeout in the zadig code. Perhaps it should be added there too somewhere to fix similar issue.)

@karelbilek
Copy link
Contributor Author

Hm, now we had a very similar issue, which was solved by further increasing the timeout, from 2 minutes to 10 minutes. :) since the faulty device that kept getting re-installed took 5 minutes to fail.

I can increase it here too.

@karelbilek
Copy link
Contributor Author

karelbilek commented Sep 17, 2018

By the way, is the check about another pending install necessary? What exactly does it prevent; why cannot Windows install two drivers at the same time?

I am under the impression the CMP_WaitNoPendingInstallEvents is not needed.

Would you be OK if I made a PR that disables this check altogether with an option?

libwdi/libwdi/libwdi.c

Lines 1488 to 1496 in 38f4820

if ((params->options != NULL) && (pfCMP_WaitNoPendingInstallEvents != NULL)) {
if (pfCMP_WaitNoPendingInstallEvents(params->options->pending_install_timeout) == WAIT_TIMEOUT) {
wdi_warn("timeout expired while waiting for another pending installation - aborting");
r = WDI_ERROR_PENDING_INSTALLATION;
goto out;
}
} else {
wdi_dbg("CMP_WaitNoPendingInstallEvents not available");
}

@pbatard
Copy link
Owner

pbatard commented Sep 17, 2018

By the way, is the check about another pending install necessary?

Windows does not like installing two drivers at the same time. The OS usually prevents that.

Besides I hope you can also appreciate why removing the check and ensuring that people can install 2 drivers simultaneously would be HELL to test. Even if Windows was fine with it, the cost of validating that the feature of allowing 2 simultaneous driver installation is way too high. So we are much better off not allowing something that is hard to validate, to avoid issues like "I tried to install 2 drivers at once but I encountered problem X. Now, since your software allows this, please invest 1 week of your time to troubleshoot my specific issue!" → No Thanks!

@karelbilek
Copy link
Contributor Author

karelbilek commented Sep 17, 2018

I cannot find any windows documentation about two drivers being installed at the same time causing problems. Microsoft does not use this function anywhere in its driver examples, just here (this function calls CMP_WaitNoPendingInstallEvents) , and only for UX reasons, not system reasons.

What I want to do now is not two libwdi drivers being installed at the same time, but one libwdi installer and some other stray installer (in this case, some weird bluetooth headphone thing, but that doesn't matter) being installed.

I understand your point of view though! I know now that driver issues are hell to debug :D

@pbatard
Copy link
Owner

pbatard commented Sep 17, 2018

I cannot find any windows documentation about two drivers being installed at the same time causing problems.

Feel free to test it then.

What I want to do now is not two libwdi drivers being installed at the same time,

Yes, I understood that.

but one libwdi installer and some other stray

And my previous answer therefore applies.

If you want to invest a lot of time validating a scenario that's not going to benefit anyone, be my guest. But please don't count on me to follow you there.

@karelbilek
Copy link
Contributor Author

All right.

This PR though should be fine, no? (Maybe with even increased timeouts)

@pbatard
Copy link
Owner

pbatard commented Sep 17, 2018

Yes, the PR looks okay, especially as I don't care that much about what are essentially sample scripts. You're supposed to use them as a base for your installation, and modify as you see fit. But I agree that demonstrating the use of the timeout option should help people who ran into the same issue you did.

I will integrate your PR into libwdi. I just don't know when.

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.

2 participants