[Qemu-devel] [PATCH 0/6] local state directory fixes for win32 qga

[Qemu-devel] [PATCH 0/6] local state directory fixes for win32 qga

[Laszlo Ersek]

Qouting patch 2/6:

> Since commit 39097daf ("qemu-ga: use key-value store to avoid
> recycling fd handles after restart") we've relied on the state
> directory for the fd handles' key-value store. Even though we don't
> support the guest-file-* commands on win32 yet, the key-value store is
> written, and it's the first use of the state directory on win32. We
> should have a sensible default for its location.

Motivated by RHBZ#962669.

I've perpetrated this in the second half of a Friday->Sunday
all-nighter, so be gentle. Thanks.

Laszlo Ersek (6):
  osdep: add qemu_get_local_state_pathname()
  qga: determine default state dir and pidfile dynamically
  configure: don't save any fixed local_statedir for win32
  qga: create state directory on win32
  qga: remove undefined behavior in ga_install_service()
  qga: save state directory in ga_install_service()

 configure            |   12 +++++++---
 include/qemu/osdep.h |   11 +++++++++
 qga/service-win32.h  |    3 +-
 qga/main.c           |   57 +++++++++++++++++++++++++++++++++++++++++++------
 qga/service-win32.c  |   25 ++++++++++++++--------
 util/oslib-posix.c   |    9 ++++++++
 util/oslib-win32.c   |   22 +++++++++++++++++++
 7 files changed, 118 insertions(+), 21 deletions(-)

[Qemu-devel] [PATCH 1/6] osdep: add qemu_get_local_state_pathname()

[Laszlo Ersek]

This function returns ${prefix}/var/RELATIVE_PATHNAME on POSIX-y systems,
and <CSIDL_COMMON_APPDATA>/RELATIVE_PATHNAME on Win32.

http://msdn.microsoft.com/en-us/library/bb762494.aspx

  [...] This folder is used for application data that is not user
  specific. For example, an application can store a spell-check
  dictionary, a database of clip art, or a log file in the
  CSIDL_COMMON_APPDATA folder. [...]

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 include/qemu/osdep.h |   11 +++++++++++
 util/oslib-posix.c   |    9 +++++++++
 util/oslib-win32.c   |   22 ++++++++++++++++++++++
 3 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 57d7b1f..26136f1 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -204,4 +204,15 @@ const char *qemu_get_version(void);
 void fips_set_state(bool requested);
 bool fips_get_state(void);
 
+/* Return a dynamically allocated pathname denoting a file or directory that is
+ * appropriate for storing local state.
+ *
+ * @relative_pathname need not start with a directory separator; one will be
+ * added automatically.
+ *
+ * The caller is responsible for releasing the value returned with g_free()
+ * after use.
+ */
+char *qemu_get_local_state_pathname(const char *relative_pathname);
+
 #endif
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 631a1de..3dc8b1b 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -47,6 +47,8 @@ extern int daemon(int, int);
 #  define QEMU_VMALLOC_ALIGN getpagesize()
 #endif
 
+#include <glib/gprintf.h>
+
 #include "config-host.h"
 #include "sysemu/sysemu.h"
 #include "trace.h"
@@ -232,3 +234,10 @@ int qemu_utimens(const char *path, const struct timespec *times)
 
     return utimes(path, &tv[0]);
 }
+
+char *
+qemu_get_local_state_pathname(const char *relative_pathname)
+{
+    return g_strdup_printf("%s/%s", CONFIG_QEMU_LOCALSTATEDIR,
+                           relative_pathname);
+}
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index df2ecbd..961fbf5 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -26,12 +26,17 @@
  * THE SOFTWARE.
  */
 #include <windows.h>
+#include <glib.h>
+#include <stdlib.h>
 #include "config-host.h"
 #include "sysemu/sysemu.h"
 #include "qemu/main-loop.h"
 #include "trace.h"
 #include "qemu/sockets.h"
 
+/* this must come after including "trace.h" */
+#include <shlobj.h>
+
 void *qemu_oom_check(void *ptr)
 {
     if (ptr == NULL) {
@@ -160,3 +165,20 @@ int qemu_get_thread_id(void)
 {
     return GetCurrentThreadId();
 }
+
+char *
+qemu_get_local_state_pathname(const char *relative_pathname)
+{
+    HRESULT result;
+    char base_path[MAX_PATH+1] = "";
+
+    result = SHGetFolderPath(NULL, CSIDL_COMMON_APPDATA, NULL,
+                             /* SHGFP_TYPE_CURRENT */ 0, base_path);
+    if (result != S_OK) {
+        /* misconfigured environment */
+        g_critical("CSIDL_COMMON_APPDATA unavailable: %ld", (long)result);
+        abort();
+    }
+    return g_strdup_printf("%s" G_DIR_SEPARATOR_S "%s", base_path,
+                           relative_pathname);
+}
-- 
1.7.1

[Qemu-devel] [PATCH 2/6] qga: determine default state dir and pidfile dynamically

[Laszlo Ersek]

No effective change on POSIX, but on Win32 the defaults come from the
environment / session.

Since commit 39097daf ("qemu-ga: use key-value store to avoid recycling fd
handles after restart") we've relied on the state directory for the fd
handles' key-value store. Even though we don't support the guest-file-*
commands on win32 yet, the key-value store is written, and it's the first
use of the state directory on win32. We should have a sensible default for
its location.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 qga/main.c |   32 ++++++++++++++++++++++++++------
 1 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index 44a2836..f5f033d 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -45,16 +45,21 @@
 
 #ifndef _WIN32
 #define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent.0"
+#define QGA_STATE_RELATIVE_DIR  "run"
 #else
 #define QGA_VIRTIO_PATH_DEFAULT "\\\\.\\Global\\org.qemu.guest_agent.0"
+#define QGA_STATE_RELATIVE_DIR  "qemu-ga"
 #endif
-#define QGA_STATEDIR_DEFAULT CONFIG_QEMU_LOCALSTATEDIR "/run"
-#define QGA_PIDFILE_DEFAULT QGA_STATEDIR_DEFAULT "/qemu-ga.pid"
 #ifdef CONFIG_FSFREEZE
 #define QGA_FSFREEZE_HOOK_DEFAULT CONFIG_QEMU_CONFDIR "/fsfreeze-hook"
 #endif
 #define QGA_SENTINEL_BYTE 0xFF
 
+static struct {
+    const char *state_dir;
+    const char *pidfile;
+} dfl_pathnames;
+
 typedef struct GAPersistentState {
 #define QGA_PSTATE_DEFAULT_FD_COUNTER 1000
     int64_t fd_counter;
@@ -106,6 +111,17 @@ DWORD WINAPI service_ctrl_handler(DWORD ctrl, DWORD type, LPVOID data,
 VOID WINAPI service_main(DWORD argc, TCHAR *argv[]);
 #endif
 
+static void
+init_dfl_pathnames(void)
+{
+    g_assert(dfl_pathnames.state_dir == NULL);
+    g_assert(dfl_pathnames.pidfile == NULL);
+    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");
+}
+
 static void quit_handler(int sig)
 {
     /* if we're frozen, don't exit unless we're absolutely forced to,
@@ -198,11 +214,11 @@ static void usage(const char *cmd)
 "  -h, --help        display this help and exit\n"
 "\n"
 "Report bugs to <mdroth@linux.vnet.ibm.com>\n"
-    , cmd, QEMU_VERSION, QGA_VIRTIO_PATH_DEFAULT, QGA_PIDFILE_DEFAULT,
+    , cmd, QEMU_VERSION, QGA_VIRTIO_PATH_DEFAULT, dfl_pathnames.pidfile,
 #ifdef CONFIG_FSFREEZE
     QGA_FSFREEZE_HOOK_DEFAULT,
 #endif
-    QGA_STATEDIR_DEFAULT);
+    dfl_pathnames.state_dir);
 }
 
 static const char *ga_log_level_str(GLogLevelFlags level)
@@ -908,11 +924,11 @@ int main(int argc, char **argv)
     const char *sopt = "hVvdm:p:l:f:F::b:s:t:";
     const char *method = NULL, *path = NULL;
     const char *log_filepath = NULL;
-    const char *pid_filepath = QGA_PIDFILE_DEFAULT;
+    const char *pid_filepath;
 #ifdef CONFIG_FSFREEZE
     const char *fsfreeze_hook = NULL;
 #endif
-    const char *state_dir = QGA_STATEDIR_DEFAULT;
+    const char *state_dir;
 #ifdef _WIN32
     const char *service = NULL;
 #endif
@@ -942,6 +958,10 @@ int main(int argc, char **argv)
 
     module_call_init(MODULE_INIT_QAPI);
 
+    init_dfl_pathnames();
+    pid_filepath = dfl_pathnames.pidfile;
+    state_dir = dfl_pathnames.state_dir;
+
     while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
         switch (ch) {
         case 'm':
-- 
1.7.1

[Qemu-devel] [PATCH 3/6] configure: don't save any fixed local_statedir for win32

[Laszlo Ersek]

... because now we can get the dynamic value with
qemu_get_local_state_pathname().

The only user of the fixed value was the guest agent, which we've moved to
qemu_get_local_state_pathname() in the previous patch.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 configure |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/configure b/configure
index 5ae7e4a..ef7259c 100755
--- a/configure
+++ b/configure
@@ -588,7 +588,7 @@ EOF
   qemu_docdir="\${prefix}"
   bindir="\${prefix}"
   sysconfdir="\${prefix}"
-  local_statedir="\${prefix}"
+  local_statedir=
   confsuffix=""
   libs_qga="-lws2_32 -lwinmm -lpowrprof $libs_qga"
 fi
@@ -1083,7 +1083,7 @@ echo "  --docdir=PATH            install documentation in PATH$confsuffix"
 echo "  --bindir=PATH            install binaries in PATH"
 echo "  --libdir=PATH            install libraries in PATH"
 echo "  --sysconfdir=PATH        install config in PATH$confsuffix"
-echo "  --localstatedir=PATH     install local state in PATH"
+echo "  --localstatedir=PATH     install local state in PATH (set at runtime on win32)"
 echo "  --with-confsuffix=SUFFIX suffix for QEMU data inside datadir and sysconfdir [$confsuffix]"
 echo "  --enable-debug-tcg       enable TCG debugging"
 echo "  --disable-debug-tcg      disable TCG debugging (default)"
@@ -3488,10 +3488,12 @@ echo "library directory `eval echo $libdir`"
 echo "libexec directory `eval echo $libexecdir`"
 echo "include directory `eval echo $includedir`"
 echo "config directory  `eval echo $sysconfdir`"
-echo "local state directory   `eval echo $local_statedir`"
 if test "$mingw32" = "no" ; then
+echo "local state directory   `eval echo $local_statedir`"
 echo "Manual directory  `eval echo $mandir`"
 echo "ELF interp prefix $interp_prefix"
+else
+echo "local state directory   queried at runtime"
 fi
 echo "Source path       $source_path"
 echo "C compiler        $cc"
@@ -3612,7 +3614,9 @@ echo "sysconfdir=$sysconfdir" >> $config_host_mak
 echo "qemu_confdir=$qemu_confdir" >> $config_host_mak
 echo "qemu_datadir=$qemu_datadir" >> $config_host_mak
 echo "qemu_docdir=$qemu_docdir" >> $config_host_mak
-echo "qemu_localstatedir=$local_statedir" >> $config_host_mak
+if test "$mingw32" = "no" ; then
+  echo "qemu_localstatedir=$local_statedir" >> $config_host_mak
+fi
 echo "qemu_helperdir=$libexecdir" >> $config_host_mak
 echo "extra_cflags=$EXTRA_CFLAGS" >> $config_host_mak
 echo "extra_ldflags=$EXTRA_LDFLAGS" >> $config_host_mak
-- 
1.7.1

[Qemu-devel] [PATCH 4/6] qga: create state directory on win32

[Laszlo Ersek]

On Win32 the local state directory is application specific and users might
expect qemu-ga to create it automatically.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 qga/main.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index f5f033d..5f2d141 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -1041,6 +1041,20 @@ int main(int argc, char **argv)
         }
     }
 
+#ifdef _WIN32
+    /* On win32 the state directory is application specific (be it the default
+     * or a user override). We got past the command line parsing; let's create
+     * the directory (with any intermediate directories). If we run into an
+     * error later on, we won't try to clean up the directory, it is considered
+     * persistent.
+     */
+    if (g_mkdir_with_parents(state_dir, S_IRWXU) == -1) {
+        g_critical("unable to create (an ancestor of) the state directory"
+                   " '%s': %s", state_dir, strerror(errno));
+        return EXIT_FAILURE;
+    }
+#endif
+
     s = g_malloc0(sizeof(GAState));
     s->log_level = log_level;
     s->log_file = stderr;
-- 
1.7.1

[Qemu-devel] [PATCH 5/6] qga: remove undefined behavior in ga_install_service()

[Laszlo Ersek]

We shouldn't snprintf() from a buffer to the same buffer.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 qga/service-win32.c |   19 +++++++++++--------
 1 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/qga/service-win32.c b/qga/service-win32.c
index 843398a..8a5de8a 100644
--- a/qga/service-win32.c
+++ b/qga/service-win32.c
@@ -39,34 +39,36 @@ int ga_install_service(const char *path, const char *logfile)
 {
     SC_HANDLE manager;
     SC_HANDLE service;
-    TCHAR cmdline[MAX_PATH];
+    TCHAR module_fname[MAX_PATH];
+    GString *cmdline;
 
-    if (GetModuleFileName(NULL, cmdline, MAX_PATH) == 0) {
+    if (GetModuleFileName(NULL, module_fname, MAX_PATH) == 0) {
         printf_win_error("No full path to service's executable");
         return EXIT_FAILURE;
     }
 
-    _snprintf(cmdline, MAX_PATH - strlen(cmdline), "%s -d", cmdline);
+    cmdline = g_string_new(module_fname);
+    g_string_append(cmdline, " -d");
 
     if (path) {
-        _snprintf(cmdline, MAX_PATH - strlen(cmdline), "%s -p %s", cmdline, path);
+        g_string_append_printf(cmdline, " -p %s", path);
     }
     if (logfile) {
-        _snprintf(cmdline, MAX_PATH - strlen(cmdline), "%s -l %s -v",
-            cmdline, logfile);
+        g_string_append_printf(cmdline, " -l %s -v", logfile);
     }
 
-    g_debug("service's cmdline: %s", cmdline);
+    g_debug("service's cmdline: %s", cmdline->str);
 
     manager = OpenSCManager(NULL, NULL, SC_MANAGER_ALL_ACCESS);
     if (manager == NULL) {
         printf_win_error("No handle to service control manager");
+        g_string_free(cmdline, TRUE);
         return EXIT_FAILURE;
     }
 
     service = CreateService(manager, QGA_SERVICE_NAME, QGA_SERVICE_DISPLAY_NAME,
         SERVICE_ALL_ACCESS, SERVICE_WIN32_OWN_PROCESS, SERVICE_AUTO_START,
-        SERVICE_ERROR_NORMAL, cmdline, NULL, NULL, NULL, NULL, NULL);
+        SERVICE_ERROR_NORMAL, cmdline->str, NULL, NULL, NULL, NULL, NULL);
 
     if (service) {
         SERVICE_DESCRIPTION desc = { (char *)QGA_SERVICE_DESCRIPTION };
@@ -80,6 +82,7 @@ int ga_install_service(const char *path, const char *logfile)
     CloseServiceHandle(service);
     CloseServiceHandle(manager);
 
+    g_string_free(cmdline, TRUE);
     return (service == NULL);
 }
 
-- 
1.7.1

[Qemu-devel] [PATCH 6/6] qga: save state directory in ga_install_service()

[Laszlo Ersek]

If the user selects a non-default state directory at service installation
time, we should remember it in the registered service.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 qga/service-win32.h |    3 ++-
 qga/main.c          |   11 ++++++++++-
 qga/service-win32.c |    6 +++++-
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/qga/service-win32.h b/qga/service-win32.h
index 99dfc53..3b9e870 100644
--- a/qga/service-win32.h
+++ b/qga/service-win32.h
@@ -24,7 +24,8 @@ typedef struct GAService {
     SERVICE_STATUS_HANDLE status_handle;
 } GAService;
 
-int ga_install_service(const char *path, const char *logfile);
+int ga_install_service(const char *path, const char *logfile,
+                       const char *state_dir);
 int ga_uninstall_service(void);
 
 #endif
diff --git a/qga/main.c b/qga/main.c
index 5f2d141..0e04e73 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -1022,7 +1022,16 @@ int main(int argc, char **argv)
         case 's':
             service = optarg;
             if (strcmp(service, "install") == 0) {
-                return ga_install_service(path, log_filepath);
+                const char *fixed_state_dir;
+
+                /* If the user passed the "-t" option, we save that state dir
+                 * in the service. Otherwise we let the service fetch the state
+                 * dir from the environment when it starts.
+                 */
+                fixed_state_dir = (state_dir == dfl_pathnames.state_dir) ?
+                                  NULL :
+                                  state_dir;
+                return ga_install_service(path, log_filepath, fixed_state_dir);
             } else if (strcmp(service, "uninstall") == 0) {
                 return ga_uninstall_service();
             } else {
diff --git a/qga/service-win32.c b/qga/service-win32.c
index 8a5de8a..02926ab 100644
--- a/qga/service-win32.c
+++ b/qga/service-win32.c
@@ -35,7 +35,8 @@ static int printf_win_error(const char *text)
     return n;
 }
 
-int ga_install_service(const char *path, const char *logfile)
+int ga_install_service(const char *path, const char *logfile,
+                       const char *state_dir)
 {
     SC_HANDLE manager;
     SC_HANDLE service;
@@ -56,6 +57,9 @@ int ga_install_service(const char *path, const char *logfile)
     if (logfile) {
         g_string_append_printf(cmdline, " -l %s -v", logfile);
     }
+    if (state_dir) {
+        g_string_append_printf(cmdline, " -t %s", state_dir);
+    }
 
     g_debug("service's cmdline: %s", cmdline->str);
 
-- 
1.7.1

[Qemu-devel] [PATCH 7/6] Makefile: create ".../var/run" when installing the POSIX guest agent

[Laszlo Ersek]

Otherwise the default local state directory of POSIX qga won't exist after
installation with a non-standard ${prefix} or DESTDIR.

For now qga is the only user of ".../var" (= $qemu_localstatedir) too, so
don't create that directory either unless we're installing the agent.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 Makefile |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index 7dc0204..1b1630d 100644
--- a/Makefile
+++ b/Makefile
@@ -318,13 +318,21 @@ endif
 install-datadir:
 	$(INSTALL_DIR) "$(DESTDIR)$(qemu_datadir)"
 
+install-localstatedir:
+ifdef CONFIG_POSIX
+ifneq (,$(findstring qemu-ga,$(TOOLS)))
+	$(INSTALL_DIR) "$(DESTDIR)$(qemu_localstatedir)"/run
+endif
+endif
+
 install-confdir:
 	$(INSTALL_DIR) "$(DESTDIR)$(qemu_confdir)"
 
 install-sysconfig: install-datadir install-confdir
 	$(INSTALL_DATA) $(SRC_PATH)/sysconfigs/target/target-x86_64.conf "$(DESTDIR)$(qemu_confdir)"
 
-install: all $(if $(BUILD_DOCS),install-doc) install-sysconfig install-datadir
+install: all $(if $(BUILD_DOCS),install-doc) install-sysconfig \
+install-datadir install-localstatedir
 	$(INSTALL_DIR) "$(DESTDIR)$(bindir)"
 ifneq ($(TOOLS),)
 	$(INSTALL_PROG) $(STRIP_OPT) $(TOOLS) "$(DESTDIR)$(bindir)"
-- 
1.7.1

Re: [Qemu-devel] [PATCH 0/6] local state directory fixes for win32 qga

[mdroth]

On Sat, May 18, 2013 at 06:31:47AM +0200, Laszlo Ersek wrote:
> Qouting patch 2/6:
> 
> > Since commit 39097daf ("qemu-ga: use key-value store to avoid
> > recycling fd handles after restart") we've relied on the state
> > directory for the fd handles' key-value store. Even though we don't
> > support the guest-file-* commands on win32 yet, the key-value store is
> > written, and it's the first use of the state directory on win32. We
> > should have a sensible default for its location.
> 
> Motivated by RHBZ#962669.
> 
> I've perpetrated this in the second half of a Friday->Sunday
> all-nighter, so be gentle. Thanks.

:)

I still need to do some testing to make sure our w32 incantations are
doing what we expect, but I reviewed your series and it looks good to
me. Also, thanks to 4/7 and the fact that the keystore isn't currently
used on w32, we should be able to switch existing users to the new
default state directory without any problems.

Series:

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> 
> Laszlo Ersek (6):
>   osdep: add qemu_get_local_state_pathname()
>   qga: determine default state dir and pidfile dynamically
>   configure: don't save any fixed local_statedir for win32
>   qga: create state directory on win32
>   qga: remove undefined behavior in ga_install_service()
>   qga: save state directory in ga_install_service()
> 
>  configure            |   12 +++++++---
>  include/qemu/osdep.h |   11 +++++++++
>  qga/service-win32.h  |    3 +-
>  qga/main.c           |   57 +++++++++++++++++++++++++++++++++++++++++++------
>  qga/service-win32.c  |   25 ++++++++++++++--------
>  util/oslib-posix.c   |    9 ++++++++
>  util/oslib-win32.c   |   22 +++++++++++++++++++
>  7 files changed, 118 insertions(+), 21 deletions(-)
>