-
Notifications
You must be signed in to change notification settings - Fork 480
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
vmware: Add warning when no metadata file is found for a vmem file. #1016
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.
Huh, well I hadn't realized it but apparently we now do require a metadata file with the vmem format. That wasn't previously required (because some images made by vmware were just flat images, and with no way to tell from the image itself).
I guess given we do already break without one, this message is ok, but given we might be wrong I'd prefer it were a little less doom-sayerish... 5;P Then happy to merge it.
@@ -232,6 +232,9 @@ def stack( | |||
) | |||
|
|||
if not vmss_success and not vmsn_success: | |||
vollog.warning( | |||
f"No metadata file alongside VMEM file! A VMSS or VMSN file is required to correctly process a VMEM file. These should be placed in the same directory with the same file name, e.g. sample.vmem and sample.vmsn.", |
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'd probably remove the exclamation mark, we're not trying to scare the user and this might be a mistake on our part (some vmem files don't strictly need an associated metadata file, but it's impossible to tell from the image alone). I'd say rather than file name, e.g. sample.vmem and sample.vmsn
, I'd say basename and appropriate file extension (vmsn or vmss)
.
@ikelos haha, yeah, I can tone down the warning. :D if some of them have been flat files in the past, is it worth looking into changing the logic back so it still tries? That way if it turns out to be okay without the metadata files then at least the user gets results. I'll have a dig through the commit history and see why that return none got added. Edit: thinking through, if it's just a flat file that'll get caught anyway as a normal physical layer, so it is right that the vmware layer returns none. In the case of those special flat files there is nothing extra that the vmware layer is adding? |
I'm not sure how common it would be, and I think there'd still be a metadata file, it would just say everything's in one big chunk starting at 0. If we allow it to load incorrectly, then it's possible that a later stacker which might have worked will fail, so I think the logic's ok. Correct, it's fine if it gets treated as a normal raw file (which again is why not to scare people with exclamation marks, since everything might actually be just fine)... 5;) |
@ikelos - I've updated the warning to be less doom and gloom. What do you think? I don't mind changing it again if needed, much rather it was the right tone. Help users that get stuck without the metadata files, but not scare users when it is working as expected. You're completely right about the flat files not needing the vmware layer, once I thought about it a little it was obvious. The warning would look like this now:
|
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.
Yep, looks good, thanks for making the changes! 5:)
Hello,
Richard Davis of 13cubed (@13Cubed) posted a video on YouTube showing how to process a vmem file the metadata files are also needed. Highlighting how it isn't very obvious. Not just for vol, but other tools as well. The verbose logs do show this, however you'd have to know what you are looking for. I've seen a few instances of this problem affecting people in the slack community.
This PR simply adds a warning when vol is unable to find the metadata files, which might mean less people struggle with this as a warning is shown:
Here is the video: https://www.youtube.com/watch?v=P0yw93GJsYU
Thanks!