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

Fix DevSetupAgent logging and some other fixes. #2566

Merged
merged 3 commits into from
Apr 9, 2024

Conversation

sshilov7
Copy link
Member

@sshilov7 sshilov7 commented Apr 9, 2024

Summary of the pull request

DevSetupAgent and DevSetupEngine were crashing on start after previous (#2474) change to logging setup.
This change

  • Fixes DevSetupAgent and DevSetupEngine logging to output loggs into
    %localappdata%\Temp\DevSetupEngine\Logs\HyperV directory (like C:\Users\LocalAdmin\AppData\Local\Temp\DevSetupEngine\Logs\HyperV for DevSetupgent)
  • Adds COM access permission to DevSetupAgent to allow calls from LocalSystem and Interactive Users only.
  • Move Hyper-V DevSetupAgent PS script into a separate file (DevSetupAgent.ps1)
  • Move HResultException.cs to HyperVExtension.HostGuestCommunication library to use on DevSetupAgent side
  • Small fixes to error message in Host-Guest communication

Validation steps performed

Tested DevSetupAgent setup, start and configuration workflow on a Hyper-V VM.

PR checklist

  • Closes #xxx
  • Tests added/passed
  • Documentation updated

@sshilov7 sshilov7 self-assigned this Apr 9, 2024
@krschau
Copy link
Collaborator

krschau commented Apr 9, 2024

I think we should keep all the Logs under the Logs folder, so instead of
%localappdata%\Temp\DevSetupEngine\Logs\HyperV
it would be
%localappdata%\TempState\Logs\DevSetupEngine\HyperV

From my understanding of how future enhancements to let FeedbackHub collect logs would work, using this new place would be a headache.

krschau
krschau previously requested changes Apr 9, 2024
Copy link
Collaborator

@krschau krschau left a comment

Choose a reason for hiding this comment

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

Log location comment. See

Environment.SetEnvironmentVariable("DEVHOME_LOGS_ROOT", Path.Join(Helpers.Logging.LogFolderRoot, "CoreWidgets"));
for suggestion on how to get the location.

@sshilov7
Copy link
Member Author

sshilov7 commented Apr 9, 2024

@krschau , I don't understand what you are asking for referring to devhome/extensions/CoreWidgetProvider/Program.cs. Please clarify.

As for the actual location: %localappdata%\TempState normally does not exist and is not documented. Currently, Dev Home logs go under %localappdata%\Packages<PackageFamilyName>\TempState\Logs. Code changed in this PR is not part of Dev Home packaged app and doesn't have package identity and cannot write logs under that location. Furthermore, DevSetupAgent runs under LocalSystem account and cannot write logs under some user's account where Dev Home may exist.


if (PInvoke.MakeAbsoluteSD(securityDescriptor, absolutSd, ref absoluteSdSize, null, ref daclSize, null, ref saclSize, ownerSid, ref ownerSize, groupSid, ref groupSize))
{
throw new HResultException(HRESULT.E_UNEXPECTED);
Copy link
Contributor

@huzaifa-d huzaifa-d Apr 9, 2024

Choose a reason for hiding this comment

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

Suggestion: Add a brief message describing the error and where it failed.

@krschau
Copy link
Collaborator

krschau commented Apr 9, 2024

@krschau , I don't understand what you are asking for referring to devhome/extensions/CoreWidgetProvider/Program.cs. Please clarify.

As for the actual location: %localappdata%\TempState normally does not exist and is not documented. Currently, Dev Home logs go under %localappdata%\Packages\TempState\Logs. Code changed in this PR is not part of Dev Home packaged app and doesn't have package identity and cannot write logs under that location. Furthermore, DevSetupAgent runs under LocalSystem account and cannot write logs under some user's account where Dev Home may exist.

Sorry, I assumed this was part of the extension and could write to the Dev Home logs.

@krschau krschau self-requested a review April 9, 2024 21:34
@krschau krschau dismissed their stale review April 9, 2024 21:34

Misunderstood logging

@sshilov7 sshilov7 merged commit 690127e into main Apr 9, 2024
4 checks passed
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.

4 participants