Avoid an exception if file system does not support reparse points by iSazonov · Pull Request #13634

Avoid an exception if file system does not support reparse points by iSazonov · Pull Request #13634 · PowerShell/PowerShell

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

Conversation

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

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
Breaking changes
- None
- OR
- Experimental feature(s) needed
  - Experimental feature name(s):
User-facing changes
- Not Applicable
- OR
- Documentation needed
  - Issue filed:
Testing - New and feature
- N/A or can only be tested interactively
- OR
- Make sure you've added a new test if existing tests do not effectively test the code changed
Tooling
- I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
- OR
- I have considered the user experience from a tooling perspective and enumerated concerns in the summary. This may include:
  - Impact on PowerShell Editor Services which is used in the PowerShell extension for VSCode (which runs in a different PS Host).
  - Impact on Completions (both in the console and in editors) - one of PowerShell's most powerful features.
  - Impact on PSScriptAnalyzer (which provides linting & formatting in the editor extensions).
  - Impact on EditorSyntax (which provides syntax highlighting with in VSCode, GitHub, and many other editors).

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

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?

@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.

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?

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

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?

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

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 removed the Waiting on Author

The PR was reviewed and requires changes or comments from the author before being accept

label

Sep 28, 2020

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?

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.

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.

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.

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.

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?

Co-authored-by: Robert Holt <rjmholt@gmail.com>

Co-authored-by: Robert Holt <rjmholt@gmail.com>

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).