Skip to content

Removed RegDeleteTreeA usage in storage_winuser#5182

Merged
gaurav2699 merged 8 commits into
mainfrom
gamecore_error
Jul 1, 2025
Merged

Removed RegDeleteTreeA usage in storage_winuser#5182
gaurav2699 merged 8 commits into
mainfrom
gamecore_error

Conversation

@gaurav2699

@gaurav2699 gaurav2699 commented Jun 24, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Gamecore builds are failing because it is not able to find the definition of RegDeleteTreeA in the undock pipelines.

Testing

Do any existing tests cover this change? Are new tests needed?

Documentation

Is there any documentation impact for this change?

@gaurav2699
gaurav2699 requested a review from a team as a code owner June 24, 2025 12:03
@gaurav2699
gaurav2699 requested a review from anrossi June 24, 2025 12:03
@codecov

codecov Bot commented Jun 24, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.25%. Comparing base (fbcbaa2) to head (80e66b7).
Report is 7 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #5182      +/-   ##
==========================================
- Coverage   86.68%   86.25%   -0.44%     
==========================================
  Files          59       59              
  Lines       18233    18330      +97     
==========================================
+ Hits        15806    15811       +5     
- Misses       2427     2519      +92

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nibanks

nibanks commented Jun 24, 2025

Copy link
Copy Markdown
Collaborator

It seems like there is a missing include instead.

mtfriesen
mtfriesen previously approved these changes Jun 24, 2025
View reviewed changes
@mtfriesen

mtfriesen commented Jun 24, 2025

Copy link
Copy Markdown
Contributor

It seems like there is a missing include instead.

I was wondering how this could be broken, but the documentation for RegDeleteTreeA says to include windows.h which we surely haven't missed?

@anrossi

anrossi commented Jun 24, 2025

Copy link
Copy Markdown
Collaborator

@nibanks I think gamecore specifically doesn't define the registry API, so we must define it here ourselves to build.

guhetier
guhetier reviewed Jun 24, 2025
View reviewed changes
Comment thread src/platform/storage_winuser.c Outdated
@nibanks

nibanks commented Jun 24, 2025

Copy link
Copy Markdown
Collaborator

I missed that this was gamecore specific. Generally, we just have to remove usage for things not on gamecore.

@mtfriesen
mtfriesen dismissed their stale review June 24, 2025 17:52

API is actually not in Gamecore

@mtfriesen

mtfriesen commented Jun 24, 2025

Copy link
Copy Markdown
Contributor

I missed that this was gamecore specific. Generally, we just have to remove usage for things not on gamecore.

Yeah, now that I read the documentation for these partitions more carefully, if we declare functions ourselves, it's possible for Gamecore to successfully compile and maybe even link to APIs that will not be present on every Gamecore system, leading to runtime failures down the line.

@guhetier

guhetier commented Jun 24, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Oh, that would be bad and annoying.

Also, I just remembered that #5112 actually remove the use of RegDeleteTreeA and replace it with an iteration over the relevant keys. This PR might not be needed if we merge #5112.

@gaurav2699 gaurav2699 changed the title added RegDeleteTreeA definition in restricted build Removed RegDeleteTreeA usage in storage_winuser Jun 26, 2025
@gaurav2699

gaurav2699 commented Jun 26, 2025

Copy link
Copy Markdown
Contributor Author

Added the storage_winuser part from #5112 as discussed with Anthony to unblock gamecore builds as the PR still requires changes before it gets merged.

@guhetier

guhetier commented Jun 26, 2025

Copy link
Copy Markdown
Collaborator

You need to update the CLog manifest (scripts/update-sidecar.ps1)

@guhetier guhetier mentioned this pull request Jun 26, 2025
Merged
mtfriesen
mtfriesen reviewed Jun 26, 2025
View reviewed changes
Comment thread src/platform/storage_winuser.c Outdated
mtfriesen
mtfriesen reviewed Jun 26, 2025
View reviewed changes
Comment thread src/platform/storage_winuser.c
mtfriesen
mtfriesen reviewed Jun 26, 2025
View reviewed changes
Comment thread src/platform/storage_winuser.c Outdated

CXPLAT_FREE(ValueName, QUIC_POOL_PLATFORM_TMP_ALLOC);

