Update LVM2Device sizes if the LVM2Device size is smaller than the PhysicalVolume size#61
Update LVM2Device sizes if the LVM2Device size is smaller than the PhysicalVolume size#61
Conversation
|
@tuttimann I did find a issue in our lvm implementation, I believe this is also the issue you came accross in fox-it/dissect#93 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #61 +/- ##
==========================================
+ Coverage 77.67% 77.88% +0.20%
==========================================
Files 29 29
Lines 2343 2360 +17
==========================================
+ Hits 1820 1838 +18
+ Misses 523 522 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ysicalVolume size
It takes the pv size of the PhysicalVolume and uses it when opening the device
the size of the LVM2Device has been manually changed. While the test functions as intended, you can't use a loopback device to mount the file itself
Co-authored-by: twiggler <12800443+twiggler@users.noreply.github.com>
Miauwkeru
force-pushed
the
update-device-sizes
branch
from
72f2bd1 to
0301a81
Compare
dissect/volume/lvm/metadata.py
Outdated
|
|
||
| size = max(stream.size, self.size) | ||
| runlist = [ | ||
| (area.offset // SECTOR_SIZE, (area.size or size) // SECTOR_SIZE) for area in self.dev._data_area_descriptors |
There was a problem hiding this comment.
This (area.size or size) is bothering me. I know it's also in the original open of LVM2Device, and I know I'm the one who put it there, but I don't understand why this logic is this way? Is this in the case there's only one data area that does not have a size specified?
I assume you have the internals of LVM2 better in your head right now than I do, could you comment on this?
There was a problem hiding this comment.
I looked a bit into it to be sure.
Seems a pv currently only supports one data area per pv, according to the lvm2 code.
This size is always put on 0.
- https://gitlab.com/lvmteam/lvm2/-/blame/main/lib/cache/lvmcache.c?ref_type=heads#L2717
- https://gitlab.com/lvmteam/lvm2/-/blob/main/lib/cache/lvmcache.c?ref_type=heads#L2812
There is also more evidence in the documentation / code of lvm2 that it only populates one data area, and always puts the size to 0 in the pvck. (https://gitlab.com/lvmteam/lvm2/-/blob/main/tools/pvck.c?ref_type=heads#L2425)
Before my the research it, I thought it could be 0 if and only if there was just one data area available. Where the 0 denoted the whole size of the disk. Which was also the assumption here I believe.
I do have a feeling that once multiple data areas are allowed in the PV, the 0 means that it should be as large as the remaining size. So in that case the current code would be wrong. But that is just speculation on my part
We can add a comment to it why it is like this. Or just use size for now and add other logic later once they use more data area sections. Ofc, with the later option also having a comment about the why.
There was a problem hiding this comment.
Maybe raise an error if there's more than one data area, and remove the list comprehension?
There was a problem hiding this comment.
that's fine, I'll make a patch for it
There was a problem hiding this comment.
Can you structure this file similarly to how it's done in fox-it/dissect.evidence#42?
There was a problem hiding this comment.
As in directory structure or file structure. A "yes" would be valid for both
3 participants
This seems to be the cause of fox-it/dissect#93
Where it, assumedly, tried to read beyond the LVM2Device disk size where it could not reach.
TODO