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

["Daniel P. Berrange" <berrange@redhat.com>]

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 <mdroth@linux.vnet.ibm.com>
> > ---
> >  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.

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