| Message ID | 20240716-run-v4-0-5f7a29631168@daynix.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | util: Introduce qemu_get_runtime_dir() | expand |
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
Queued, thanks. Paolo
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
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
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
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
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
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
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
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,