Am 10.10.2022 um 06:04 hat Bin Meng geschrieben:
> At present there are two callers of get_tmp_filename() and they are
> inconsistent.
>
> One does:
>
> /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
> char *tmp_filename = g_malloc0(PATH_MAX + 1);
> ...
> ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
>
> while the other does:
>
> s->qcow_filename = g_malloc(PATH_MAX);
> ret = get_tmp_filename(s->qcow_filename, PATH_MAX);
>
> As we can see different 'size' arguments are passed. There are also
> platform specific implementations inside the function, and the use
> of snprintf is really undesirable.
>
> The function name is also misleading. It creates a temporary file,
> not just a filename.
>
> Refactor this routine by changing its name and signature to:
>
> char *create_tmp_file(Error **errp)
>
> and use g_get_tmp_dir() / g_mkstemp() for a consistent implementation.
>
> While we are here, add some comments to mention that /var/tmp is
> preferred over /tmp on non-win32 hosts.
>
> Signed-off-by: Bin Meng <bin.m...@windriver.com>
> ---
>
> Changes in v6:
> - use g_mkstemp() and stick to use /var/tmp for non-win32 hosts
>
> Changes in v5:
> - minor change in the commit message
> - add some notes in the function comment block
> - add g_autofree for tmp_filename
>
> Changes in v4:
> - Rename the function to create_tmp_file() and take "Error **errp" as
> a parameter, so that callers can pass errp all the way down to this
> routine.
> - Commit message updated to reflect the latest change
>
> Changes in v3:
> - Do not use errno directly, instead still let get_tmp_filename() return
> a negative number to indicate error
>
> Changes in v2:
> - Use g_autofree and g_steal_pointer
>
> include/block/block_int-common.h | 2 +-
> block.c | 56 +++++++++++++++++---------------
> block/vvfat.c | 7 ++--
> 3 files changed, 34 insertions(+), 31 deletions(-)
>
> diff --git a/include/block/block_int-common.h
> b/include/block/block_int-common.h
> index 8947abab76..d7c0a7e96f 100644
> --- a/include/block/block_int-common.h
> +++ b/include/block/block_int-common.h
> @@ -1230,7 +1230,7 @@ static inline BlockDriverState *child_bs(BdrvChild
> *child)
> }
>
> int bdrv_check_request(int64_t offset, int64_t bytes, Error **errp);
> -int get_tmp_filename(char *filename, int size);
> +char *create_tmp_file(Error **errp);
> void bdrv_parse_filename_strip_prefix(const char *filename, const char
> *prefix,
> QDict *options);
>
> diff --git a/block.c b/block.c
> index 582c205307..8eeaa5623e 100644
> --- a/block.c
> +++ b/block.c
> @@ -860,35 +860,42 @@ int bdrv_probe_geometry(BlockDriverState *bs,
> HDGeometry *geo)
>
> /*
> * Create a uniquely-named empty temporary file.
> - * Return 0 upon success, otherwise a negative errno value.
> + * Return the actual file name used upon success, otherwise NULL.
> + * This string should be freed with g_free() when not needed any longer.
> + *
> + * Note: creating a temporary file for the caller to (re)open is
> + * inherently racy. Use g_file_open_tmp() instead whenever practical.
> */
> -int get_tmp_filename(char *filename, int size)
> +char *create_tmp_file(Error **errp)
> {
> -#ifdef _WIN32
> - char temp_dir[MAX_PATH];
> - /* GetTempFileName requires that its output buffer (4th param)
> - have length MAX_PATH or greater. */
> - assert(size >= MAX_PATH);
> - return (GetTempPath(MAX_PATH, temp_dir)
> - && GetTempFileName(temp_dir, "qem", 0, filename)
> - ? 0 : -GetLastError());
> -#else
> int fd;
> const char *tmpdir;
> - tmpdir = getenv("TMPDIR");
> - if (!tmpdir) {
> + g_autofree char *filename = NULL;
> +
> + tmpdir = g_get_tmp_dir();
> +#ifndef _WIN32
> + /*
> + * See commit 69bef79 ("block: use /var/tmp instead of /tmp for
> -snapshot")
> + *
> + * This function is used to create temporary disk images (like
> -snapshot),
> + * so the files can become very large. /tmp is often a tmpfs where as
> + * /var/tmp is usually on a disk, so more appropriate for disk images.
> + */
> + if (!g_strcmp0(tmpdir, "/tmp")) {
> tmpdir = "/var/tmp";
> }
This is still a slight change in behaviour: If the TMPDIR environment
variable was explicit set to /tmp, QEMU would store the image file in
/tmp before this patch. After the patch, it will use /var/tmp even in
this case.
I suppose this is a tolerable change.
Kevin
