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

[Simon Ruderich]

On Tue, Apr 10, 2018 at 04:33:03PM -0500, Eric Blake wrote:
>> +void qmp_pmemload(int64_t addr, int64_t size, const char *filename,
>> +                  Error **errp)
>> +{
>> +    FILE *f;
>> +    size_t l;
>> +    uint8_t buf[1024];
>> +
>> +    f = fopen(filename, "rb");
>
> Use qemu_fopen() here, so that you can automagically support /dev/fdset/
> magic filenames that work on file descriptors passed via SCM_RIGHTS.

Hello,

I can't find qemu_fopen() in the source. Did you mean
qemu_open()? From reading qemu_close() I guess that I can't use
fdopen() (as there's no qemu_fclose()) but must work with the raw
fd. Or is there an easy way to fdopen? (I prefer the FILE *
interface which is easier to work with.)

But I just copied the code from qmp_pmemsave. Should I change it
as well to use qemu_open()?

>> +++ b/qapi-schema.json
>> @@ -1136,6 +1136,24 @@
>>  { 'command': 'pmemsave',
>>    'data': {'val': 'int', 'size': 'int', 'filename': 'str'} }
>>
>> +##
>> +# @pmemload:
>> +#
>> +# Load a portion of guest physical memory from a file.
>> +#
>> +# @val: the physical address of the guest to start from
>
> Is 'val' really the best name for this, or would 'phys-addr' or similar
> be a more descriptive name?

I copied it from pmemsave to keep the argument names consistent.
Should I change it only for pmemload? Changing it for pmemsave is
problematic as it will break the existing API.

>> +#
>> +# @size: the size of memory region to load
>> +#
>> +# @filename: the file to load the memory from as binary data
>> +#
>> +# Returns: Nothing on success
>> +#
>> +# Since: 2.10
>
> You've missed 2.10 by a long shot.  The earliest this new feature could
> appear is 2.13.

Will change.

> 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)?

I didn't need it for my usage and wanted to keep the patch as
simple as possible. But I see how it can be useful and will add
it to my patch in the next iteration.

Thank you for the review!

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9