|
From: | Baojun Wang |
Subject: | Re: [Qemu-devel] [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. |
Date: | Tue, 8 Apr 2014 16:29:19 -0700 |
> + 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.
Would it make sense to put filename as the FIRST argument, with val and
> + .name = "pmemload",
> + .args_type = "val:l,size:i,filename:s",
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?
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
Your patch is lacking a Signed-off-by designation, therefore we cannot
> 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.
>
accept it yet.
This comment may be useful to reviewers, but is not part of the commit
> Many thanks to Eric Blake for review the patch.
itself, so it belongs better...
>
> ---
...here, after the --- separator.
Rather than using fopen() and FILE* operations, I'd prefer you use
>
> +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");
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.
This is lousy at error detection. Again, you should be using read(),
> + 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;
not fread(), and properly erroring out on read failures rather than
silently ignoring them.
Would it make sense to put filename as the FIRST argument, with val and
> + .name = "pmemload",
> + .args_type = "val:l,size:i,filename:s",
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?
Delete this line. I guess I didn't make it clear in my first review
> +++ 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
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
[Prev in Thread] | Current Thread | [Next in Thread] |