From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38103) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aJkhJ-0002bo-AQ for qemu-devel@nongnu.org; Thu, 14 Jan 2016 11:24:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aJkhG-0003tP-3Z for qemu-devel@nongnu.org; Thu, 14 Jan 2016 11:24:29 -0500 Received: from mx1.redhat.com ([209.132.183.28]:57763) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aJkhF-0003tK-To for qemu-devel@nongnu.org; Thu, 14 Jan 2016 11:24:26 -0500 References: <1452761307-57200-1-git-send-email-frankja@linux.vnet.ibm.com> From: Laszlo Ersek Message-ID: <5697CBB7.4070805@redhat.com> Date: Thu, 14 Jan 2016 17:24:23 +0100 MIME-Version: 1.0 In-Reply-To: <1452761307-57200-1-git-send-email-frankja@linux.vnet.ibm.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC 0/5] scripts/dump-guest-memory.py: Add multi-arch support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Janosch Frank , qemu-devel@nongnu.org Cc: pbonzini@redhat.com, Drew Jones 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 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 Thanks Laszlo