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.
>>
>> 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 <drjo...@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.
Great points; thanks, Drew!
Laszlo
>
> Thanks,
> drew
>
>
>>
>> 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
>>
