Re: [Qemu-devel] [RFC 0/5] scripts/dump-guest-memory.py: Add multi-arch support

[Laszlo Ersek]

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 <address@hidden>

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 <address@hidden>

Thanks
Laszlo