Implement close method for containers (fox-it#100) #104
Conversation
|
@Kafow thank you for your contribution! As this is your first code contribution, please read the following Contributor License Agreement (CLA). If you agree with the CLA, please reply with the following information:
Contributor License Agreement
Contribution License AgreementThis Contribution License Agreement ("Agreement") governs your Contribution(s) (as defined below) and conveys certain license rights to Fox-IT B.V. ("Fox-IT") for your Contribution(s) to Fox-IT"s open source Dissect project. This Agreement covers any and all Contributions that you ("You" or "Your"), now or in the future, Submit (as defined below) to this project. This Agreement is between Fox-IT B.V. and You and takes effect when you click an “I Accept” button, check box presented with these terms, otherwise accept these terms or, if earlier, when You Submit a Contribution.
|
|
Hi @Kafow, sorry for the delay. Please give us a moment to review and check how we can best incorporate these changes to make sure everything works as intended! |
|
@DissectBot agree |
| @@ -37,4 +37,4 @@ def tell(self) -> int: | |||
| return self.qcow2.tell() | |||
|
|
|||
| def close(self) -> None: | |||
| pass | |||
| self.qcow2.fh.close() | |||
There was a problem hiding this comment.
Thanks for looking into the container close() functions! They have been waiting to be implemented, but so far we haven't yet gotten to that.
I think the best place to implement the functionality is in the respective file format implementations (e.g. QCow, VDI, etc.), and call their close() method, similar as how it is already done for AsdfContainer and EwfContainer. The file format implementations should be responsible for managing their resources as they know about the details of them. This becomes clear in case of VMDK later on.
Of course the SplitContainer is fully defined and managed here, so the close() you implemented there is ok.
A side note on AsdfContainer and EwfContainer: they already use the close() method of the underlying implementation, which in case of EwfContainer is effectively a no-op, so feel free to also look at that. The AsdfContainer.close() is even worse as the ASDF implementation it doesn't even have a close(), I'll create a bug for that on our internal ticketing system.
There was a problem hiding this comment.
I understand that from the library as a whole perspective and I mostly agree.
If I understand you correctly, I should open a PR in each of the underlying format implementations (e.g. qcow2 in dissect.hypervisor) and implement close over there right?
BTW, I tried to trace the close call of ASDF and EWF, and noticed that they are deriving it from dissect.utils.AlignedStream which is also doing nothing :)
Due to the fact every disk implementation is deriving from dissect.utils.AlignedStream, I suggest we create consistency between EWF, ASDF and the rest, and do the same to prepare for overriding close method in each disk implantation.
| self.qcow2.fh.close() | |
| self.qcow2.close() |
There was a problem hiding this comment.
I looked a bit longer at the issue and have to revise my suggestion a bit.
As a general rule, you want the object that opens a file handle to be responsible for closing it, as that is where most information is available about what is going on with that file handle. The place this happens in the dissect code turns out to be more variable then I expected.
Most of the file handle opening happens in the Container sub-classes in dissect.target, so that is where the close() method should be implemented. However, most of the Container sub-classes (with the exception of SplitContainer) do not yet store the opened file handles in an instance variable, so that should be fixed.
There are 3 Container sub-classes that are an exception to this: AsdfContainer, VhdxContainer and VmdkContainer. Here the underlying container implementation of these 3 opens the file handle itself, it also may open other file handles. So for these 3 the close() method should be implemented in AsdfSnapshot (in target.evidence), VHDX and VMDK (both in target.hypervisor). These can then be called by the close() of their Container sub-class.
Finally there is the big exception on all of this that the Container sub-classes and those 3 underlying implementation classes also allow an already opened file handle to be passed. But that is something we can ignore for now.
I don't think AlignedStream is the right place to implement the close(), as it is just an abstract reader of an IO stream. It doesn't know anything about file handles, so it shouldn't concern itself with it.
There was a problem hiding this comment.
I think that before we get into fixing the issue of closing the file handles, we need to decide where should we open the file handle. Now the inconsistency between those 3 containers, and other container implementation is just making more confusion in the code and whose the one managing the resources.
IMO It should be in the container implementation due to how SplitContainer is implemented, but I might miss some other dissect use cases of disk implementation that such change might break.
And I totally agree that the file handle should be instance variable everywhere.
There was a problem hiding this comment.
When implementing it in the Container class, there are still the 3 execptions that need to do it in the underlying implementation (AsdfContainer, VhdxContainer and VmdkContainer). So for me it makes more sense to do it in the underlying implementation.
This would mean accepting Paths (and opening them) as well as open file handles. But that would be ok as we want the underlying implementations to be usable on their own. SplitContainer and RawContainer do not have an underlying implementation, so those two will need to implement the close() necessarily in their own class.
We could sub-class AlignedStream into something called AlignedFileStream which would implement the close() and an __init__() that deals with file Path(s) and open file handle(s) and keeps hem around.
One other thing that close() and __init__() need to do is to keep track of whether the file was opened by the instance or if an already open file handle was passed.
As AlignedStream (and also Container) is sub-classed from io.RawIOBase (resp. io.IOBase), close() will be called on destruction of an instance. Which is something that happens when the instance goes out of scope. If the instance was passed an already open file handle, it would be closed while the caller would not be aware of the closing, which would lead to all kinds of issues.
| @@ -35,4 +35,5 @@ def tell(self) -> int: | |||
| return self.vmdk.tell() | |||
|
|
|||
| def close(self) -> None: | |||
| pass | |||
| for disk in self.vmdk.disks: | |||
There was a problem hiding this comment.
VMDK.disks can have a .parent VMDK which also has .disks etc. These should also be iterated over and closed.
#100