From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46797) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xb4Xy-0005pL-Ou for qemu-devel@nongnu.org; Mon, 06 Oct 2014 05:25:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xb4Xs-0005A2-Je for qemu-devel@nongnu.org; Mon, 06 Oct 2014 05:25:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55658) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xb4Xs-00059P-Cf for qemu-devel@nongnu.org; Mon, 06 Oct 2014 05:25:32 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s969PS4C022713 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Mon, 6 Oct 2014 05:25:28 -0400 Message-ID: <54326004.8000009@redhat.com> Date: Mon, 06 Oct 2014 11:25:24 +0200 From: Laszlo Ersek MIME-Version: 1.0 References: <1412581491-31656-1-git-send-email-imammedo@redhat.com> <1412581491-31656-2-git-send-email-imammedo@redhat.com> In-Reply-To: <1412581491-31656-2-git-send-email-imammedo@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] guest-agent: keep persistent state on persistent storage List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov , qemu-devel@nongnu.org On 10/06/14 09:44, Igor Mammedov wrote: > GA was keepeing persistent state info in /var/run/qga.state > file. However it's lost after every reboot since /var/run > usually is located on tmpfs or cleaned on start-up. > > Fix issue by keeping state file in /var/lib/qemu-ga/ > directory, which is intended for keeping persistent > local state according to FHS. > > Signed-off-by: Igor Mammedov > --- > qga/main.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/qga/main.c b/qga/main.c > index 227f2bd..5afba01 100644 > --- a/qga/main.c > +++ b/qga/main.c > @@ -45,7 +45,8 @@ > > #ifndef _WIN32 > #define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent.0" > -#define QGA_STATE_RELATIVE_DIR "run" > +#define QGA_VOLATILE_STATE_RELATIVE_DIR "run" > +#define QGA_STATE_RELATIVE_DIR "lib/qemu-ga" > #define QGA_SERIAL_PATH_DEFAULT "/dev/ttyS0" > #else > #define QGA_VIRTIO_PATH_DEFAULT "\\\\.\\Global\\org.qemu.guest_agent.0" > @@ -121,7 +122,7 @@ init_dfl_pathnames(void) > dfl_pathnames.state_dir = qemu_get_local_state_pathname( > QGA_STATE_RELATIVE_DIR); > dfl_pathnames.pidfile = qemu_get_local_state_pathname( > - QGA_STATE_RELATIVE_DIR G_DIR_SEPARATOR_S "qemu-ga.pid"); > + QGA_VOLATILE_STATE_RELATIVE_DIR G_DIR_SEPARATOR_S "qemu-ga.pid"); > } > > static void quit_handler(int sig) > I think this patch is right, but perhaps another hunk should be added. According to commit 39097daf, persistent state should indeed survive guest reboots. This seems appropriate for at least "fd_counter". However we store "qga.state.isfrozen" too in the state directory ("state_filepath_isfrozen"). According to commit f789aa7b, that file should survive qemu-ga crashes and restarts, but I think it shouldn't survive entire *guest* restarts. (When the guest reboots, its filesystems certainly won't be frozen, and the "qga.state.isfrozen" will be stale.) What we have now is: - state (including fd_counter and isfrozen status) is lost across guest reboot. This is wrong for fd_counter, but right for isfrozen. If you make the state persistent for real, then: - fd_counter will work more safely, but isfrozen will (theoretically) regress. Hence I think that "state_filepath_isfrozen" should also use QGA_VOLATILE_STATE_RELATIVE_DIR. And that's a huge mess then, because: - in Windows guests, we only have one state dir, and the "state_filepath_isfrozen" assignment is currently guest-type independent - in Linux guests, we'd have two state dirs (non-volatile and volatile), but the user can control only one with the -t option. I don't know what to recommend for this. Anyway I have some worries about the 2nd patch, let me continue there. Thanks Laszlo