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

fs: Remove a couple debug logs #1015

Merged
merged 1 commit into from Dec 7, 2023

Conversation

nicholasbishop
Copy link
Contributor

The trace log in open can be a bit misleading since at first glance it looks like an error, but in some cases it's expected for open to fail, like when calling try_exists.

The debug log in create_dir_all is showing internal details so probably not all that useful to end users.

Checklist

  • Sensible git history (for example, squash "typo" or "fix" commits). See the Rewriting History guide for help.
  • Update the changelog (if necessary)

@nicholasbishop
Copy link
Contributor Author

I debated a bit here as to whether it might be better to go the other direction here, and add more logs at trace or debug level, e.g. on each entry to open we could trace the path and other parameters. In other parts of the uefi crate we have very little logging, so went with removing -- but I'd be open to a different approach.

(My main motivation for this change is that I spent a little time being confused about why I was getting a file-open error, until I realized it was trace logging happening when calling write on a new file; write calls try_exists, which calls open.)

@phip1611
Copy link
Contributor

phip1611 commented Dec 7, 2023

I'm in favor of removing them. The interface seems to be stable enough (no bug reports in the last couple of months), so we can also remove this very verbose logging - especially as it results in confusions

The trace log in `open` can be a bit misleading since at first glance it looks
like an error, but in some cases it's expected for open to fail, like when
calling `try_exists`.

The debug log in `create_dir_all` is showing internal details so probably not
all that useful to end users.
@phip1611 phip1611 added this pull request to the merge queue Dec 7, 2023
Merged via the queue into rust-osdev:main with commit 5de89dd Dec 7, 2023
12 checks passed
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.

None yet

2 participants