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

[Andrew Jones <drjones@redhat.com>]

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.
> 
> 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>


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.

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
> Laszlo
>