Re: [PATCH v6 2/2] block: Refactor get_tmp_filename()

[Kevin Wolf]

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.meng@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