From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:60730) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QCuMj-0007Jy-Ge for qemu-devel@nongnu.org; Thu, 21 Apr 2011 09:56:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QCuMi-0007cs-22 for qemu-devel@nongnu.org; Thu, 21 Apr 2011 09:56:17 -0400 Received: from e33.co.us.ibm.com ([32.97.110.151]:37395) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QCuMh-0007cj-TA for qemu-devel@nongnu.org; Thu, 21 Apr 2011 09:56:16 -0400 Received: from d03relay04.boulder.ibm.com (d03relay04.boulder.ibm.com [9.17.195.106]) by e33.co.us.ibm.com (8.14.4/8.13.1) with ESMTP id p3LDnIx5013874 for ; Thu, 21 Apr 2011 07:49:18 -0600 Received: from d03av06.boulder.ibm.com (d03av06.boulder.ibm.com [9.17.195.245]) by d03relay04.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id A01FBIqw054808 for ; Thu, 1 Jan 1970 08:11:19 -0700 Received: from d03av06.boulder.ibm.com (loopback [127.0.0.1]) by d03av06.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p3LE0wrk004653 for ; Thu, 21 Apr 2011 08:00:58 -0600 Message-ID: <4DB0376B.8010505@linux.vnet.ibm.com> Date: Thu, 21 Apr 2011 08:55:55 -0500 From: Michael Roth MIME-Version: 1.0 References: <1303138953-1334-1-git-send-email-mdroth@linux.vnet.ibm.com> <4DAFFD05.20304@redhat.com> In-Reply-To: <4DAFFD05.20304@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC][PATCH v2 00/11] QEMU Guest Agent: QMP-based host/guest communication (virtagent) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jes Sorensen Cc: aliguori@linux.vnet.ibm.com, agl@linux.vnet.ibm.com, qemu-devel@nongnu.org On 04/21/2011 04:46 AM, Jes Sorensen wrote: > On 04/18/11 17:02, Michael Roth wrote: >> These apply on top of Anthony's glib tree, commit 03d5927deb5e6baebaade1b4c8ff2428a85e125c currently, and can also be obtained from: >> git://repo.or.cz/qemu/mdroth.git qga_v2 >> >> Patches 1-8 are general json/QAPI-related fixes. Anthony, please consider pulling these into your glib tree. The json fix-ups may need further evaluation, but I'm confident they're at least an improvement. The QAPI ones are mostly trivial fix-ups. >> >> Changes since V1: >> >> - Added guest agent worker thread to execute RPCs in the guest. With this in place we have a reliable timeout mechanism for hung commands, currently set at 30 seconds. >> - Add framework for registering init/cleanup routines for stateful RPCs to clean up after themselves after a timeout. >> - Added the following RPCs: guest-file-{open,close,read,write,seek}, guest-shutdown, guest-info, and removed stubs for guest-view-file (now deprecated) >> - Added GUEST_AGENT_UP/GUEST_AGENT_DOWN QMP events >> - Switched to a TCP-style host-initiated 3-way handshake for channel negotiation, this simplifies client negotiation/interaction over the wire >> - Added configurable log level/log file/pid file options for guest agent >> - Various fixes for bugs/memory leaks and checkpatch.pl fixups >> >> ISSUES/TODOS: >> >> - Fix QMP proxy handling of error responses sent by guest agent, currently ignored >> - Add unit tests for guest agent wire protocol >> - Add unit tests for QMP interfaces >> - Add host-side timeout mechanism for async QMP commands >> - Return error for guest commands if guest up event has not yet been recieved >> - Make QMP param names more consistent between related commands >> - Clean up logging >> > > Hi, > > I had a look through this patchset and generally it looks pretty good. > There were a few nits, and I ignored all the python gibberish to avoid > getting a headache. > > Did you do anything with the fsfreeze patches, or were they dropped in > the migration to qapi? They were pending some changes required on the agent side that weren't really addressed/doable until this patchset, namely: 1) server-side timeout mechanism to recover from RPCs that can hang indefinitely or take a really long time (fsfreeze, fopen, etc), currently it's 30 seconds, may need to bump it to 60 for fsfreeze, or potentially add an RPC to change the server-side timeout 2) a simple way to temporarily turn off logging so agent doesn't deadlock itself 3) a way to register a cleanup handler when a timeout occurs. 4) disabling RPCs where proper accounting/logging is required (guest-open-file, guest-shutdown, etc) #4 isn't implemented...I think this could be done fairly in-evasively with something like: Response important_rpc(): if (!ga_log("syslog", LEVEL_CRITICAL, "important stuff happening")) return ERROR_LOGGING_CURRENTLY_DISABLED ... bool ga_log(log_domain, level, msg): if (log_domain == "syslog") if (!logging_enabled && is_critical(log_level)) return False; syslog(msg, ...) else if (logging_enabled) normallog(msg, ...) return True With that I think we could actually drop the fsfreeze stuff in. Thoughts? > > Cheers, > Jes >