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.

Sign up for GitHub

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

Jump to bottom

Replace custom path handling in MRT #908

Merged
merged 3 commits into from
Jun 4, 2021
Merged

Replace custom path handling in MRT #908

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
04ba502
Replace custom path handling
huichen123 Jun 3, 2021
d73e714
use default size
huichen123 Jun 4, 2021
a221fdd
Merge branch 'main' into user/huichen/pathparse
huichen123 Jun 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions build/build-mrt.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

#name: $(BuildDefinitionName)-$(date:yyMM).$(date:dd)$(rev:rrr)
parameters:
appxPackageDir: '$(build.artifactStagingDirectory)\AppxPackages\\'
MRTSourcesDirectory: $(Build.SourcesDirectory)\dev\MRTCore
MRTBinariesDirectory: $(Build.SourcesDirectory)\BuildOutput

Expand Down Expand Up @@ -114,7 +113,7 @@ steps:
platform: '$(buildPlatform)'
solution: '${{ parameters.MRTSourcesDirectory }}\mrt\MrtCore.sln'
configuration: '$(buildConfiguration)'
msbuildArguments: '/restore /p:AppxBundlePlatforms="$(buildPlatform)" /p:AppxPackageDir="${{ parameters.appxPackageDir }}" /p:AppxBundle=Always /p:UapAppxPackageBuildMode=StoreUpload /binaryLogger:$(Build.SourcesDirectory)/mrtcore.$(buildPlatform).$(buildConfiguration).binlog'
msbuildArguments: '/restore /binaryLogger:$(Build.SourcesDirectory)/mrtcore.$(buildPlatform).$(buildConfiguration).binlog'

- task: PublishBuildArtifacts@1
displayName: 'Publish mrtcore binlog'
Expand Down
83 changes: 26 additions & 57 deletions dev/MRTCore/mrt/Core/src/MRM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License. See LICENSE in the project root for license information.

