Re: [Qemu-trivial] [PATCH V3 1/1] Add pmemsave command for both monitor and qmp, which could be useful to have qemu-softmmu as a cross debugger to load guest physical memory.

[Baojun Wang]

(Re-send because missing reply-to-all)

Really appreciate your time and valuable input Eric. I almost didn't submit any patches, so there is quite a lot to learn about it.

I'd agree with you on this, but memsave/pmemsave function are using stdio, thus I was trying to be consistent with them.
 

> +    if (!f) {
> +        error_setg_file_open(errp, errno, filename);
> +        return;
> +    }
> +
> +    while (size != 0) {
> +        l = fread(buf, 1, sizeof(buf), f);
> +        if (l > size)
> +            l = size;

How about error handling as below, the same error handling is used by memsave/pmemsave.

        l = fread(buf, 1, sizeof(buf), f);

        if (l ! = sizeof(buf)) {
            error_set(errp, QERR_IO_ERROR);
            goto eixt;
        }

 

This is lousy at error detection.  Again, you should be using read(),
not fread(), and properly erroring out on read failures rather than
silently ignoring them.

> +        .name       = "pmemload",
> +        .args_type  = "val:l,size:i,filename:s",

Would it make sense to put filename as the FIRST argument, with val and
size as optional arguments (defaulting to reading in at address 0 and
for the size determined by the length of the input file), rather than
forcing the user to pass in a size up front?

Same reason as above, I though it could be easier if pmemload takes the same arguments as memsave/pmemsave.

Again, really thanks for your patient.

Best Regards

On Tue, Apr 8, 2014 at 12:42 PM, Eric Blake <address@hidden> wrote:

On 04/08/2014 01:30 PM, Baojun Wang wrote:

Your subject line is extremely long.  In general, we strive for
something less than 60 characters, so that 'git shortlog -30' can
display the entire line in an 80-column terminal.  Also, the subject
mentions pmemsave, but your commit mentions pmemload - it's very
misleading to provide a patch where the commit message doesn't even
mention the name of the command the patch is adding.  I would suggest a
subject line of:

qmp: add pmemload command

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

Your patch is lacking a Signed-off-by designation, therefore we cannot
accept it yet.

> Many thanks to Eric Blake for review the patch.

This comment may be useful to reviewers, but is not part of the commit
itself, so it belongs better...

>
> ---

...here, after the --- separator.

>
> +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");

Rather than using fopen() and FILE* operations, I'd prefer you use
qemu_open() and lower-level read() operations.  In particular, this will
automatically make it possible for management applications to pass in
'/dev/fdset/1' to reuse a file descriptor that they passed in to qemu,
rather than forcing qemu to directly open the file.

> +    if (!f) {
> +        error_setg_file_open(errp, errno, filename);
> +        return;
> +    }
> +
> +    while (size != 0) {
> +        l = fread(buf, 1, sizeof(buf), f);
> +        if (l > size)
> +            l = size;

This is lousy at error detection.  Again, you should be using read(),
not fread(), and properly erroring out on read failures rather than
silently ignoring them.

> +        .name       = "pmemload",
> +        .args_type  = "val:l,size:i,filename:s",

Would it make sense to put filename as the FIRST argument, with val and
size as optional arguments (defaulting to reading in at address 0 and
for the size determined by the length of the input file), rather than
forcing the user to pass in a size up front?

> +++ b/qapi-schema.json
> @@ -1708,6 +1708,26 @@
>    'data': {'val': 'int', 'size': 'int', 'filename': 'str'} }
>
>  ##
> +# @pmemload:
> +#
> +# Load a portion of guest physical memory from a file.
> +#
> +# @val: the physical address of the guest to start from
> +#
> +# @size: the size of memory region to load
> +#
> +# @filename: the file to load the memory from as binary data
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 2.1
> +#
> +# Notes: Errors were not reliably returned until 2.1

Delete this line.  I guess I didn't make it clear in my first review
that this line is bogus copy and paste.  You don't need it.  pmemsave
needed it, because pmemsave went through some iterations where earlier
versions were buggy.  But your pmemload command is brand new, and should
not have bugs worth worrying about.

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