-
Notifications
You must be signed in to change notification settings - Fork 451
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
Include panic logs for vfs in diagnose #1408
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments, beyond that this looks reasonable to me. However it's in code that's unfamiliar to me, so I'll leave approval to someone better versed in this code.
|
||
foreach (string filePath in Directory.GetFiles(DiagnosticReportsDirectory, PanicFileExtention)) | ||
{ | ||
if (File.ReadAllText(filePath).Contains(ProjFSKext.DriverName)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will match any panics for which the ProjFS kext was loaded, not just those where it appeared in the stack trace. (There is a list of loaded kexts at the end of each panic log.) Is that the intended behaviour? Either way, it might be worth commenting on the intention of this check here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
Is that the intended behaviour?
I think the intended behavior is to only include the panics where ProjFS kext appeared in the stack trace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at several panics it appears that if we're in the backtrace, we'll have the form org.vfsforgit.PrjFSKext(
.
Would you advise that as the simplest way to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed this out-of-band and this does seem like the simplest solution.
I'm wondering if we can remove the
plus the panic logs? I'm not sure how much cleaner this would be or what you would need to pass to the method but I saw that we were already doing some |
@kewillford an exellent point and something I certainly though about. TryFlushLogs has a different purpose and I think putting it all in one fuctions gives that function too much responsibility. What we have here is pretty explicit (and hopefully easy to understand! :-) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me now!
resolves #1395