Re: [PATCH v4] block: Refactor get_tmp_filename()

[Markus Armbruster]

Bin Meng <bmeng.cn@gmail.com> writes:

> From: Bin Meng <bin.meng@windriver.com>
>
> 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 this use

Suggest "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_file_open_tmp() for a consistent implementation.
>
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
>
> 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                          | 47 ++++++++++++--------------------
>  block/vvfat.c                    |  7 ++---
>  3 files changed, 21 insertions(+), 35 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 bc85f46eed..b33bd774ae 100644
> --- a/block.c
> +++ b/block.c
> @@ -860,38 +860,27 @@ 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.
>   */

Suggest to add:

    * 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
> +    g_autofree char *name = NULL;
> +    g_autoptr(GError) err = NULL;
>      int fd;
> -    const char *tmpdir;
> -    tmpdir = getenv("TMPDIR");
> -    if (!tmpdir) {
> -        tmpdir = "/var/tmp";
> -    }
> -    if (snprintf(filename, size, "%s/vl.XXXXXX", tmpdir) >= size) {
> -        return -EOVERFLOW;
> -    }
> -    fd = mkstemp(filename);
> +
> +    fd = g_file_open_tmp("vl.XXXXXX", &name, &err);
>      if (fd < 0) {
> -        return -errno;
> +        error_setg_errno(errp, -ENOENT, "%s", err->message);

The actual error need not be ENOENT, and when it isn't, the resulting
error message is misleading.  Instead:

           error_setg(errp, "%s", err->message);

> +        return NULL;
>      }
>      if (close(fd) != 0) {
> -        unlink(filename);
> -        return -errno;
> +        unlink(name);
> +        error_setg_errno(errp, -errno, "Could not close the temporary file");

Yes, but which file?

Error message quality hardly matters, though, since errors are
vanishingly unlikely here.

Moreover, failing the function on this error is questionable.  The
temporary file has been created and is ready for use.  Even the file
descriptor is almost certainly closed; see close(2) under "Dealing with
error returns from close()".  Even a warning seems of doubtful value.  I
think we should simply ignore close() failure here.  Separate patch, I
guess.

> +        return NULL;
>      }
> -    return 0;
> -#endif
> +
> +    return g_steal_pointer(&name);
>  }
>  
>  /*
> @@ -3717,8 +3706,7 @@ static BlockDriverState 
> *bdrv_append_temp_snapshot(BlockDriverState *bs,
>                                                     QDict *snapshot_options,
>                                                     Error **errp)
>  {
> -    /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
> -    char *tmp_filename = g_malloc0(PATH_MAX + 1);
> +    char *tmp_filename = NULL;

Could switch to g_autofree while there.

>      int64_t total_size;
>      QemuOpts *opts = NULL;
>      BlockDriverState *bs_snapshot = NULL;
> @@ -3737,9 +3725,8 @@ static BlockDriverState 
> *bdrv_append_temp_snapshot(BlockDriverState *bs,
>      }
>  
>      /* Create the temporary image */
> -    ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
> -    if (ret < 0) {
> -        error_setg_errno(errp, -ret, "Could not get temporary filename");
> +    tmp_filename = create_tmp_file(errp);
> +    if (!tmp_filename) {
>          goto out;
>      }
>  
> diff --git a/block/vvfat.c b/block/vvfat.c
> index d6dd919683..f9bf8406d3 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -3146,10 +3146,9 @@ static int enable_write_target(BlockDriverState *bs, 
> Error **errp)
>  
>      array_init(&(s->commits), sizeof(commit_t));
>  
> -    s->qcow_filename = g_malloc(PATH_MAX);
> -    ret = get_tmp_filename(s->qcow_filename, PATH_MAX);
> -    if (ret < 0) {
> -        error_setg_errno(errp, -ret, "can't create temporary file");
> +    s->qcow_filename = create_tmp_file(errp);
> +    if (!s->qcow_filename) {
> +        ret = -ENOENT;

I believe this error code will be misleading more often than not.  Can't
see an easy way to do better as long as GLib swallows errno.  Which it
really, really should not, but I digress.

>          goto err;
>      }