Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Avoid an exception if file system does not support reparse points#13634

Merged
rjmholt merged 6 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
iSazonov:no-reparse-point-supportiSazonov/PowerShell:no-reparse-point-supportCopy head branch name to clipboard
Dec 3, 2020
Merged

Avoid an exception if file system does not support reparse points#13634
rjmholt merged 6 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
iSazonov:no-reparse-point-supportiSazonov/PowerShell:no-reparse-point-supportCopy head branch name to clipboard

Conversation

@iSazonov

@iSazonov iSazonov commented Sep 15, 2020

Copy link
Copy Markdown
Collaborator

PR Summary

Fix #13605.

We should return null if ERROR_INVALID_FUNCTION - the error code in the context implicitly says that reparse points are not supported.

PR Context

PR Checklist

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Sep 15, 2020
@iSazonov iSazonov requested a review from anmenaga as a code owner September 15, 2020 11:49
@ghost ghost assigned rjmholt Sep 15, 2020
@DHowett

DHowett commented Sep 15, 2020

Copy link
Copy Markdown

I'm slightly worried that since these error codes are provided by the filesystem driver, there might be an infinite amount of them. What if another filesystem driver we've not yet seen fails this question in a different way, like with ERROR_INVALID_ARGUMENT or something? Is it reasonable to log a non-fatal error somewhere and continue with the "not a reparse point" case for all errors?

@iSazonov

Copy link
Copy Markdown
Collaborator Author

@DHowett An alternative could be ignoring any error and return null but I think it is not good practice to hide errors which we do not know.

@iSazonov

Copy link
Copy Markdown
Collaborator Author

Test confirmation #13605 (comment)

@vexx32

vexx32 commented Sep 17, 2020

Copy link
Copy Markdown
Collaborator

Hmm. While ignoring errors from this check may not be the best idea, the purpose of this check is just to verify if the path is a reparse point, right? So if it errors out, it might be OK to assume it's probably not and log the error the driver reports somewhere without outright failing the overall cmdlet operation?

@ghost ghost added the Review - Needed The PR is being reviewed label Sep 25, 2020
@ghost

ghost commented Sep 25, 2020

Copy link
Copy Markdown

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@rjmholt

rjmholt commented Sep 28, 2020

Copy link
Copy Markdown
Collaborator

It sounds like we're not yet resolved on the right course of action for this PR. It feels to me like we need to be tolerant of errors at this level, but let the information that some error has occurred bubble up to the user in some way.

Writing to the error stream would be misleading, if the actual intended action didn't fail. But if we experience a real failure, how will we know? Perhaps a real failure will surface in a later error, but we can ensure that this earlier failure is recorded somewhere.

Is there anywhere we could consistently log that cross-platform, or is the warning stream a better place perhaps?

@ghost ghost removed the Review - Needed The PR is being reviewed label Sep 28, 2020
@rjmholt rjmholt added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Sep 28, 2020
@iSazonov

iSazonov commented Sep 28, 2020

Copy link
Copy Markdown
Collaborator Author

I agree that we can ignore the error in main code path but my concern is that we lost information in the problematic area (PowerShell does not work well with reparse points.).
We could add the information (check on reparse point) in tracing. Thoughts?

@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Sep 28, 2020
@rjmholt

rjmholt commented Sep 28, 2020

Copy link
Copy Markdown
Collaborator

We could add the information (check on reparse point) in tracing

As in showing it in Trace-Command? Is there also another more prominent place we could add it?

@iSazonov

Copy link
Copy Markdown
Collaborator Author

I don't think it makes sense to somehow inform the user about this situation. It only makes sense if something goes wrong - user does not get expected result. In this case, tracing will help.

@rjmholt

rjmholt commented Sep 28, 2020

Copy link
Copy Markdown
Collaborator

It only makes sense if something goes wrong - user does not get expected result.

Right. The question is, how reliably does PowerShell know if something goes wrong?

If possible I'd prefer this not to be a subtle behaviour that people are forced to use a trace to unravel. I suspect many users would be mystified by it, not use a trace, and either give up or open an issue, where they'll tell us that we should be throwing an error.

