From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=40958 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PgKfJ-0004H5-Cn for qemu-devel@nongnu.org; Fri, 21 Jan 2011 12:20:51 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PgKfA-0003bc-8m for qemu-devel@nongnu.org; Fri, 21 Jan 2011 12:20:41 -0500 Received: from mx1.redhat.com ([209.132.183.28]:49785) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PgKfA-0003b7-0T for qemu-devel@nongnu.org; Fri, 21 Jan 2011 12:20:40 -0500 Date: Fri, 21 Jan 2011 17:20:26 +0000 From: "Daniel P. Berrange" Subject: Re: [Qemu-devel] Re: [RFC][PATCH v6 08/23] virtagent: add va.getfile RPC Message-ID: <20110121172026.GK12589@redhat.com> References: <1295270117-24760-1-git-send-email-mdroth@linux.vnet.ibm.com> <1295270117-24760-9-git-send-email-mdroth@linux.vnet.ibm.com> <4D39B716.1010400@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <4D39B716.1010400@redhat.com> Reply-To: "Daniel P. Berrange" List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jes Sorensen Cc: agl@linux.vnet.ibm.com, stefanha@linux.vnet.ibm.com, abeekhof@redhat.com, marcel.mittelstaedt@de.ibm.com, qemu-devel@nongnu.org, Michael Roth , aliguori@linux.vnet.ibm.com, ryanh@us.ibm.com, markus_mueller@de.ibm.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 > > --- > > 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