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

WIP: Use newly added LinkTarget property #15960

Closed
wants to merge 4 commits into from

Conversation

jozkee
Copy link

@jozkee jozkee commented Aug 19, 2021

PR Summary

Replace the current logic of retrieving a Link's Target with the newly added API LinkTarget.

PR Context

From a libraries perspective, I wanted to get validation from product code that can implement this API so we can confirm that it works as expected in real world scenarios.

I'm sending this as draft as it builds on top of #15896.

cc @iSazonov @mklement0

PR Checklist

Comment on lines -8504 to -8517
case IO_REPARSE_TAG_MOUNT_POINT:
REPARSE_DATA_BUFFER_MOUNTPOINT reparseMountPointDataBuffer = Marshal.PtrToStructure<REPARSE_DATA_BUFFER_MOUNTPOINT>(outBuffer);
targetDir = Encoding.Unicode.GetString(reparseMountPointDataBuffer.PathBuffer, reparseMountPointDataBuffer.SubstituteNameOffset, reparseMountPointDataBuffer.SubstituteNameLength);
break;

case IO_REPARSE_TAG_APPEXECLINK:
REPARSE_DATA_BUFFER_APPEXECLINK reparseAppExeDataBuffer = Marshal.PtrToStructure<REPARSE_DATA_BUFFER_APPEXECLINK>(outBuffer);
// The target file is at index 2
if (reparseAppExeDataBuffer.StringCount >= 3)
{
string temp = Encoding.Unicode.GetString(reparseAppExeDataBuffer.StringList);
targetDir = temp.Split('\0')[2];
}
break;

Choose a reason for hiding this comment

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

We are currently only supporting symlinks, not mount points or apexeclinks. I don't think this should be deleted yet.

Copy link
Author

Choose a reason for hiding this comment

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

We can defer merging after .NET 6 RC1 is released (and consumed by pwsh), which will include that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it already in a PR?

Copy link
Author

Choose a reason for hiding this comment

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

@jozkee jozkee changed the title Use newly added LinkTarget property WIP: Use newly added LinkTarget property Aug 20, 2021
Comment on lines 2070 to +2072
if (InternalSymbolicLinkLinkCodeMethods.IsReparsePointLikeSymlink(fileInfo))
{
return $"{PSStyle.Instance.FileInfo.SymbolicLink}{fileInfo.Name}{PSStyle.Instance.Reset} -> {InternalSymbolicLinkLinkCodeMethods.GetTarget(instance)}";
return $"{PSStyle.Instance.FileInfo.SymbolicLink}{fileInfo.Name}{PSStyle.Instance.Reset} -> {fileInfo.LinkTarget}";
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be great to get rid of IsReparsePointLikeSymlink() call and check fileInfo.LinkTarget is not null but .Net API has slightly different behavior - can we enhance the .Net API?

// The name surrogate bit 0x20000000 is defined in https://docs.microsoft.com/windows/win32/fileio/reparse-point-tags
// Name surrogates (0x20000000) are reparse points that point to other named entities local to the filesystem
// (like symlinks and mount points).
// In the case of OneDrive, they are not name surrogates and would be safe to recurse into.
if ((data.dwReserved0 & 0x20000000) == 0 && (data.dwReserved0 != IO_REPARSE_TAG_APPEXECLINK))
{
return false;
}

(Also you can see as IsReparsePointLikeSymlink() is used in other places. It seems it work the same for directory removal scenario but directory enumeration requires enhaсing of .Net API)

Comment on lines -8504 to -8517
case IO_REPARSE_TAG_MOUNT_POINT:
REPARSE_DATA_BUFFER_MOUNTPOINT reparseMountPointDataBuffer = Marshal.PtrToStructure<REPARSE_DATA_BUFFER_MOUNTPOINT>(outBuffer);
targetDir = Encoding.Unicode.GetString(reparseMountPointDataBuffer.PathBuffer, reparseMountPointDataBuffer.SubstituteNameOffset, reparseMountPointDataBuffer.SubstituteNameLength);
break;

case IO_REPARSE_TAG_APPEXECLINK:
REPARSE_DATA_BUFFER_APPEXECLINK reparseAppExeDataBuffer = Marshal.PtrToStructure<REPARSE_DATA_BUFFER_APPEXECLINK>(outBuffer);
// The target file is at index 2
if (reparseAppExeDataBuffer.StringCount >= 3)
{
string temp = Encoding.Unicode.GetString(reparseAppExeDataBuffer.StringList);
targetDir = temp.Split('\0')[2];
}
break;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it already in a PR?

@rjmholt rjmholt marked this pull request as draft August 20, 2021 14:19
@daxian-dbw
Copy link
Member

@jozkee We are not moving to .NET 6 preview.7 due to a couple bugs in that preview (dotnet/roslyn#55609, dotnet/runtime#57107), and that's why #15896 stays as a draft.

For the new LinkTarget property, we first need to know if there is any behavior difference between the existing Target and the new LinkTarget. If there is no difference, then we will make Target an alias property in PowerShell that points to LinkTarget, because removing Target is a breaking change.

According to @carlossanlop, the functionality of LinkTarget is not in parity with the existing Target, so let's hold off this change until the functionality is proven to be in parity. I will close this PR for now.

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.

6 participants