AllocatedLength *= 2; // The API doesn't tell us the needed length; just double it.

@mtfriesen mtfriesen Jun 26, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Apparently the RegQueryInfoKeyA function returns lpcbMaxValueNameLen which is the answer.

@anrossi anrossi Jun 27, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah, so at the top call RegQueryInfoKeyA to get the initial value to use for AllocatedLength?

@mtfriesen mtfriesen Jun 27, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yup, that's the theory.

@guhetier guhetier Jun 27, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

+1 on using it then, it seems to me like a more natural pattern!

@gaurav2699 gaurav2699 Jun 30, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have modified it according to the suggested method, please see if it looks fine. Thanks.

mtfriesen
mtfriesen previously approved these changes Jun 27, 2025
View reviewed changes

@mtfriesen mtfriesen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM as-is, though the RegQueryInfoKeyA approach may be tidier.

anrossi
anrossi reviewed Jun 27, 2025
View reviewed changes
Comment thread src/platform/storage_winuser.c Outdated
@anrossi

anrossi commented Jun 27, 2025

Copy link
Copy Markdown
Collaborator

Agree with Michael's comment, but this looks good too.

anrossi
anrossi previously approved these changes Jun 27, 2025
View reviewed changes
nibanks
nibanks reviewed Jun 30, 2025
View reviewed changes
Comment thread src/platform/storage_winuser.c
nibanks
nibanks previously approved these changes Jun 30, 2025
View reviewed changes
guhetier
guhetier previously approved these changes Jun 30, 2025
View reviewed changes
@gaurav2699 @nibanks
Update src/platform/storage_winuser.c
80e66b7 
Co-authored-by: Nick Banks <nibanks@microsoft.com>
@gaurav2699
gaurav2699 dismissed stale reviews from guhetier and nibanks via 80e66b7 June 30, 2025 19:22
@gaurav2699
gaurav2699 enabled auto-merge (squash) June 30, 2025 19:23
@gaurav2699
gaurav2699 requested a review from sachinrk July 1, 2025 08:40
@gaurav2699
gaurav2699 requested review from guhetier and nibanks July 1, 2025 15:31
@gaurav2699
gaurav2699 merged commit 2623c07 into main Jul 1, 2025
438 of 439 checks passed
@gaurav2699
gaurav2699 deleted the gamecore_error branch July 1, 2025 15:51
gaurav2699 added a commit that referenced this pull request Jul 25, 2025
@gaurav2699 @nibanks
Removed RegDeleteTreeA usage in storage_winuser (#5182)
2699a64 
* added RegDeleteTreeA definition in restricted build

* removed usage of RegDeleteTreeA

* updated clog

* max short

* added comment

* added RegQuertyInfKeyA

* added line

* Update src/platform/storage_winuser.c

Co-authored-by: Nick Banks <nibanks@microsoft.com>

---------

Co-authored-by: Nick Banks <nibanks@microsoft.com>
gaurav2699 added a commit that referenced this pull request Jul 25, 2025
@gaurav2699 @nibanks
Backport gamecorefixes in release/2.5 (#5277)
4c20a45 
* Removed RegDeleteTreeA usage in storage_winuser (#5182)

* added RegDeleteTreeA definition in restricted build

* removed usage of RegDeleteTreeA

* updated clog

* max short

* added comment

* added RegQuertyInfKeyA

* added line

* Update src/platform/storage_winuser.c

Co-authored-by: Nick Banks <nibanks@microsoft.com>

---------

Co-authored-by: Nick Banks <nibanks@microsoft.com>

* whitespace to trigger whitespace

* remove whitespace to trigger CI

---------

Co-authored-by: Nick Banks <nibanks@microsoft.com>
@gaurav2699 gaurav2699 linked an issue Aug 20, 2025 that may be closed by this pull request
Migrate Internal OneBranch Automation to Undock Repo #4766
Open
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guhetier guhetier guhetier approved these changes
@mtfriesen mtfriesen mtfriesen left review comments
@anrossi anrossi anrossi left review comments
@nibanks nibanks Awaiting requested review from nibanks
+1 more reviewer
@sachinrk sachinrk sachinrk approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Migrate Internal OneBranch Automation to Undock Repo

6 participants

@gaurav2699 @nibanks @mtfriesen @anrossi @guhetier @sachinrk