Mac: Don't assert/panic when we fail to read attributes during a FileOp operation #451
Conversation
| assert(TryGetFileIsFlaggedAsInRoot(currentVnode, context, &fileFlaggedInRoot)); | ||
| if (!TryGetFileIsFlaggedAsInRoot(currentVnode, context, &fileFlaggedInRoot)) | ||
| { | ||
| KextLog_Info("Failed to read attributes when handling FileOp operation. Path is %s", path); |
There was a problem hiding this comment.
Can you add a // Mac(TODO) comment here that this KextLog_Info should be added to the kext log file?
There was a problem hiding this comment.
(I suspect that once we have a log file implementation we won't want to include all events in it, but this seems like one we will want)
There was a problem hiding this comment.
I would argue that we do want to capture all kext log statements into the log file. Why else would we record them?
There was a problem hiding this comment.
I agree with Saeed, if you look at our existing use of KextLog, we log all those messages today and I can't imagine when we wouldn't want all of these messages to go to the logger.
If it ends up being problematic down the line, we could always revisit.
There was a problem hiding this comment.
Why else would we record them?
If we wanted to take the same approach as ProjFS on Windows a subset of our logging calls would always be written to file and the rest would be disabled unless a user enables verbose logging (to track down specific issues they might have encountered).
we log all those messages today and I can't imagine when we wouldn't want all of these messages to go to the logger.
Perhaps we start with only logging error and\or critical events?
I think there's going to be a balance between log file size, and how far back we can go in log file history (assuming we do circular logging).
There was a problem hiding this comment.
(All this logging discussion is outside the scope of this PR, so I don't think there's any need to block on it)
Historically, many of us have seen a panic on reading the attempting to read the attributes on some path outside of our virtualization root (Most reports of this have been when no virtualization root is attached). For now, let's avoid the panic here and log the path where the failure occurred.