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
signature.asc
Description: OpenPGP digital signature