[Qemu-devel] Re: [RFC][PATCH v5 07/21] virtagent: add va.getfile RPC

[Adam Litke]

Hi Jes, you raise some good points and pitfalls with the current getfile
approach.  I've been thinking about an alternative and am wondering what
you (and others) think...

First off, I think we should switch to a copyfile() API that allows us
to avoid presenting the file contents to the user.  Neither the human
monitor nor the control monitor are designed to be file pagers.  Let the
user decide how to consume the data once it has been transferred.  Now
we don't need to care if the file is binary or text.

The virtagent RPC protocol is bi-directional and supports asynchronous
events.  We can use these to implement a better copyfile RPC that can
transfer larger files without wasting memory.  The host issues a
copyfile(<guest-path>, <host-path>) RPC.  The immediate result of this
call will indicate whether the guest is able to initiate the transfer.
The guest will generate a series of events (<offset>, <size>, <payload>)
until the entire contents has been transferred.  The host and guest
could negotiate the chunk size if necessary.  Once the transfer is
complete, the guest sends a final event to indicate this (<file-size>,
0).

This interface could be integrated into the monitor with a pair of
commands (va_copyfile and info va_copyfile), the former used to initiate
transfers and the latter to check on the status.

Thoughts on this?

On Tue, 2010-12-07 at 15:18 +0100, Jes Sorensen wrote:
> On 12/03/10 19:03, Michael Roth wrote:
> > Add RPC to retrieve a guest file. This interface is intended
> > for smaller reads like peeking at logs and /proc and such.
> 
> I think you need to redesign your approach here..... see below.
> 
> In 06/21 you had:
> 
> +#define VA_GETFILE_MAX 1 << 30
> 
> > +    while ((ret = read(fd, buf, VA_FILEBUF_LEN)) > 0) {
> > +        file_contents = qemu_realloc(file_contents, count + 
> > VA_FILEBUF_LEN);
> > +        memcpy(file_contents + count, buf, ret);
> 
> UH OH!
> 
> realloc will do a malloc and a memcpy of the data, this is going to turn
> into a really nasty malloc memcpy loop if someone tries to transfer a
> large file using this method. You could end up with almost 4GB of
> parallel allocations for a guest that might have been configured as a
> 1GB guest. This would allow the guest to effectively blow the expected
> memory consumption out of the water. It's not exactly going to be fast
> either :(
> 
> Maybe use a tmp file, and write data out to that as you receive it to
> avoid the malloc ballooning.
> 
> Jes

-- 
Thanks,
Adam