mbox series

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

Message ID 20240716-run-v4-0-5f7a29631168@daynix.com (mailing list archive)
Headers show
Series util: Introduce qemu_get_runtime_dir() | expand

Message

Akihiko Odaki July 16, 2024, 7:27 a.m. UTC
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,

Comments

Michael Tokarev July 16, 2024, 8:06 a.m. UTC | #1
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
Paolo Bonzini July 16, 2024, 8:45 a.m. UTC | #2
Queued, thanks.

Paolo
Akihiko Odaki July 16, 2024, 9:32 a.m. UTC | #3
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
Michael Tokarev July 16, 2024, 9:41 a.m. UTC | #4
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
Daniel P. Berrangé July 16, 2024, 9:56 a.m. UTC | #5
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
Paolo Bonzini July 16, 2024, 10:43 a.m. UTC | #6
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
Akihiko Odaki July 16, 2024, 12:45 p.m. UTC | #7
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
Paolo Bonzini July 16, 2024, 1:29 p.m. UTC | #8
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
Akihiko Odaki July 16, 2024, 1:35 p.m. UTC | #9
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