Re: [Qemu-trivial] [PATCH V4 1/1] qmp: add pmemload command

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1714 bytes --]

On 04/09/2014 10:54 AM, Baojun Wang wrote:
> Signed-off-by: Baojun Wang <wangbj@gmail.com>

You lost this part of your commit message, which gives more details
about the 'why' (the subject line covers the 'what', but it is often the
'why' that is most needed when reviewing a commit later).  Your earlier
mail had:

I found this could be useful to have qemu-softmmu as a cross debugger
(launch
with -s -S command line option), then if we can have a command to load guest
physical memory, we can use cross gdb to do some target debug which gdb
cannot
do directly.


>  
> +void qmp_pmemload(int64_t addr, int64_t size, const char *filename,
> +                  Error **errp)
> +{
> +    FILE *f;
> +    uint32_t l;
> +    uint8_t buf[1024];
> +
> +    f = fopen(filename, "rb");

I still think that fopen()/fread() is wrong, and that you should be
using qemu_open()/read() - that way, libvirt could pass in a file
descriptor and use qemu's already-existing /dev/fdset/nnn support rather
than forcing qemu to open something from the filesystem.


> +++ b/hmp-commands.hx
> @@ -809,6 +809,19 @@ save to disk physical memory dump starting at @var{addr} of size @var{size}.
>  ETEXI
>  
>      {
> +        .name       = "pmemload",
> +        .args_type  = "val:l,size:i,filename:s",
> +        .params     = "addr size file",
> +        .help       = "load from disk physical memory dump starting at 'addr' of size 'size'",
> +        .mhandler.cmd = hmp_pmemload,

And I still think that you should list filename first, with val and size
optional at the end.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]