Skip to content

fs: Remove a couple debug logs #1015

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

Merged
merged 1 commit into from
Dec 7, 2023

Conversation

nicholasbishop
Copy link
Member

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
Member 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
Member

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 force-pushed the bishop-remove-trace branch from 9973f87 to 02770ef Compare December 7, 2023 10:02
@phip1611 phip1611 enabled auto-merge December 7, 2023 10:02
@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
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.

2 participants