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.

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.

Many thanks to Eric Blake for review the patch.

---

+++ b/qapi-schema.json

qapi-schema.json +5 -0

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

cpus.c exit: · line 1486

Quoted 9 lines

cpus.c +9 -0

+    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

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.

Review

Quoted 2 lines

(quoted diff) +2 -0

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

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

Quoted 1 line

I found this could be useful to have qemu-softmmu as a cross debugger

(launch

Quoted 1 line

with -s -S command line option), then if we can have a command to load

guest

Quoted 1 line

physical memory, we can use cross gdb to do some target debug which gdb

cannot

Quoted 2 lines

do directly.

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

Quoted 1 line

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

Quoted 2 lines

---

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

cpus.c exit: · line 1477

Quoted 9 lines

cpus.c +8 -0

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

cpus.c exit: · line 1486

Quoted 9 lines

cpus.c +9 -0

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

Review

Quoted 2 lines

(quoted diff) +2 -0

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

Quoted 10 lines

+++ b/qapi-schema.json

qapi-schema.json +5 -0

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

qapi-schema.json line 1725

Quoted 10 lines

qapi-schema.json +10 -0

+#
+# @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

Signed-off-by Baojun Wang <[email protected]>

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.

Many thanks to Eric Blake for review the patch and suggestions.

Quoted 5 lines · Baojun Wang

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.

Quoted 1 line

Many thanks to Eric Blake for review the patch and suggestions.

Okay, that looks sane, but can you explain please, why did you address qemu-trivial@? The change definitely is not "trivial", as it introduces a new qmp command which becomes a part of public interface. Even if the implementation may look trivial, the interface change should not be done using -trivial tree.

Quoted 1 line · Baojun Wang

I found this could be useful to have qemu-softmmu as a cross debugger

(launch

Quoted 1 line

with -s -S command line option), then if we can have a command to load

guest

Quoted 1 line

physical memory, we can use cross gdb to do some target debug which gdb

cannot

Quoted 3 lines

do directly.

Many thanks to Eric Blake for review the patch and suggestions.

Okay, that looks sane, but can you explain please, why did you address qemu-trivial@? The change definitely is not "trivial", as it introduces a new qmp command which becomes a part of public interface. Even if the implementation may look trivial, the interface change should not be done using -trivial tree.

Thanks,

/mjt