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

[Markus Armbruster]

Simon Ruderich <address@hidden> 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 <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.

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