Rather, we can include it in the trace, but if we end up throwing an error at the end of the action, it might be nice to include the information from the reparse point processing.

@iSazonov

Copy link
Copy Markdown
Collaborator Author

We know that the scenario works well without the exception. Only concern is there is a scenario where it could not work because reparse point is generic and sometimes unpredictable from our experience. For diagnostic we have to use a debugger that could be complex but we could use more simple tracing as in other code paths in the file system provider.

@ghost ghost added the Review - Needed The PR is being reviewed label Oct 6, 2020
@ghost

ghost commented Oct 6, 2020

Copy link
Copy Markdown

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@iSazonov

iSazonov commented Oct 8, 2020

Copy link
Copy Markdown
Collaborator Author

I tried to do minimal changes but after looking more I think the original code confuses and we should simplify it - fast return if we can not read a reparse point and it doesn't matter for whatever reason it happens - it is not a reparse point or the file system doesn't support reparse points or something else.

@ghost ghost removed the Review - Needed The PR is being reviewed label Oct 8, 2020

@rjmholt rjmholt left a comment

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.

Is there the possibility of adding a log or trace before we return null?

Comment thread src/System.Management.Automation/namespaces/FileSystemProvider.cs Outdated
Comment thread src/System.Management.Automation/namespaces/FileSystemProvider.cs Outdated
iSazonov and others added 2 commits October 9, 2020 08:08
@iSazonov

iSazonov commented Oct 9, 2020

Copy link
Copy Markdown
Collaborator Author

Is there the possibility of adding a log or trace before we return null?

@rjmholt I think now it makes no sense. Before the fix we have a tricky code paths which made us think that there were unpredictable scenarios. In fact, the source code contained an error because it allowed handling an uninitialized structure (after lastError == ERROR_NOT_A_REPARSE_POINT we go to switch).

@ghost ghost removed the Review - Needed The PR is being reviewed label Oct 16, 2020
@SteveL-MSFT SteveL-MSFT added this to the 7.1-Consider milestone Oct 16, 2020
@adityapatwardhan adityapatwardhan removed this from the 7.1-Consider milestone Oct 19, 2020
@ghost ghost added the Review - Needed The PR is being reviewed label Oct 27, 2020
@ghost

ghost commented Oct 27, 2020

Copy link
Copy Markdown

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

Comment thread src/System.Management.Automation/namespaces/FileSystemProvider.cs
@ghost ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Review - Needed The PR is being reviewed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Nov 1, 2020
@ghost ghost added the Review - Needed The PR is being reviewed label Nov 9, 2020
@ghost

ghost commented Nov 9, 2020

Copy link
Copy Markdown

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@SteveL-MSFT SteveL-MSFT left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@SteveL-MSFT SteveL-MSFT left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@SteveL-MSFT

Copy link
Copy Markdown
Member

Looks like you can't execute native commands in WinPE w/o this fix

@ghost ghost removed the Review - Needed The PR is being reviewed label Dec 2, 2020
@rjmholt

rjmholt commented Dec 2, 2020

Copy link
Copy Markdown
Collaborator

@PoshChan please remind me in 5 days

@daxian-dbw daxian-dbw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@rjmholt rjmholt merged commit 2804576 into PowerShell:master Dec 3, 2020
@iSazonov iSazonov deleted the no-reparse-point-support branch December 3, 2020 19:54
@iSazonov iSazonov added this to the 7.2.0-preview.2 milestone Dec 3, 2020
@PoshChan

PoshChan commented Dec 7, 2020

Copy link
Copy Markdown
Collaborator

@rjmholt, this is the reminder you requested 5 days ago

@ghost

ghost commented Dec 15, 2020

Copy link
Copy Markdown

🎉v7.2.0-preview.2 has been released which incorporates this pull request.:tada:

Handy links:

@ghost

ghost commented Jan 14, 2021

Copy link
Copy Markdown

🎉v7.1.1 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Backport-7.1.x-Done Backport to 7.1.x completed CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"Incorrect function" exception when running an exe not in C:\

9 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.