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. > 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 -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
signature.asc
Description: PGP signature
