Pre linux.pagecache.recoverfs support#1561
Pre linux.pagecache.recoverfs support#1561ikelos merged 14 commits intovolatilityfoundation:developfrom
Conversation
ikelos
left a comment
ikelos
left a comment
There was a problem hiding this comment.
Thanks, generally looks good, just the broken-out function doesn't stick to the general convention we've got going for plugin API methods and I'm trying to stick to it for as long as possible...
Also, there's an int cast that I think is unnecessary... 5;)
| access_time_dt = self.inode.get_access_time() | ||
| modification_time_dt = self.inode.get_modification_time() | ||
| change_time_dt = self.inode.get_change_time() | ||
| inode_size = int(self.inode.i_size) |
There was a problem hiding this comment.
Why is this being cast to an int? Is it not already an int? Please check, I'm really try to avoid people including unnecessary casts because they don't expect something to be an int, because then other people see it and they think they need to cast to an int, then some people do it out of caution, and before you know it the whole codebase is bursting full of pointless casts...
There was a problem hiding this comment.
This was explained by gcmoreira, in the comment right before the added line:
Ensure all types are atomic immutable. Otherwise, astuple() will take a long time doing a deepcopy of the Volatility objects.
I was also able to verify that not casting it to int, like the other properties, results in nested deepcopies.
|
|
||
| @staticmethod | ||
| def write_inode_content_to_stream( | ||
| inode: interfaces.objects.ObjectInterface, |
There was a problem hiding this comment.
This is starting to make use of more complex data types (not too bad) and break away from the standard format for methods (first param context). I'd really prefer this take a context and a layer_name rather than just a layer.
I don't have very good reason for sticking with convention, originally it was because everything you'd do needed a context (and if you wanted to extend this in the future, you'll likely want the context there to do it). It was also the thought in the back of my mind of one day serializing such calls for some reason, although I no longer remember what (perhaps parallelization). Regardless, I think it'd be a good convention to maintain unless you have strong objections...
There was a problem hiding this comment.
I basically followed the write_inode_content_to_file design, as it was the accepted way of doing it.
I also prefer to pass a context and a layer name, happy to revert it that way !
There was a problem hiding this comment.
Yes please, I try not to be too nitpicky but it's hard keeping everything in order as it comes in so quickly. So it's be MINOR bump if we're just adding write_inode_content_to_stream and MAJOR bump if we're changing the signature of write_inode_content_to_file.
There was a problem hiding this comment.
I proceeded to update write_inode_content_to_file as well.
|
Ok, looks good, thanks! |
|
Hey @ikelos, the changes in this PR are good. However, the timing is challenging, as @atcuno and I are currently focused on stabilizing several core APIs: Page Cache, Mountinfo, VMA, tasks, etc with related PRs still under review. Could we consider temporarily reverting this PR and revisiting it once the pending parity release PRs are merged and @atcuno confirms that the changes have passed the full regression tests? Additionally, I would greatly appreciate it, and it seems prudent, to delay the merging of any other pending or future PRs that impact this code until the parity release PRs have been finalized. Thanks |
|
|
||
| @staticmethod | ||
| def format_symlink(symlink_source: str, symlink_dest: str) -> str: | ||
| return f"{symlink_source} -> {symlink_dest}" |
There was a problem hiding this comment.
Not too fan of this format_symlink staticmethod. I'm not sure about the benefit of including a method in the inode data class that simply formats two external strings to 's1 -> s2'?
I think it would make more sense, and it's my mistake, but _follow_symlink should ideally be a public method in the inode object extension
There was a problem hiding this comment.
A future implementation will require to format symlinks given an inode as well. Unifying it to a function allows to prevent code duplication, even if it might seem futile for one line indeed.
I put it under InodeUser as it is in fact only needed to make it more user readable after rendering 👍.
3 participants
Hi,
To prepare for the new
linux.pagecache.recoverfsplugin, I figured it would be better to split changes non directly related to it in a separate PR.A few readability tweaks were operated, as well as splitting the
inodecontent extraction and writing process to allow for better flexibility.The
InodeSizecolumn was added toFilesoutput, as I think it is a valuable piece of information. It will also complement very welllinux.pagecache.RecoverFsRecovered FileSizecolumn, as the following snippet demonstrates (scroll right to the last columns) :As you can see, this will provide easy lookups to compare the recovered file size to the announced size. It also arguments the choice of placing
InodeSizeat the very end. Happy to discuss it however !Versioning
linux.pagecache.Files
linux.pagecache.InodePages