Re: [Qemu-devel] [PATCH v3 5/5] qmp: add pmemload command

[Simon Ruderich]

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 <address@hidden>:

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