Simon Ruderich <si...@ruderich.org> writes: > On Tue, Aug 14, 2018 at 05:49:12PM +0200, Markus Armbruster wrote: >>> On Fri, Aug 10, 2018 at 11:36:51AM +0100, Dr. David Alan Gilbert wrote: >>>>> --- a/hmp-commands.hx >>>>> +++ b/hmp-commands.hx >> >> Subject claims "qmp: add", but the patch also adds to hmp. Recommend to >> split the patch into QMP and HMP part. > > Hello, > > Sure, I can do that. > >>> qapi/misc.json seems to always use 'int' for integer types. Is >>> this value large enough on 64-bit architectures? >> >> Yes. QAPI's int translates to int64_t. > > Thanks. > >>> Just curious, what is the difference between 's' and 'F'. Is that >>> only for documentation purposes (and maybe tab completion) or is >>> the usage different? I noticed existing code uses qdict_get_str() >>> for both 's' and 'F'. >> >> The main behavioral difference is completion. > > Good to know, thanks. > >> I recommend to start with the QMP interface. Parameters are unordered >> there. memsave and pmemsave both take mandatory @val, @size, @filename. >> memsave additionally takes optional @cpu-index. > > Yes. > >> Your pmemload has pmemsave's arguments plus and mandatory @offset. >> Rationale for adding @offset? You may have answered this question >> already; pointer to that answer would be fine. > > My initial patch didn't have the offset. It was suggested by Eric > Blake in <0a960aa8-2a3f-8667-3d46-cecf8e65e...@redhat.com>: > > On Tue, Apr 10, 2018 at 04:33:03PM -0500, Eric Blake wrote: >> Do you additionally need an offset where to start reading from within >> the file (that is, since you already have the 'size' parameter to avoid >> reading the entire file, and the 'val' parameter to target anywhere in >> physical memory, how do I start reading anywhere from the file)? > > It sounded useful to me so I added it.
Feels like an optional parameter to me. >> Once we got the QMP interface nailed down, we can move to the HMP >> interface. > > Good point. > >> These two should become a separate bug fix patch. The bug being fixed >> is completion. > > Sure, they are in separate patches. Just wanted to show the > general changes I applied from the reviews. > > Thanks for the review. > > Regards > Simon
