[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC 0/5] scripts/dump-guest-memory.py: Add multi-arch
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [RFC 0/5] scripts/dump-guest-memory.py: Add multi-arch support |
Date: |
Thu, 14 Jan 2016 17:24:23 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 |
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
- [Qemu-devel] [RFC 4/5] scripts/dump-guest-memory.py: Cleanup functions, (continued)
- [Qemu-devel] [RFC 4/5] scripts/dump-guest-memory.py: Cleanup functions, Janosch Frank, 2016/01/14
- [Qemu-devel] [RFC 3/5] scripts/dump-guest-memory.py: Improve python 3 compatibility, Janosch Frank, 2016/01/14
- [Qemu-devel] [RFC 2/5] scripts/dump-guest-memory.py: Make methods functions, Janosch Frank, 2016/01/14
- [Qemu-devel] [RFC 5/5] scripts/dump-guest-memory.py: Introduce multi-arch support, Janosch Frank, 2016/01/14
- [Qemu-devel] [RFC 1/5] scripts/dump-guest-memory.py: Move constants to the top, Janosch Frank, 2016/01/14
- Re: [Qemu-devel] [RFC 0/5] scripts/dump-guest-memory.py: Add multi-arch support,
Laszlo Ersek <=
- Re: [Qemu-devel] [RFC 0/5] scripts/dump-guest-memory.py: Add multi-arch support, Andrew Jones, 2016/01/18
- Re: [Qemu-devel] [RFC 0/5] scripts/dump-guest-memory.py: Add multi-arch support, Laszlo Ersek, 2016/01/18
- Re: [Qemu-devel] [RFC 0/5] scripts/dump-guest-memory.py: Add multi-arch support, Janosch Frank, 2016/01/20
- Re: [Qemu-devel] [RFC 0/5] scripts/dump-guest-memory.py: Add multi-arch support, Paolo Bonzini, 2016/01/20
- Re: [Qemu-devel] [RFC 0/5] scripts/dump-guest-memory.py: Add multi-arch support, Markus Armbruster, 2016/01/20
- Re: [Qemu-devel] [RFC 0/5] scripts/dump-guest-memory.py: Add multi-arch support, Laszlo Ersek, 2016/01/20