#include <Windows.h>
#include <Pathcch.h>
#include "wil/win32_helpers.h"
#include "wil/filesystem.h"
#include "mrm/BaseInternal.h"
#include "mrm/common/platform.h"
#include "mrm/readers/MrmReaders.h"
Expand Down Expand Up @@ -874,49 +876,20 @@ STDAPI MrmGetFilePathFromName(_In_opt_ PCWSTR filename, _Outptr_ PWSTR* filePath
{
*filePath = nullptr;

wchar_t path[MAX_PATH];
DWORD size = ARRAYSIZE(path);
DWORD length = GetModuleFileName(nullptr, path, size);
DWORD lastError = GetLastError();
std::unique_ptr<wchar_t[]> pathAllocated;
wil::unique_cotaskmem_string path;
RETURN_IF_FAILED((wil::GetModuleFileNameW<wil::unique_cotaskmem_string, 256>(nullptr, path)));
Copy link
Member

Choose a reason for hiding this comment

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

You can leave off the , 256 parameter and then remove the additional ( ) on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

256 is what i want to override. the default is 128. Many times, the path created by the reunion template is fairly long. Thus i want to bump up the default.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it better to use MAX_PATH instead of 256 here? MAX_PATH may be greater than 256 after a user modification, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

256 is just twice of the default (128). Some people may just don't like MAX_PATH. I doubt anyone is going to modify MAX_PATH. If it does, that's just one more reason to not use MAX_PATH.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're trading codebase readability for a perceived micro-optimization. Other folks coming across this code are going to scratch their heads as to why the initial buffer size was not used here. Something to consider.

Copy link
Member

Choose a reason for hiding this comment

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

It's better to use no number here. WIL always produces a heap-allocated string from this method, and the buffer parameter is just a seed to know where to start growing the buffer. Just don't specify, let WIL do its thing, then there's no Mysterious Constant hanging out here. (Mysterious Constants in library implementations are fine.)


while ((length == size) && (lastError == ERROR_INSUFFICIENT_BUFFER))
{
size *= 2;
RETURN_HR_IF(E_UNEXPECTED, size > 32 * 1024);

pathAllocated.reset(new (std::nothrow) wchar_t[size]);
RETURN_IF_NULL_ALLOC(pathAllocated);

length = GetModuleFileName(nullptr, pathAllocated.get(), size);
lastError = GetLastError();
}

RETURN_HR_IF(HRESULT_FROM_WIN32(lastError), lastError != 0);

// Remove module file name
PWSTR pointerToPath = pathAllocated ? pathAllocated.get() : path;
PWSTR lastSlash = nullptr;
PWSTR secondToLastSlash = nullptr;
while (*pointerToPath)
{
if (*pointerToPath == L'\\')
{
secondToLastSlash = lastSlash;
lastSlash = pointerToPath;
}
pointerToPath++;
}
PCWSTR name = wil::find_last_path_segment(path.get());
Copy link
Member

Choose a reason for hiding this comment

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

Should this use the std::filesystem APIs instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The file contains no-throwing code only. with std, i am going to worry about whether it throws, and std::filesystem API does throw.

wchar_t moduleFilename[MAX_PATH];
Copy link
Contributor

Choose a reason for hiding this comment

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

883-884 can go into the "// Prep for the second iteration" if block where if name is empty/null, we just break. We can avoid the unnecessary setting of module name to path and then a second iteration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

at that time, path is already modified, and lost the name.

RETURN_IF_FAILED(StringCchCopyW(moduleFilename, ARRAYSIZE(moduleFilename), *name ? name : path.get()));

pointerToPath = pathAllocated ? pathAllocated.get() : path;
size_t length;
RETURN_IF_FAILED(StringCchLengthW(path.get(), STRSAFE_MAX_CCH, &length));

wchar_t moduleFilename[MAX_PATH];
RETURN_IF_FAILED(StringCchCopyW(moduleFilename, ARRAYSIZE(moduleFilename), lastSlash != nullptr ? lastSlash + 1 : pointerToPath));
size_t bufferCount;
RETURN_IF_FAILED(SizeTAdd(length, 1, &bufferCount));

if (lastSlash != nullptr)
{
*(lastSlash + 1) = 0;
}
RETURN_IF_FAILED(PathCchRemoveFileSpec(path.get(), bufferCount));

std::unique_ptr<wchar_t, decltype(&MrmFreeResource)> finalPath(nullptr, MrmFreeResource);

Expand All @@ -935,24 +908,25 @@ STDAPI MrmGetFilePathFromName(_In_opt_ PCWSTR filename, _Outptr_ PWSTR* filePath
// - if not exist, search under same path with [modulename].pri
for (int i = 0; i < 2; i++)
{
size_t lengthInSizeT;
RETURN_IF_FAILED(StringCchLengthW(pointerToPath, STRSAFE_MAX_CCH, &lengthInSizeT));
length = static_cast<DWORD>(lengthInSizeT);
size_t lengthOfName;
RETURN_IF_FAILED(StringCchLengthW(filenameToUse, STRSAFE_MAX_CCH, &lengthOfName));

RETURN_IF_FAILED(StringCchLengthW(filenameToUse, STRSAFE_MAX_CCH, &lengthInSizeT));
DWORD lengthOfName = static_cast<DWORD>(lengthInSizeT);
// We over-allocate a little bit so that we don't have to calculate the path length each time.
RETURN_IF_FAILED(SizeTAdd(bufferCount, lengthOfName, &length));

RETURN_IF_FAILED(DWordAdd(length, lengthOfName, &length));
RETURN_IF_FAILED(DWordAdd(length, 1, &length));
RETURN_IF_FAILED(DWordMult(length, sizeof(wchar_t), &size));
size_t size;
RETURN_IF_FAILED(SizeTMult(length, sizeof(wchar_t), &size));

PWSTR rawOutputPath = reinterpret_cast<PWSTR>(MrmAllocateBuffer(size));
RETURN_IF_NULL_ALLOC(rawOutputPath);

std::unique_ptr<wchar_t, decltype(&MrmFreeResource)> outputPath(rawOutputPath, MrmFreeResource);

RETURN_IF_FAILED(StringCchCopyW(outputPath.get(), length, pointerToPath));
RETURN_IF_FAILED(StringCchCatW(outputPath.get(), length, filenameToUse));
RETURN_IF_FAILED(PathCchCombineEx(
outputPath.get(),
size,
path.get(),
filenameToUse,
PATHCCH_ALLOW_LONG_PATHS));

DWORD attributes = GetFileAttributes(outputPath.get());

Expand All @@ -979,12 +953,7 @@ STDAPI MrmGetFilePathFromName(_In_opt_ PCWSTR filename, _Outptr_ PWSTR* filePath
else
{
// move to parent folder
if (secondToLastSlash == nullptr)
{
break;
}

*(secondToLastSlash + 1) = 0;
RETURN_IF_FAILED(PathCchRemoveFileSpec(path.get(), bufferCount));
}
}
}
Expand Down