[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v3 5/5] qmp: add pmemload command
Date: Wed, 15 Aug 2018 06:22:51 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]