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

Improve HaxmLoader. #217

Merged
merged 1 commit into from Nov 22, 2019

Conversation

@coxuintel
Copy link
Contributor

coxuintel commented Jun 24, 2019

  • Stop release service "IntelHaxm" before loading the test service.
    User doesn't have to stop "IntelHaxm" manually before use HaxmLoader.
  • Prompt required administrative privileges if not run as administrators.
    If without administrative privileges, user only see access denied but
    isn't clear what should do.

Signed-off-by: Colin Xu colin.xu@intel.com

Copy link
Contributor

wcwang left a comment

This change is quite practical for debugging. Previously the developer has to run 'sc stop IntelHaxm' before loading the driver.

BOOL bElevated = FALSE;
HANDLE hToken = NULL;

if (OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &hToken)) {

This comment has been minimized.

Copy link
@wcwang

wcwang Jul 4, 2019

Contributor
if (!OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &hToken) ||
    (hToken == NULL)) {
    return FALSE;
}

TOKEN_ELEVATION procTokenElevation;
DWORD cbSize = sizeof(TOKEN_ELEVATION);

if (GetTokenInformation(hToken, TokenElevation, &procTokenElevation,
                        sizeof(procTokenElevation), &cbSize)) {
    bElevated = procTokenElevation.TokenIsElevated;
}

CloseHandle(hToken);

This comment has been minimized.

Copy link
@coxuintel

coxuintel Jul 5, 2019

Author Contributor

Personally I don't prefer early returns a lot. But I'll follow the style around.

This comment has been minimized.

Copy link
@coxuintel

coxuintel Jul 5, 2019

Author Contributor

Checking hToken after successful OpenProcessToken seems redundant, A process HANDLE can never be NULL if it's obtained successfully.

@@ -66,6 +90,28 @@ static int DriverUninstall(void)
printf("%s(): OpenSCManager failed.\r\n", __FUNCTION__);
goto cleanup;
}
// Stop installed service
hService = OpenServiceW(hSCManager, SERVICE_NAME_RELEASE, SERVICE_ALL_ACCESS);

This comment has been minimized.

Copy link
@wcwang

wcwang Jul 4, 2019

Contributor

Line break before SERVICE_ALL_ACCESS as this line exceeds column 80.

SERVICE_STATUS_PROCESS ssp = { 0 };
DWORD ssp_size = 0;

if (QueryServiceStatusEx(hService, SC_STATUS_PROCESS_INFO, (LPBYTE)(&ssp), sizeof(ssp), &ssp_size)) {

This comment has been minimized.

Copy link
@wcwang

wcwang Jul 4, 2019

Contributor

Line break before LPBYTE(&ssp). According to the guide https://google.github.io/styleguide/cppguide.html, should it remove the parentheses of LPBYTE?

This comment has been minimized.

Copy link
@coxuintel

coxuintel Jul 5, 2019

Author Contributor

The parentheses around LPBYTE is necessary, otherwise LPBYTE(&ssp) is treated as a function and cause C2063 compiling error.

if (ControlService(hService, SERVICE_CONTROL_STOP, &ss)) {
wprintf(L"%s(): %s is stopped.\r\n",
WFUNCTION, SERVICE_NAME_RELEASE);
}

This comment has been minimized.

Copy link
@wcwang

wcwang Jul 4, 2019

Contributor

} else {

printf("%s(): Failed to get absolute path:\r\n",
__FUNCTION__);
wprintf(L" sysFilePath='%s'\r\n", sysFilePath);
wprintf(L"%s(): Failed to get absolute path:\r\n sysFilePath=\"%s\"\r\n",

This comment has been minimized.

Copy link
@wcwang

wcwang Jul 4, 2019

Contributor

Can wchar_t string be split into multiple lines as this line exceeds 80 columns?

This comment has been minimized.

Copy link
@coxuintel

coxuintel Jul 5, 2019

Author Contributor

Sure, will do that.

#define WIDE1(x) WIDE2(x)
#define WFUNCTION WIDE1(__FUNCTION__)

#define SERVICE_NAME_RELEASE L"IntelHaxm"
#define SERVICE_NAME L"haxm service for test"

This comment has been minimized.

Copy link
@wcwang

wcwang Jul 4, 2019

Contributor

Do you think it is appropriate to rename the SERVICE_NAME to "IntelHaxm_t" or "HaxmTest" as it is inconvenient for command line parameter using the long string with spaces?

This comment has been minimized.

Copy link
@coxuintel

coxuintel Jul 5, 2019

Author Contributor

emm, good point. I'll change that too.

@coxuintel coxuintel force-pushed the coxuintel:haxmloader branch from 3f84f11 to d6d61d6 Jul 5, 2019
- Naming test service display name as "Intel HAXM Test Service" while
  installed release service is "Intel HAXM Service".
- Stop release service "IntelHaxm" before loading the test service.
  User doesn't have to stop "IntelHaxm" manually before using HaxmLoader.
  Before the change, HaxmLoader can't load the test service.
- Allow test service overwriting.
  Before the change user must -u before -i the driver from different location.
- Prompt required administrative privileges if not run as administrators.
  If no administrative privileges, user only see "access denied" error but
  isn't clear what should do.
- Update version to 1.1.0.

Signed-off-by: Colin Xu <colin.xu@intel.com>
@coxuintel coxuintel force-pushed the coxuintel:haxmloader branch from d6d61d6 to 6656a1c Nov 21, 2019
@wcwang
wcwang approved these changes Nov 22, 2019
@wcwang wcwang merged commit 42c26eb into intel:master Nov 22, 2019
2 checks passed
2 checks passed
Jenkins Build Result pass
Details
Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.