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

[Eric Blake]

On 04/11/2018 02:36 AM, Simon Ruderich wrote:
> 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()?

Looks like you're right; we don't have an automatic FILE wrapper.

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

You could always add qemu_fopen/qemu_fclose to match the existing
qemu_open/qemu_close.  But you do have a point that you can't call
qemu_close/fclose (because fclose would be left with a stale fd that
might spuriously close something that has been opened in another thread
during the race window), nor fclose/qemu_close.

The FILE interface may sometimes be easier to work with, but it also
comes with buffering issues that you don't have to worry about when
using the fd interface.

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

Good point - but yes, ideally it should always be possible to pass in an
fd instead of having qemu itself open a file, because there are
execution environments where qemu is intentionally prohibited from
directly calling open() for security reasons (where the management app,
such as libvirt, opens and then passes fds to qemu instead).


>>> +##
>>> +# @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.

You are correct that we can't really change the existing interface; so
documenting in the commit message that your choice of names is for
consistency reasons may be sufficient.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature