[PATCH v4 0/7] util: Introduce qemu_get_runtime_dir()

[PATCH v4 0/7] util: Introduce qemu_get_runtime_dir()

[Akihiko Odaki]

qemu_get_runtime_dir() returns a dynamically allocated directory path
that is appropriate for storing runtime files. It corresponds to "run"
directory in Unix.

With a tree-wide search, it was found that there are several cases
where such a functionality is implemented so let's have one as a common
utlity function.

A notable feature of qemu_get_runtime_dir() is that it uses
$XDG_RUNTIME_DIR if available. While the function is often called by
executables which requires root privileges, it is still possible that
they are called from a user without privilege to write the system
runtime directory. In fact, I decided to write this patch when I ran
virtiofsd in a Linux namespace created by a normal user and realized
it tries to write the system runtime directory, not writable in this
case. $XDG_RUNTIME_DIR should provide a writable directory in such
cases.

This function does not use qemu_get_local_state_dir() or its logic
for Windows. Actually the implementation of qemu_get_local_state_dir()
for Windows seems not right as it calls g_get_system_data_dirs(),
which refers to $XDG_DATA_DIRS. In Unix terminology, it is basically
"/usr/share", not "/var", which qemu_get_local_state_dir() is intended
to provide. Instead, this function try to use the following in order:
- $XDG_RUNTIME_DIR
- LocalAppData folder
- get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")

This function does not use g_get_user_runtime_dir() either as it
falls back to g_get_user_cache_dir() when $XDG_DATA_DIRS is not
available. In the case, we rather use:
get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")

V2 -> V3:
  Rebase to the current master.
  Dropped patch "qga: Remove platform GUID definitions" since it is
  irrelevant.

V1 -> V2:
  Rebased to the current master since Patchew complains.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
Changes in v4:
- Rebased.
- Link to v3: https://lore.kernel.org/r/20230921075425.16738-1-akihiko.odaki@daynix.com

---
Akihiko Odaki (7):
      util: Introduce qemu_get_runtime_dir()
      ivshmem-server: Use qemu_get_runtime_dir()
      qga: Use qemu_get_runtime_dir()
      scsi: Use qemu_get_runtime_dir()
      module: Use qemu_get_runtime_dir()
      util: Remove qemu_get_local_state_dir()
      spice-app: Use qemu_get_runtime_dir()

 include/qemu/osdep.h          | 10 +++++++---
 contrib/ivshmem-server/main.c | 20 ++++++++++++++++----
 qga/main.c                    |  9 ++++-----
 scsi/qemu-pr-helper.c         |  6 +++---
 ui/spice-app.c                |  4 ++--
 util/module.c                 |  3 ++-
 util/oslib-posix.c            |  9 +++++++--
 util/oslib-win32.c            | 24 ++++++++++++++++++++----
 8 files changed, 61 insertions(+), 24 deletions(-)
---
base-commit: f2cb4026fccfe073f84a4b440e41d3ed0c3134f6
change-id: 20240218-run-6f0d91ec7439

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>

[PATCH v4 1/7] util: Introduce qemu_get_runtime_dir()

[Akihiko Odaki]

qemu_get_runtime_dir() returns a dynamically allocated directory path
that is appropriate for storing runtime files. It corresponds to "run"
directory in Unix.

With a tree-wide search, it was found that there are several cases
where such a functionality is implemented so let's have one as a common
utlity function.

A notable feature of qemu_get_runtime_dir() is that it uses
$XDG_RUNTIME_DIR if available. While the function is often called by
executables which requires root privileges, it is still possible that
they are called from a user without privilege to write the system
runtime directory. In fact, I decided to write this patch when I ran
virtiofsd in a Linux namespace created by a normal user and realized
it tries to write the system runtime directory, not writable in this
case. $XDG_RUNTIME_DIR should provide a writable directory in such
cases.

This function does not use qemu_get_local_state_dir() or its logic
for Windows. Actually the implementation of qemu_get_local_state_dir()
for Windows seems not right as it calls g_get_system_data_dirs(),
which refers to $XDG_DATA_DIRS. In Unix terminology, it is basically
"/usr/share", not "/var", which qemu_get_local_state_dir() is intended
to provide. Instead, this function try to use the following in order:
- $XDG_RUNTIME_DIR
- LocalAppData folder
- get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")

This function does not use g_get_user_runtime_dir() either as it
falls back to g_get_user_cache_dir() when $XDG_DATA_DIRS is not
available. In the case, we rather use:
get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Message-Id: <20230921075425.16738-2-akihiko.odaki@daynix.com>
---
 include/qemu/osdep.h | 12 ++++++++++++
 util/oslib-posix.c   | 11 +++++++++++
 util/oslib-win32.c   | 26 ++++++++++++++++++++++++++
 3 files changed, 49 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 191916f38e6d..fe8609fc1375 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -670,6 +670,18 @@ void qemu_set_cloexec(int fd);
  */
 char *qemu_get_local_state_dir(void);
 
+/**
+ * qemu_get_runtime_dir:
+ *
+ * Return a dynamically allocated directory path that is appropriate for storing
+ * runtime files. It corresponds to "run" directory in Unix, and uses
+ * $XDG_RUNTIME_DIR if available.
+ *
+ * The caller is responsible for releasing the value returned with g_free()
+ * after use.
+ */
+char *qemu_get_runtime_dir(void);
+
 /**
  * qemu_getauxval:
  * @type: the auxiliary vector key to lookup
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index e76441695bdc..9599509a9aa7 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -278,6 +278,17 @@ qemu_get_local_state_dir(void)
     return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR);
 }
 
+char *
+qemu_get_runtime_dir(void)
+{
+    char *env = getenv("XDG_RUNTIME_DIR");
+    if (env) {
+        return g_strdup(env);
+    }
+
+    return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run");
+}
+
 void qemu_set_tty_echo(int fd, bool echo)
 {
     struct termios tty;
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index b623830d624f..8c5a02ee881d 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -27,6 +27,8 @@
  */
 
 #include "qemu/osdep.h"
+#include <shlobj.h>
+#include <wchar.h>
 #include <windows.h>
 #include "qapi/error.h"
 #include "qemu/main-loop.h"
@@ -237,6 +239,30 @@ qemu_get_local_state_dir(void)
     return g_strdup(data_dirs[0]);
 }
 
+char *
+qemu_get_runtime_dir(void)
+{
+    size_t size = GetEnvironmentVariableA("XDG_RUNTIME_DIR", NULL, 0);
+    if (size) {
+        char *env = g_malloc(size);
+        GetEnvironmentVariableA("XDG_RUNTIME_DIR", env, size);
+        return env;
+    }
+
+    PWSTR wpath;
+    const wchar_t *cwpath;
+    if (!SHGetKnownFolderPath(&FOLDERID_LocalAppData, KF_FLAG_DEFAULT, NULL, &wpath)) {
+        cwpath = wpath;
+        size = wcsrtombs(NULL, &cwpath, 0, &(mbstate_t){0}) + 1;
+        char *path = g_malloc(size);
+        wcsrtombs(path, &cwpath, size, &(mbstate_t){0});
+        CoTaskMemFree(wpath);
+        return path;
+    }
+
+    return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run");
+}
+
 void qemu_set_tty_echo(int fd, bool echo)
 {
     HANDLE handle = (HANDLE)_get_osfhandle(fd);

-- 
2.45.2

Re: [PATCH v4 1/7] util: Introduce qemu_get_runtime_dir()

[Daniel P. Berrangé]

On Tue, Jul 16, 2024 at 04:27:31PM +0900, Akihiko Odaki wrote:
> qemu_get_runtime_dir() returns a dynamically allocated directory path
> that is appropriate for storing runtime files. It corresponds to "run"
> directory in Unix.
> 
> With a tree-wide search, it was found that there are several cases
> where such a functionality is implemented so let's have one as a common
> utlity function.
> 
> A notable feature of qemu_get_runtime_dir() is that it uses
> $XDG_RUNTIME_DIR if available. While the function is often called by
> executables which requires root privileges, it is still possible that
> they are called from a user without privilege to write the system
> runtime directory. In fact, I decided to write this patch when I ran
> virtiofsd in a Linux namespace created by a normal user and realized
> it tries to write the system runtime directory, not writable in this
> case. $XDG_RUNTIME_DIR should provide a writable directory in such
> cases.
> 
> This function does not use qemu_get_local_state_dir() or its logic
> for Windows. Actually the implementation of qemu_get_local_state_dir()
> for Windows seems not right as it calls g_get_system_data_dirs(),
> which refers to $XDG_DATA_DIRS. In Unix terminology, it is basically
> "/usr/share", not "/var", which qemu_get_local_state_dir() is intended
> to provide. Instead, this function try to use the following in order:
> - $XDG_RUNTIME_DIR
> - LocalAppData folder
> - get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")
> 
> This function does not use g_get_user_runtime_dir() either as it
> falls back to g_get_user_cache_dir() when $XDG_DATA_DIRS is not
> available. In the case, we rather use:
> get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> Message-Id: <20230921075425.16738-2-akihiko.odaki@daynix.com>
> ---
>  include/qemu/osdep.h | 12 ++++++++++++
>  util/oslib-posix.c   | 11 +++++++++++
>  util/oslib-win32.c   | 26 ++++++++++++++++++++++++++
>  3 files changed, 49 insertions(+)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 191916f38e6d..fe8609fc1375 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -670,6 +670,18 @@ void qemu_set_cloexec(int fd);
>   */
>  char *qemu_get_local_state_dir(void);
>  
> +/**
> + * qemu_get_runtime_dir:
> + *
> + * Return a dynamically allocated directory path that is appropriate for storing
> + * runtime files. It corresponds to "run" directory in Unix, and uses
> + * $XDG_RUNTIME_DIR if available.
> + *
> + * The caller is responsible for releasing the value returned with g_free()
> + * after use.
> + */
> +char *qemu_get_runtime_dir(void);
> +
>  /**
>   * qemu_getauxval:
>   * @type: the auxiliary vector key to lookup
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index e76441695bdc..9599509a9aa7 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -278,6 +278,17 @@ qemu_get_local_state_dir(void)
>      return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR);
>  }
>  
> +char *
> +qemu_get_runtime_dir(void)
> +{
> +    char *env = getenv("XDG_RUNTIME_DIR");
> +    if (env) {
> +        return g_strdup(env);
> +    }
> +
> +    return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run");
> +}

I'm not convinced this is the correct logic to be following.

In the cover letter you mention not using g_get_user_runtime_dir()
because it falls back to XDG_CACHE_HOME, and we need to fallback
to LOCALSTATEDIR/run. This is not right for normal users though,
where falling back to LOCALSTATEDIR/run is always wrong, as it
won't be writable - the g_get_user_runtime_dir() fallback is
desirable for non-root users.

IMHO we should be doing something more like this

#ifndef WIN32
  if (geteuid() == 0) {
     return  get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run");
  } else {
#endif
     return g_get_user_runtime_dir();
#ifndef WIN32
  }
#endif

> +
>  void qemu_set_tty_echo(int fd, bool echo)
>  {
>      struct termios tty;
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index b623830d624f..8c5a02ee881d 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -27,6 +27,8 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include <shlobj.h>
> +#include <wchar.h>
>  #include <windows.h>
>  #include "qapi/error.h"
>  #include "qemu/main-loop.h"
> @@ -237,6 +239,30 @@ qemu_get_local_state_dir(void)
>      return g_strdup(data_dirs[0]);
>  }
>  
> +char *
> +qemu_get_runtime_dir(void)
> +{
> +    size_t size = GetEnvironmentVariableA("XDG_RUNTIME_DIR", NULL, 0);
> +    if (size) {
> +        char *env = g_malloc(size);
> +        GetEnvironmentVariableA("XDG_RUNTIME_DIR", env, size);
> +        return env;
> +    }
> +
> +    PWSTR wpath;
> +    const wchar_t *cwpath;
> +    if (!SHGetKnownFolderPath(&FOLDERID_LocalAppData, KF_FLAG_DEFAULT, NULL, &wpath)) {
> +        cwpath = wpath;
> +        size = wcsrtombs(NULL, &cwpath, 0, &(mbstate_t){0}) + 1;
> +        char *path = g_malloc(size);
> +        wcsrtombs(path, &cwpath, size, &(mbstate_t){0});
> +        CoTaskMemFree(wpath);
> +        return path;
> +    }
> +
> +    return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run");
> +}
> +
>  void qemu_set_tty_echo(int fd, bool echo)
>  {
>      HANDLE handle = (HANDLE)_get_osfhandle(fd);
> 
> -- 
> 2.45.2
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v4 1/7] util: Introduce qemu_get_runtime_dir()

[Akihiko Odaki]

On 2024/07/16 18:53, Daniel P. Berrangé wrote:
> On Tue, Jul 16, 2024 at 04:27:31PM +0900, Akihiko Odaki wrote:
>> qemu_get_runtime_dir() returns a dynamically allocated directory path
>> that is appropriate for storing runtime files. It corresponds to "run"
>> directory in Unix.
>>
>> With a tree-wide search, it was found that there are several cases
>> where such a functionality is implemented so let's have one as a common
>> utlity function.
>>
>> A notable feature of qemu_get_runtime_dir() is that it uses
>> $XDG_RUNTIME_DIR if available. While the function is often called by
>> executables which requires root privileges, it is still possible that
>> they are called from a user without privilege to write the system
>> runtime directory. In fact, I decided to write this patch when I ran
>> virtiofsd in a Linux namespace created by a normal user and realized
>> it tries to write the system runtime directory, not writable in this
>> case. $XDG_RUNTIME_DIR should provide a writable directory in such
>> cases.
>>
>> This function does not use qemu_get_local_state_dir() or its logic
>> for Windows. Actually the implementation of qemu_get_local_state_dir()
>> for Windows seems not right as it calls g_get_system_data_dirs(),
>> which refers to $XDG_DATA_DIRS. In Unix terminology, it is basically
>> "/usr/share", not "/var", which qemu_get_local_state_dir() is intended
>> to provide. Instead, this function try to use the following in order:
>> - $XDG_RUNTIME_DIR
>> - LocalAppData folder
>> - get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")
>>
>> This function does not use g_get_user_runtime_dir() either as it
>> falls back to g_get_user_cache_dir() when $XDG_DATA_DIRS is not
>> available. In the case, we rather use:
>> get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> Message-Id: <20230921075425.16738-2-akihiko.odaki@daynix.com>
>> ---
>>   include/qemu/osdep.h | 12 ++++++++++++
>>   util/oslib-posix.c   | 11 +++++++++++
>>   util/oslib-win32.c   | 26 ++++++++++++++++++++++++++
>>   3 files changed, 49 insertions(+)
>>
>> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>> index 191916f38e6d..fe8609fc1375 100644
>> --- a/include/qemu/osdep.h
>> +++ b/include/qemu/osdep.h
>> @@ -670,6 +670,18 @@ void qemu_set_cloexec(int fd);
>>    */
>>   char *qemu_get_local_state_dir(void);
>>   
>> +/**
>> + * qemu_get_runtime_dir:
>> + *
>> + * Return a dynamically allocated directory path that is appropriate for storing
>> + * runtime files. It corresponds to "run" directory in Unix, and uses
>> + * $XDG_RUNTIME_DIR if available.
>> + *
>> + * The caller is responsible for releasing the value returned with g_free()
>> + * after use.
>> + */
>> +char *qemu_get_runtime_dir(void);
>> +
>>   /**
>>    * qemu_getauxval:
>>    * @type: the auxiliary vector key to lookup
>> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
>> index e76441695bdc..9599509a9aa7 100644
>> --- a/util/oslib-posix.c
>> +++ b/util/oslib-posix.c
>> @@ -278,6 +278,17 @@ qemu_get_local_state_dir(void)
>>       return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR);
>>   }
>>   
>> +char *
>> +qemu_get_runtime_dir(void)
>> +{
>> +    char *env = getenv("XDG_RUNTIME_DIR");
>> +    if (env) {
>> +        return g_strdup(env);
>> +    }
>> +
>> +    return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run");
>> +}
> 
> I'm not convinced this is the correct logic to be following.
> 
> In the cover letter you mention not using g_get_user_runtime_dir()
> because it falls back to XDG_CACHE_HOME, and we need to fallback
> to LOCALSTATEDIR/run. This is not right for normal users though,
> where falling back to LOCALSTATEDIR/run is always wrong, as it
> won't be writable - the g_get_user_runtime_dir() fallback is
> desirable for non-root users.

It also checks LocalAppData, which should be usually available.

g_get_user_runtime_dir() is not a proper fallback in case neither of 
XDG_RUNTIME_DIR and LocalAppData are available. g_get_user_cache_dir(), 
which gets called by g_get_user_runtime_dir(), internally uses:
- XDG_CACHE_HOME or
- FOLDERID_InternetCache

g_get_user_cache_dir() just returns NULL if neither of them is available.

We can't expect XDG_CACHE_HOME is present when XDG_RUNTIME_DIR is 
missing. FOLDERID_InternetCache points to 
%LOCALAPPDATA%\Microsoft\Windows\Temporary Internet Files, according to:
https://learn.microsoft.com/en-us/windows/win32/shell/knownfolderid

So we can't expect FOLDERID_InternetCache is available when LocalAppData 
is missing.

Regards,
Akihiko Odaki

Re: [PATCH v4 1/7] util: Introduce qemu_get_runtime_dir()

[Daniel P. Berrangé]

On Tue, Jul 16, 2024 at 07:52:00PM +0900, Akihiko Odaki wrote:
> On 2024/07/16 18:53, Daniel P. Berrangé wrote:
> > On Tue, Jul 16, 2024 at 04:27:31PM +0900, Akihiko Odaki wrote:
> > > qemu_get_runtime_dir() returns a dynamically allocated directory path
> > > that is appropriate for storing runtime files. It corresponds to "run"
> > > directory in Unix.
> > > 
> > > With a tree-wide search, it was found that there are several cases
> > > where such a functionality is implemented so let's have one as a common
> > > utlity function.
> > > 
> > > A notable feature of qemu_get_runtime_dir() is that it uses
> > > $XDG_RUNTIME_DIR if available. While the function is often called by
> > > executables which requires root privileges, it is still possible that
> > > they are called from a user without privilege to write the system
> > > runtime directory. In fact, I decided to write this patch when I ran
> > > virtiofsd in a Linux namespace created by a normal user and realized
> > > it tries to write the system runtime directory, not writable in this
> > > case. $XDG_RUNTIME_DIR should provide a writable directory in such
> > > cases.
> > > 
> > > This function does not use qemu_get_local_state_dir() or its logic
> > > for Windows. Actually the implementation of qemu_get_local_state_dir()
> > > for Windows seems not right as it calls g_get_system_data_dirs(),
> > > which refers to $XDG_DATA_DIRS. In Unix terminology, it is basically
> > > "/usr/share", not "/var", which qemu_get_local_state_dir() is intended
> > > to provide. Instead, this function try to use the following in order:
> > > - $XDG_RUNTIME_DIR
> > > - LocalAppData folder
> > > - get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")
> > > 
> > > This function does not use g_get_user_runtime_dir() either as it
> > > falls back to g_get_user_cache_dir() when $XDG_DATA_DIRS is not
> > > available. In the case, we rather use:
> > > get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")
> > > 
> > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > > Message-Id: <20230921075425.16738-2-akihiko.odaki@daynix.com>
> > > ---
> > >   include/qemu/osdep.h | 12 ++++++++++++
> > >   util/oslib-posix.c   | 11 +++++++++++
> > >   util/oslib-win32.c   | 26 ++++++++++++++++++++++++++
> > >   3 files changed, 49 insertions(+)
> > > 
> > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > index 191916f38e6d..fe8609fc1375 100644
> > > --- a/include/qemu/osdep.h
> > > +++ b/include/qemu/osdep.h
> > > @@ -670,6 +670,18 @@ void qemu_set_cloexec(int fd);
> > >    */
> > >   char *qemu_get_local_state_dir(void);
> > > +/**
> > > + * qemu_get_runtime_dir:
> > > + *
> > > + * Return a dynamically allocated directory path that is appropriate for storing
> > > + * runtime files. It corresponds to "run" directory in Unix, and uses
> > > + * $XDG_RUNTIME_DIR if available.
> > > + *
> > > + * The caller is responsible for releasing the value returned with g_free()
> > > + * after use.
> > > + */
> > > +char *qemu_get_runtime_dir(void);
> > > +
> > >   /**
> > >    * qemu_getauxval:
> > >    * @type: the auxiliary vector key to lookup
> > > diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> > > index e76441695bdc..9599509a9aa7 100644
> > > --- a/util/oslib-posix.c
> > > +++ b/util/oslib-posix.c
> > > @@ -278,6 +278,17 @@ qemu_get_local_state_dir(void)
> > >       return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR);
> > >   }
> > > +char *
> > > +qemu_get_runtime_dir(void)
> > > +{
> > > +    char *env = getenv("XDG_RUNTIME_DIR");
> > > +    if (env) {
> > > +        return g_strdup(env);
> > > +    }
> > > +
> > > +    return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run");
> > > +}
> > 
> > I'm not convinced this is the correct logic to be following.
> > 
> > In the cover letter you mention not using g_get_user_runtime_dir()
> > because it falls back to XDG_CACHE_HOME, and we need to fallback
> > to LOCALSTATEDIR/run. This is not right for normal users though,
> > where falling back to LOCALSTATEDIR/run is always wrong, as it
> > won't be writable - the g_get_user_runtime_dir() fallback is
> > desirable for non-root users.
> 
> It also checks LocalAppData, which should be usually available.
> 
> g_get_user_runtime_dir() is not a proper fallback in case neither of
> XDG_RUNTIME_DIR and LocalAppData are available. g_get_user_cache_dir(),
> which gets called by g_get_user_runtime_dir(), internally uses:
> - XDG_CACHE_HOME or
> - FOLDERID_InternetCache
> 
> g_get_user_cache_dir() just returns NULL if neither of them is available.
> 
> We can't expect XDG_CACHE_HOME is present when XDG_RUNTIME_DIR is missing.
> FOLDERID_InternetCache points to %LOCALAPPDATA%\Microsoft\Windows\Temporary
> Internet Files, according to:
> https://learn.microsoft.com/en-us/windows/win32/shell/knownfolderid
> 
> So we can't expect FOLDERID_InternetCache is available when LocalAppData is
> missing.

XDG_CACHE_HOME isn't required to be present. Glib will use a fallback
location if XDG_CACHE_HOME isn't set, and it will mkdir() the location
if it doesn't exist.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v4 1/7] util: Introduce qemu_get_runtime_dir()

[Akihiko Odaki]

On 2024/07/16 19:54, Daniel P. Berrangé wrote:
> On Tue, Jul 16, 2024 at 07:52:00PM +0900, Akihiko Odaki wrote:
>> On 2024/07/16 18:53, Daniel P. Berrangé wrote:
>>> On Tue, Jul 16, 2024 at 04:27:31PM +0900, Akihiko Odaki wrote:
>>>> qemu_get_runtime_dir() returns a dynamically allocated directory path
>>>> that is appropriate for storing runtime files. It corresponds to "run"
>>>> directory in Unix.
>>>>
>>>> With a tree-wide search, it was found that there are several cases
>>>> where such a functionality is implemented so let's have one as a common
>>>> utlity function.
>>>>
>>>> A notable feature of qemu_get_runtime_dir() is that it uses
>>>> $XDG_RUNTIME_DIR if available. While the function is often called by
>>>> executables which requires root privileges, it is still possible that
>>>> they are called from a user without privilege to write the system
>>>> runtime directory. In fact, I decided to write this patch when I ran
>>>> virtiofsd in a Linux namespace created by a normal user and realized
>>>> it tries to write the system runtime directory, not writable in this
>>>> case. $XDG_RUNTIME_DIR should provide a writable directory in such
>>>> cases.
>>>>
>>>> This function does not use qemu_get_local_state_dir() or its logic
>>>> for Windows. Actually the implementation of qemu_get_local_state_dir()
>>>> for Windows seems not right as it calls g_get_system_data_dirs(),
>>>> which refers to $XDG_DATA_DIRS. In Unix terminology, it is basically
>>>> "/usr/share", not "/var", which qemu_get_local_state_dir() is intended
>>>> to provide. Instead, this function try to use the following in order:
>>>> - $XDG_RUNTIME_DIR
>>>> - LocalAppData folder
>>>> - get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")
>>>>
>>>> This function does not use g_get_user_runtime_dir() either as it
>>>> falls back to g_get_user_cache_dir() when $XDG_DATA_DIRS is not
>>>> available. In the case, we rather use:
>>>> get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")
>>>>
>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>> Message-Id: <20230921075425.16738-2-akihiko.odaki@daynix.com>
>>>> ---
>>>>    include/qemu/osdep.h | 12 ++++++++++++
>>>>    util/oslib-posix.c   | 11 +++++++++++
>>>>    util/oslib-win32.c   | 26 ++++++++++++++++++++++++++
>>>>    3 files changed, 49 insertions(+)
>>>>
>>>> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>>>> index 191916f38e6d..fe8609fc1375 100644
>>>> --- a/include/qemu/osdep.h
>>>> +++ b/include/qemu/osdep.h
>>>> @@ -670,6 +670,18 @@ void qemu_set_cloexec(int fd);
>>>>     */
>>>>    char *qemu_get_local_state_dir(void);
>>>> +/**
>>>> + * qemu_get_runtime_dir:
>>>> + *
>>>> + * Return a dynamically allocated directory path that is appropriate for storing
>>>> + * runtime files. It corresponds to "run" directory in Unix, and uses
>>>> + * $XDG_RUNTIME_DIR if available.
>>>> + *
>>>> + * The caller is responsible for releasing the value returned with g_free()
>>>> + * after use.
>>>> + */
>>>> +char *qemu_get_runtime_dir(void);
>>>> +
>>>>    /**
>>>>     * qemu_getauxval:
>>>>     * @type: the auxiliary vector key to lookup
>>>> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
>>>> index e76441695bdc..9599509a9aa7 100644
>>>> --- a/util/oslib-posix.c
>>>> +++ b/util/oslib-posix.c
>>>> @@ -278,6 +278,17 @@ qemu_get_local_state_dir(void)
>>>>        return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR);
>>>>    }
>>>> +char *
>>>> +qemu_get_runtime_dir(void)
>>>> +{
>>>> +    char *env = getenv("XDG_RUNTIME_DIR");
>>>> +    if (env) {
>>>> +        return g_strdup(env);
>>>> +    }
>>>> +
>>>> +    return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run");
>>>> +}
>>>
>>> I'm not convinced this is the correct logic to be following.
>>>
>>> In the cover letter you mention not using g_get_user_runtime_dir()
>>> because it falls back to XDG_CACHE_HOME, and we need to fallback
>>> to LOCALSTATEDIR/run. This is not right for normal users though,
>>> where falling back to LOCALSTATEDIR/run is always wrong, as it
>>> won't be writable - the g_get_user_runtime_dir() fallback is
>>> desirable for non-root users.
>>
>> It also checks LocalAppData, which should be usually available.
>>
>> g_get_user_runtime_dir() is not a proper fallback in case neither of
>> XDG_RUNTIME_DIR and LocalAppData are available. g_get_user_cache_dir(),
>> which gets called by g_get_user_runtime_dir(), internally uses:
>> - XDG_CACHE_HOME or
>> - FOLDERID_InternetCache
>>
>> g_get_user_cache_dir() just returns NULL if neither of them is available.
>>
>> We can't expect XDG_CACHE_HOME is present when XDG_RUNTIME_DIR is missing.
>> FOLDERID_InternetCache points to %LOCALAPPDATA%\Microsoft\Windows\Temporary
>> Internet Files, according to:
>> https://learn.microsoft.com/en-us/windows/win32/shell/knownfolderid
>>
>> So we can't expect FOLDERID_InternetCache is available when LocalAppData is
>> missing.
> 
> XDG_CACHE_HOME isn't required to be present. Glib will use a fallback
> location if XDG_CACHE_HOME isn't set, and it will mkdir() the location
> if it doesn't exist.

The fallback location is FOLDERID_InternetCache, which is not better 
than looking at LocalAppData.

Regards,
Akihiko Odaki

[PATCH v4 2/7] ivshmem-server: Use qemu_get_runtime_dir()

[Akihiko Odaki]

qemu_get_runtime_dir() is used to construct the default PID file path.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Message-Id: <20230921075425.16738-3-akihiko.odaki@daynix.com>
---
 contrib/ivshmem-server/main.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/contrib/ivshmem-server/main.c b/contrib/ivshmem-server/main.c
index 5901f17707e0..313124dd4520 100644
--- a/contrib/ivshmem-server/main.c
+++ b/contrib/ivshmem-server/main.c
@@ -14,7 +14,6 @@
 
 #define IVSHMEM_SERVER_DEFAULT_VERBOSE        0
 #define IVSHMEM_SERVER_DEFAULT_FOREGROUND     0
-#define IVSHMEM_SERVER_DEFAULT_PID_FILE       "/var/run/ivshmem-server.pid"
 #define IVSHMEM_SERVER_DEFAULT_UNIX_SOCK_PATH "/tmp/ivshmem_socket"
 #define IVSHMEM_SERVER_DEFAULT_SHM_PATH       "ivshmem"
 #define IVSHMEM_SERVER_DEFAULT_SHM_SIZE       (4 * 1024 * 1024)
@@ -35,15 +34,23 @@ typedef struct IvshmemServerArgs {
     unsigned n_vectors;
 } IvshmemServerArgs;
 
+static char *ivshmem_server_get_default_pid_file(void)
+{
+    g_autofree char *run = qemu_get_runtime_dir();
+    return g_build_filename(run, "ivshmem-server.pid", NULL);
+}
+
 static void
 ivshmem_server_usage(const char *progname)
 {
+    g_autofree char *pid_file = ivshmem_server_get_default_pid_file();
+
     printf("Usage: %s [OPTION]...\n"
            "  -h: show this help\n"
            "  -v: verbose mode\n"
            "  -F: foreground mode (default is to daemonize)\n"
            "  -p <pid-file>: path to the PID file (used in daemon mode only)\n"
-           "     default " IVSHMEM_SERVER_DEFAULT_PID_FILE "\n"
+           "     default %s\n"
            "  -S <unix-socket-path>: path to the unix socket to listen to\n"
            "     default " IVSHMEM_SERVER_DEFAULT_UNIX_SOCK_PATH "\n"
            "  -M <shm-name>: POSIX shared memory object to use\n"
@@ -54,7 +61,7 @@ ivshmem_server_usage(const char *progname)
            "     default %u\n"
            "  -n <nvectors>: number of vectors\n"
            "     default %u\n",
-           progname, IVSHMEM_SERVER_DEFAULT_SHM_SIZE,
+           progname, pid_file, IVSHMEM_SERVER_DEFAULT_SHM_SIZE,
            IVSHMEM_SERVER_DEFAULT_N_VECTORS);
 }
 
@@ -189,10 +196,10 @@ main(int argc, char *argv[])
 {
     IvshmemServer server;
     struct sigaction sa, sa_quit;
+    g_autofree char *default_pid_file = NULL;
     IvshmemServerArgs args = {
         .verbose = IVSHMEM_SERVER_DEFAULT_VERBOSE,
         .foreground = IVSHMEM_SERVER_DEFAULT_FOREGROUND,
-        .pid_file = IVSHMEM_SERVER_DEFAULT_PID_FILE,
         .unix_socket_path = IVSHMEM_SERVER_DEFAULT_UNIX_SOCK_PATH,
         .shm_path = IVSHMEM_SERVER_DEFAULT_SHM_PATH,
         .use_shm_open = true,
@@ -207,6 +214,11 @@ main(int argc, char *argv[])
      */
     printf("*** Example code, do not use in production ***\n");
 
+    qemu_init_exec_dir(argv[0]);
+
+    default_pid_file = ivshmem_server_get_default_pid_file();
+    args.pid_file = default_pid_file;
+
     /* parse arguments, will exit on error */
     ivshmem_server_parse_args(&args, argc, argv);
 

-- 
2.45.2

[PATCH v4 3/7] qga: Use qemu_get_runtime_dir()

[Akihiko Odaki]

qemu_get_runtime_dir() is used to construct the default state directory.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Message-Id: <20230921075425.16738-5-akihiko.odaki@daynix.com>
---
 qga/main.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index f4d5f15bb3d5..4bb6f7e27b66 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -45,12 +45,11 @@
 #define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent.0"
 #endif /* CONFIG_BSD */
 #define QGA_SERIAL_PATH_DEFAULT "/dev/ttyS0"
-#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"
 #define QGA_SERIAL_PATH_DEFAULT "COM1"
 #endif
+#define QGA_STATE_RELATIVE_DIR  "qemu-ga"
 #ifdef CONFIG_FSFREEZE
 #define QGA_FSFREEZE_HOOK_DEFAULT CONFIG_QEMU_CONFDIR "/fsfreeze-hook"
 #endif
@@ -129,12 +128,12 @@ static void stop_agent(GAState *s, bool requested);
 static void
 init_dfl_pathnames(void)
 {
-    g_autofree char *state = qemu_get_local_state_dir();
+    g_autofree char *run = qemu_get_runtime_dir();
 
     g_assert(dfl_pathnames.state_dir == NULL);
     g_assert(dfl_pathnames.pidfile == NULL);
-    dfl_pathnames.state_dir = g_build_filename(state, QGA_STATE_RELATIVE_DIR, NULL);
-    dfl_pathnames.pidfile = g_build_filename(state, QGA_STATE_RELATIVE_DIR, "qemu-ga.pid", NULL);
+    dfl_pathnames.state_dir = g_build_filename(run, QGA_STATE_RELATIVE_DIR, NULL);
+    dfl_pathnames.pidfile = g_build_filename(run, QGA_STATE_RELATIVE_DIR, "qemu-ga.pid", NULL);
 }
 
 static void quit_handler(int sig)

-- 
2.45.2

[PATCH v4 4/7] scsi: Use qemu_get_runtime_dir()

[Akihiko Odaki]

qemu_get_runtime_dir() is used to construct the default paths.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Message-Id: <20230921075425.16738-6-akihiko.odaki@daynix.com>
---
 scsi/qemu-pr-helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index c6c6347e9b6a..507f23357f6b 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -77,10 +77,10 @@ static int gid = -1;
 
 static void compute_default_paths(void)
 {
-    g_autofree char *state = qemu_get_local_state_dir();
+    g_autofree char *run = qemu_get_runtime_dir();
 
-    socket_path = g_build_filename(state, "run", "qemu-pr-helper.sock", NULL);
-    pidfile = g_build_filename(state, "run", "qemu-pr-helper.pid", NULL);
+    socket_path = g_build_filename(run, "qemu-pr-helper.sock", NULL);
+    pidfile = g_build_filename(run, "qemu-pr-helper.pid", NULL);
 }
 
 static void usage(const char *name)

-- 
2.45.2

[PATCH v4 5/7] module: Use qemu_get_runtime_dir()

[Akihiko Odaki]

qemu_get_runtime_dir() is used to construct the path to module upgrades.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Message-Id: <20230921075425.16738-7-akihiko.odaki@daynix.com>
---
 util/module.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/util/module.c b/util/module.c
index 32e263163c75..580658edf486 100644
--- a/util/module.c
+++ b/util/module.c
@@ -242,7 +242,8 @@ int module_load(const char *prefix, const char *name, Error **errp)
     version_dir = g_strcanon(g_strdup(QEMU_PKGVERSION),
                              G_CSET_A_2_Z G_CSET_a_2_z G_CSET_DIGITS "+-.~",
                              '_');
-    dirs[n_dirs++] = g_strdup_printf("/var/run/qemu/%s", version_dir);
+    g_autofree char *run = qemu_get_runtime_dir();
+    dirs[n_dirs++] = g_build_filename(run, "qemu", version_dir, NULL);
 #endif
     assert(n_dirs <= ARRAY_SIZE(dirs));
 

-- 
2.45.2

[PATCH v4 6/7] util: Remove qemu_get_local_state_dir()

[Akihiko Odaki]

There are no users of the function anymore.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Message-Id: <20230921075425.16738-8-akihiko.odaki@daynix.com>
---
 include/qemu/osdep.h |  8 --------
 util/oslib-posix.c   |  6 ------
 util/oslib-win32.c   | 10 ----------
 3 files changed, 24 deletions(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index fe8609fc1375..189fdf4fb6f9 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -662,14 +662,6 @@ ssize_t qemu_write_full(int fd, const void *buf, size_t count)
 
 void qemu_set_cloexec(int fd);
 
-/* Return a dynamically allocated directory path that is appropriate for storing
- * local state.
- *
- * The caller is responsible for releasing the value returned with g_free()
- * after use.
- */
-char *qemu_get_local_state_dir(void);
-
 /**
  * qemu_get_runtime_dir:
  *
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 9599509a9aa7..60bbd9786b79 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -272,12 +272,6 @@ int qemu_socketpair(int domain, int type, int protocol, int sv[2])
     return ret;
 }
 
-char *
-qemu_get_local_state_dir(void)
-{
-    return get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR);
-}
-
 char *
 qemu_get_runtime_dir(void)
 {
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 8c5a02ee881d..504a75f548ab 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -229,16 +229,6 @@ int qemu_get_thread_id(void)
     return GetCurrentThreadId();
 }
 
-char *
-qemu_get_local_state_dir(void)
-{
-    const char * const *data_dirs = g_get_system_data_dirs();
-
-    g_assert(data_dirs && data_dirs[0]);
-
-    return g_strdup(data_dirs[0]);
-}
-
 char *
 qemu_get_runtime_dir(void)
 {

-- 
2.45.2

[PATCH v4 7/7] spice-app: Use qemu_get_runtime_dir()

[Akihiko Odaki]

qemu_get_runtime_dir() provides QEMU-specific fallback of runtime
directory.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Message-Id: <20230921075425.16738-9-akihiko.odaki@daynix.com>
---
 ui/spice-app.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ui/spice-app.c b/ui/spice-app.c
index a10b4a58fe74..bbe17358c8d4 100644
--- a/ui/spice-app.c
+++ b/ui/spice-app.c
@@ -151,8 +151,8 @@ static void spice_app_display_early_init(DisplayOptions *opts)
     atexit(spice_app_atexit);
 
     if (qemu_name) {
-        app_dir = g_build_filename(g_get_user_runtime_dir(),
-                                   "qemu", qemu_name, NULL);
+        g_autofree char *run = qemu_get_runtime_dir();
+        app_dir = g_build_filename(run, "qemu", qemu_name, NULL);
         if (g_mkdir_with_parents(app_dir, S_IRWXU) < -1) {
             error_report("Failed to create directory %s: %s",
                          app_dir, strerror(errno));

-- 
2.45.2

Re: [PATCH v4 0/7] util: Introduce qemu_get_runtime_dir()

[Michael Tokarev]

16.07.2024 10:27, Akihiko Odaki wrote:
> qemu_get_runtime_dir() returns a dynamically allocated directory path
> that is appropriate for storing runtime files. It corresponds to "run"
> directory in Unix.

Since runtime dir is always used with a filename within, how about

   char *qemu_get_runtime_path(const char *filename)

which return RUNTIME_DIR/filename instead of just RUNTIME_DIR ?

Thanks,

/mjt

-- 
GPG Key transition (from rsa2048 to rsa4096) since 2024-04-24.
New key: rsa4096/61AD3D98ECDF2C8E  9D8B E14E 3F2A 9DD7 9199  28F1 61AD 3D98 ECDF 2C8E
Old key: rsa2048/457CE0A0804465C5  6EE1 95D1 886E 8FFB 810D  4324 457C E0A0 8044 65C5
Transition statement: http://www.corpit.ru/mjt/gpg-transition-2024.txt

Re: [PATCH v4 0/7] util: Introduce qemu_get_runtime_dir()

[Akihiko Odaki]

On 2024/07/16 17:06, Michael Tokarev wrote:
> 16.07.2024 10:27, Akihiko Odaki wrote:
>> qemu_get_runtime_dir() returns a dynamically allocated directory path
>> that is appropriate for storing runtime files. It corresponds to "run"
>> directory in Unix.
> 
> Since runtime dir is always used with a filename within, how about
> 
>    char *qemu_get_runtime_path(const char *filename)
> 
> which return RUNTIME_DIR/filename instead of just RUNTIME_DIR ?

I'm not sure. Such a function would be certainly useful, but I slightly 
feel such a function concerns with too many responsibilities. Getting a 
runtime directory is one responsibility, and how to use is another. They 
are clearly distinguished; it does not matter how the path to the 
runtime directory is used after acquiring it. For example, you can keep 
the path to the runtime directory, and derive the paths to two files in 
the directory.

I don't object to such a change, but I rather keep this series as is 
unless there is anything wrong else.

Regards,
Akihiko Odaki

Re: [PATCH v4 0/7] util: Introduce qemu_get_runtime_dir()

[Michael Tokarev]

16.07.2024 12:32, Akihiko Odaki wrote:
> On 2024/07/16 17:06, Michael Tokarev wrote:

>> Since runtime dir is always used with a filename within, how about
>>
>>    char *qemu_get_runtime_path(const char *filename)
>>
>> which return RUNTIME_DIR/filename instead of just RUNTIME_DIR ?
> 
> I'm not sure. Such a function would be certainly useful, but I slightly feel such a function concerns with too many responsibilities. Getting a 
> runtime directory is one responsibility, and how to use is another. They are clearly distinguished; it does not matter how the path to the runtime 
> directory is used after acquiring it. For example, you can keep the path to the runtime directory, and derive the paths to two files in the directory.

You can pass NULL as filename and get the directory itself.

/mjt

-- 
GPG Key transition (from rsa2048 to rsa4096) since 2024-04-24.
New key: rsa4096/61AD3D98ECDF2C8E  9D8B E14E 3F2A 9DD7 9199  28F1 61AD 3D98 ECDF 2C8E
Old key: rsa2048/457CE0A0804465C5  6EE1 95D1 886E 8FFB 810D  4324 457C E0A0 8044 65C5
Transition statement: http://www.corpit.ru/mjt/gpg-transition-2024.txt

Re: [PATCH v4 0/7] util: Introduce qemu_get_runtime_dir()

[Daniel P. Berrangé]

On Tue, Jul 16, 2024 at 11:06:57AM +0300, Michael Tokarev wrote:
> 16.07.2024 10:27, Akihiko Odaki wrote:
> > qemu_get_runtime_dir() returns a dynamically allocated directory path
> > that is appropriate for storing runtime files. It corresponds to "run"
> > directory in Unix.
> 
> Since runtime dir is always used with a filename within, how about
> 
>   char *qemu_get_runtime_path(const char *filename)
> 
> which return RUNTIME_DIR/filename instead of just RUNTIME_DIR ?

Yeah, I agree, every single caller of the function goes on to call
g_build_filename with the result. The helper should just be building
the filename itself.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v4 0/7] util: Introduce qemu_get_runtime_dir()

[Paolo Bonzini]

On Tue, Jul 16, 2024 at 11:56 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Tue, Jul 16, 2024 at 11:06:57AM +0300, Michael Tokarev wrote:
> > 16.07.2024 10:27, Akihiko Odaki wrote:
> > > qemu_get_runtime_dir() returns a dynamically allocated directory path
> > > that is appropriate for storing runtime files. It corresponds to "run"
> > > directory in Unix.
> >
> > Since runtime dir is always used with a filename within, how about
> >
> >   char *qemu_get_runtime_path(const char *filename)
> >
> > which return RUNTIME_DIR/filename instead of just RUNTIME_DIR ?
>
> Yeah, I agree, every single caller of the function goes on to call
> g_build_filename with the result. The helper should just be building
> the filename itself.

That would mean using variable arguments and g_build_filename_valist().

Paolo

Re: [PATCH v4 0/7] util: Introduce qemu_get_runtime_dir()

[Akihiko Odaki]

On 2024/07/16 19:43, Paolo Bonzini wrote:
> On Tue, Jul 16, 2024 at 11:56 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>>
>> On Tue, Jul 16, 2024 at 11:06:57AM +0300, Michael Tokarev wrote:
>>> 16.07.2024 10:27, Akihiko Odaki wrote:
>>>> qemu_get_runtime_dir() returns a dynamically allocated directory path
>>>> that is appropriate for storing runtime files. It corresponds to "run"
>>>> directory in Unix.
>>>
>>> Since runtime dir is always used with a filename within, how about
>>>
>>>    char *qemu_get_runtime_path(const char *filename)
>>>
>>> which return RUNTIME_DIR/filename instead of just RUNTIME_DIR ?
>>
>> Yeah, I agree, every single caller of the function goes on to call
>> g_build_filename with the result. The helper should just be building
>> the filename itself.
> 
> That would mean using variable arguments and g_build_filename_valist().

We can't prepend an element to va_list. The best thing I came up with is 
a macro as follows:

#define get_runtime_path(first_element, ...) ({ \
   g_autofree char *_s = qemu_get_runtime_dir(); \
   g_build_filename(_s, first_element, ...); \
})

But it makes me wonder if we need such a macro.

Regards,
Akihiko Odaki

Re: [PATCH v4 0/7] util: Introduce qemu_get_runtime_dir()

[Paolo Bonzini]

On Tue, Jul 16, 2024 at 2:46 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2024/07/16 19:43, Paolo Bonzini wrote:
> > On Tue, Jul 16, 2024 at 11:56 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >>
> >> On Tue, Jul 16, 2024 at 11:06:57AM +0300, Michael Tokarev wrote:
> >>> 16.07.2024 10:27, Akihiko Odaki wrote:
> >>>> qemu_get_runtime_dir() returns a dynamically allocated directory path
> >>>> that is appropriate for storing runtime files. It corresponds to "run"
> >>>> directory in Unix.
> >>>
> >>> Since runtime dir is always used with a filename within, how about
> >>>
> >>>    char *qemu_get_runtime_path(const char *filename)
> >>>
> >>> which return RUNTIME_DIR/filename instead of just RUNTIME_DIR ?
> >>
> >> Yeah, I agree, every single caller of the function goes on to call
> >> g_build_filename with the result. The helper should just be building
> >> the filename itself.
> >
> > That would mean using variable arguments and g_build_filename_valist().
>
> We can't prepend an element to va_list.

You could do it in two steps, with g_build_filename(runtime_dir,
first) followed by g_build_filename_valist(result, ap); doing these
steps only if if first != NULL of course.

But I agree that leaving the concatenation in the caller is not
particularly worse, and makes qemu_get_runtime_dir() more readable.

Paolo


Paolo

Re: [PATCH v4 0/7] util: Introduce qemu_get_runtime_dir()

[Akihiko Odaki]

On 2024/07/16 22:29, Paolo Bonzini wrote:
> On Tue, Jul 16, 2024 at 2:46 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2024/07/16 19:43, Paolo Bonzini wrote:
>>> On Tue, Jul 16, 2024 at 11:56 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>>>>
>>>> On Tue, Jul 16, 2024 at 11:06:57AM +0300, Michael Tokarev wrote:
>>>>> 16.07.2024 10:27, Akihiko Odaki wrote:
>>>>>> qemu_get_runtime_dir() returns a dynamically allocated directory path
>>>>>> that is appropriate for storing runtime files. It corresponds to "run"
>>>>>> directory in Unix.
>>>>>
>>>>> Since runtime dir is always used with a filename within, how about
>>>>>
>>>>>     char *qemu_get_runtime_path(const char *filename)
>>>>>
>>>>> which return RUNTIME_DIR/filename instead of just RUNTIME_DIR ?
>>>>
>>>> Yeah, I agree, every single caller of the function goes on to call
>>>> g_build_filename with the result. The helper should just be building
>>>> the filename itself.
>>>
>>> That would mean using variable arguments and g_build_filename_valist().
>>
>> We can't prepend an element to va_list.
> 
> You could do it in two steps, with g_build_filename(runtime_dir,
> first) followed by g_build_filename_valist(result, ap); doing these
> steps only if if first != NULL of course.

It unfortunately involves an extra effort to free the result of the 
first call of g_build_filename().

Regards,
Akihiko Odaki

Re: [PATCH v4 0/7] util: Introduce qemu_get_runtime_dir()

[Paolo Bonzini]

Queued, thanks.

Paolo

Re: [PATCH v4 0/7] util: Introduce qemu_get_runtime_dir()

[Akihiko Odaki]

Hi,

It seems this series has been forgotten for a while. Can anyone take a 
look at it?

Regards,
Akihiko Odaki

On 2024/07/16 16:27, Akihiko Odaki wrote:
> qemu_get_runtime_dir() returns a dynamically allocated directory path
> that is appropriate for storing runtime files. It corresponds to "run"
> directory in Unix.
> 
> With a tree-wide search, it was found that there are several cases
> where such a functionality is implemented so let's have one as a common
> utlity function.
> 
> A notable feature of qemu_get_runtime_dir() is that it uses
> $XDG_RUNTIME_DIR if available. While the function is often called by
> executables which requires root privileges, it is still possible that
> they are called from a user without privilege to write the system
> runtime directory. In fact, I decided to write this patch when I ran
> virtiofsd in a Linux namespace created by a normal user and realized
> it tries to write the system runtime directory, not writable in this
> case. $XDG_RUNTIME_DIR should provide a writable directory in such
> cases.
> 
> This function does not use qemu_get_local_state_dir() or its logic
> for Windows. Actually the implementation of qemu_get_local_state_dir()
> for Windows seems not right as it calls g_get_system_data_dirs(),
> which refers to $XDG_DATA_DIRS. In Unix terminology, it is basically
> "/usr/share", not "/var", which qemu_get_local_state_dir() is intended
> to provide. Instead, this function try to use the following in order:
> - $XDG_RUNTIME_DIR
> - LocalAppData folder
> - get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")
> 
> This function does not use g_get_user_runtime_dir() either as it
> falls back to g_get_user_cache_dir() when $XDG_DATA_DIRS is not
> available. In the case, we rather use:
> get_relocated_path(CONFIG_QEMU_LOCALSTATEDIR "/run")
> 
> V2 -> V3:
>    Rebase to the current master.
>    Dropped patch "qga: Remove platform GUID definitions" since it is
>    irrelevant.
> 
> V1 -> V2:
>    Rebased to the current master since Patchew complains.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
> Changes in v4:
> - Rebased.
> - Link to v3: https://lore.kernel.org/r/20230921075425.16738-1-akihiko.odaki@daynix.com
> 
> ---
> Akihiko Odaki (7):
>        util: Introduce qemu_get_runtime_dir()
>        ivshmem-server: Use qemu_get_runtime_dir()
>        qga: Use qemu_get_runtime_dir()
>        scsi: Use qemu_get_runtime_dir()
>        module: Use qemu_get_runtime_dir()
>        util: Remove qemu_get_local_state_dir()
>        spice-app: Use qemu_get_runtime_dir()
> 
>   include/qemu/osdep.h          | 10 +++++++---
>   contrib/ivshmem-server/main.c | 20 ++++++++++++++++----
>   qga/main.c                    |  9 ++++-----
>   scsi/qemu-pr-helper.c         |  6 +++---
>   ui/spice-app.c                |  4 ++--
>   util/module.c                 |  3 ++-
>   util/oslib-posix.c            |  9 +++++++--
>   util/oslib-win32.c            | 24 ++++++++++++++++++++----
>   8 files changed, 61 insertions(+), 24 deletions(-)
> ---
> base-commit: f2cb4026fccfe073f84a4b440e41d3ed0c3134f6
> change-id: 20240218-run-6f0d91ec7439
> 
> Best regards,