On 01/14/16 09:48, Janosch Frank wrote:
> The dump guest memory script for extracting a Linux core from a qemu
> core is currently limited to amd64 and python 2.
>
> With this series we add support for python 3 (while maintaining python
> 2 support) and add the possibility to extract dumps from VMs with the
> most common architectures.
>
> This was tested on a s390 s12 guest only, I'd appreciate tests for the
> other architectures.
>
> Janosch Frank (5):
> scripts/dump-guest-memory.py: Move constants to the top
> scripts/dump-guest-memory.py: Make methods functions
> scripts/dump-guest-memory.py: Improve python 3 compatibility
> scripts/dump-guest-memory.py: Cleanup functions
> scripts/dump-guest-memory.py: Introduce multi-arch support
>
> scripts/dump-guest-memory.py | 717
> +++++++++++++++++++++++++++----------------
> 1 file changed, 453 insertions(+), 264 deletions(-)
>
So, I had a few notes for patches 1-4, but those are just insignificant
nits, so address them or not, I'm fine.
Also, I'm not a Python programmer (you can probably tell from the
source). For every three lines I wrote for this script, I had to stare
at basic Python documentation, and PEP-8, for five minutes. :)
Moving out a bunch of stuff to global namespace (from classes) in the
initial patches is fine I guess; but maybe keeping then in the class
helps with avoiding namespace collisions if a user loads other
extensions into gdb. IIRC that was my main motivation to keep those
things within the class. But, I don't feel strongly about this at all.
Patch 5 is mostly over my head ("class ELF" --> Laszlo stops reading,
almost).
I do notice that you import "ceil" from math, for a simple rounded-up
division. I think that's a bad idea (although I'm unsure about Python's
conversions between floating point and integers, and its floats in
general). Such rounding is not hard to do purely with integers; please
leave floating point out of the picture if possible.
In any case, if you have kept the script working for the x86_64 target
(I trust you regression tested it), in patch 5, then I don't object,
generally speaking. I actually welcome the aarch64 addition.
(Drew, can you perhaps check that out? IIRC you worked on the QMP
dump-guest-memory for aarch64.)
So, for patches 1-4, with the nits fixed or not:
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
For patch 5, *if* you remove floating point (--> math / ceil), *and* you
confirm that you regression-tested it for the x86_64 target (which
testing includes looking briefly, with the "crash" utility, at the
extracted kernel vmcore), then you can add my:
Acked-by: Laszlo Ersek <ler...@redhat.com>
Thanks
Laszlo
