Re: [Qemu-devel] Re: [RFC][PATCH v6 08/23] virtagent: add va.getfile RPC

[Michael Roth]

On 01/21/2011 11:20 AM, Daniel P. Berrange wrote:
> On Fri, Jan 21, 2011 at 05:40:54PM +0100, Jes Sorensen wrote:
> > On 01/17/11 14:15, 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.
> > >
> > > Signed-off-by: Michael Roth<address@hidden>
> > > ---
> > >   virtagent-server.c |   59 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >   1 files changed, 59 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/virtagent-server.c b/virtagent-server.c
> > > index c38a9e0..af4b940 100644
> > > --- a/virtagent-server.c
> > > +++ b/virtagent-server.c
> > > @@ -62,12 +62,71 @@ out:
> > >       return ret;
> > >   }
> > >
> > > +/* RPC functions common to guest/host daemons */
> > > +
> > > +/* va_getfile(): return file contents
> > > + * rpc return values:
> > > + *   - base64-encoded file contents
> > > + */
> > > +static xmlrpc_value *va_getfile(xmlrpc_env *env,
> > > +                                xmlrpc_value *params,
> > > +                                void *user_data)
> > > +{
> > > +    const char *path;
> > > +    char *file_contents = NULL;
> > > +    char buf[VA_FILEBUF_LEN];
> >
> > malloc()!
> >
> > > +    int fd, ret, count = 0;
> > > +    xmlrpc_value *result = NULL;
> > > +
> > > +    /* parse argument array */
> > > +    xmlrpc_decompose_value(env, params, "(s)",&path);
> > > +    if (env->fault_occurred) {
> > > +        return NULL;
> > > +    }
> > > +
> > > +    SLOG("va_getfile(), path:%s", path);
> > > +
> > > +    fd = open(path, O_RDONLY);
> > > +    if (fd == -1) {
> > > +        LOG("open failed: %s", strerror(errno));
> > > +        xmlrpc_faultf(env, "open failed: %s", strerror(errno));
> > > +        return NULL;
> > > +    }
> > > +
> > > +    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);
> >
> > Sorry, I brought this up before. This realloc() stuff is a disaster
> > waiting to happen. Please remove it from the patch series, until you
> > have an implementation that copies over a page of the time.
>
> I can understand the need of virtagent for lifecycle control/interactions
> with the guest OS (reboot, shutdown, ping, screen lock/unlock, etc), but
> do we really want to reinvent libguestfs for file access ? A little dev
> work could enable users to install the libguestfs agent into a guest OS,
> and access it from the host over virtio-serial + the libguestfs API.

File/dmesg/etc access is a bit of a grey area. Technically it's not 
lifecycle-specific, but it tends to become a requirement for 
higher-level management policies, and being reliant on external tools to 
provide what, at least in our case, has been an extremely common 
request/requirement, greatly reduces the usefulness of such an agent.

Ultimately however these interfaces would be exposed via libvirt, which 
libguestfs already makes use of, so it'd be a logically way to extend it 
for disk access to live guests.

getfile() is confusingly named however, it's really just a means to peek 
at a text file like /proc/meminfo. general file access will be done via 
a stateful interface that implements similar semantics to 
open()/read()/write()/close().

> This would be quite compelling usage model for app developers, because
> it would mean whether the guest OS was running, or shutoff, they can
> use the same libguestfs API for processing guest filesystem images.
> The level of functionality provided by libguestfs is really quite
> considerable now, letting you do pretty much any operation against
> files that you could do via local POSIX for non-virt access, as
> well as providing many useful higher level constructs
>
> Regards,
> Daniel