Re: [Qemu-devel] [PATCH 1/2] guest-agent: keep persistent state on persistent storage

[Laszlo Ersek]

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 <address@hidden>
> ---
>  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