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

[Janosch Frank <frankja@linux.vnet.ibm.com>]

On 01/18/2016 06:57 PM, Laszlo Ersek wrote:
> On 01/18/16 17:31, Andrew Jones wrote:
>> On Thu, Jan 14, 2016 at 05:24:23PM +0100, Laszlo Ersek wrote:
>>> 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.

Leaving floating point out in python is difficult, read pep 238.
https://www.python.org/dev/peps/pep-0238/

In python 3:
1/2 == 0.5
1//2 == 0
but a // b == floor(a/b), i.e. a cast is made.

Anyway, I got rid of the import with:
-(-len_desc // 4)

>>>
>>> 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.)
>>
>> I gave this a test run on AArch64 (LE). It worked, thus
>>
>> Tested-by: Andrew Jones <drjones@redhat.com>

Thanks for testing, I'm currently setting up a Intel system to test
X86_64. Unfortunately I didn't have the system at hand before sending
the RFC.

>>
>>
>> But the help text needs help. I'll paste the ones I think need changes
>> here in order to point out my suggestions
>>
>>>  raise gdb.GdbError("No valid arch type specified.\n"
>>>                     "Currently supported types:"
>>>                     "aarch64 be/le, X86_64, 386, s390, ppc64 be/le")
>>                               ^ missing '-'                   ^ missing '-'
>>
>> Actually it might be better to spell out aarch64-be, aarch64-le and
>> ppc64-be, ppc64-le as well.
>>
>>>  class DumpGuestMemory(gdb.Command):
>>>      """Extract guest vmcore from qemu process coredump.
>>>
>>>  The sole argument is FILE, identifying the target file to write the
>>
>> The two required arguments are FILE and ARCH. FILE identifies... ARCH
>> selects the architecture for which the core will be generated.
>>
>>>  guest vmcore to.
>>>
>>>  This GDB command reimplements the dump-guest-memory QMP command in
>>>  python, using the representation of guest memory as captured in the qemu
>>>  coredump. The qemu process that has been dumped must have had the
>>>  command line option "-machine dump-guest-core=on".
>>
>> Add one more sentence: "By default dump-guest-core is on."
>>
>>>  
>>>  For simplicity, the "paging", "begin" and "end" parameters of the QMP
>>>  command are not supported -- no attempt is made to get the guest's
>>>  internal paging structures (ie. paging=false is hard-wired), and guest
>>>  memory is always fully dumped.
>>>  
>>>  Only x86_64 guests are supported.
>>
>> aarch64-be, aarch64-le, X86_64, 386, s390, ppc64-be, ppc64-le guests are
>> supported.
>>
>>>  
>>>  The CORE/NT_PRSTATUS and QEMU notes (that is, the VCPUs' statuses) are
>>>  not written to the vmcore. Preparing these would require context that is
>>>  only present in the KVM host kernel module when the guest is alive. A
>>>  fake ELF note is written instead, only to keep the ELF parser of "crash"
>>>  happy.
>>>  
>>>  Dependent on how busted the qemu process was at the time of the
>>>  coredump, this command might produce unpredictable results. If qemu
>>>  deliberately called abort(), or it was dumped in response to a signal at
>>>  a halfway fortunate point, then its coredump should be in reasonable
>>>  shape and this command should mostly work."""
>>
>>
>> Additionally, as this was a pretty full rewrite of the script, then I
>> think it warrants an additional Authors line under Laszlo's name.
> 
> Great points; thanks, Drew!
> Laszlo
> 

All mentioned changes will land in the patch series, thanks for
reviewing/testing to both of you.

>>
>> Thanks,
>> drew
>>
>>
>>>
>>> So, for patches 1-4, with the nits fixed or not:
>>>
>>> Reviewed-by: Laszlo Ersek <lersek@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 <lersek@redhat.com>

Thanks

>>>
>>> Thanks
>>> Laszlo
>>>
>