[PATCH v3] block: Refactor get_tmp_filename()

[PATCH v3] block: Refactor get_tmp_filename()

[Bin Meng]

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
of snprintf is really undesirable.

Refactor this routine by changing its signature to:

    int get_tmp_filename(char **filename)

and use g_file_open_tmp() for a consistent implementation.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
---

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                          | 36 ++++++++++----------------------
 block/vvfat.c                    |  3 +--
 3 files changed, 13 insertions(+), 28 deletions(-)

diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 8947abab76..eb30193198 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);
+int get_tmp_filename(char **filename);
 void bdrv_parse_filename_strip_prefix(const char *filename, const char *prefix,
                                       QDict *options);
 
diff --git a/block.c b/block.c
index bc85f46eed..55ad257241 100644
--- a/block.c
+++ b/block.c
@@ -861,37 +861,24 @@ int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo)
 /*
  * Create a uniquely-named empty temporary file.
  * Return 0 upon success, otherwise a negative errno value.
+ * The caller function is responsible for freeing *filename.
  */
-int get_tmp_filename(char *filename, int size)
+int get_tmp_filename(char **filename)
 {
-#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;
     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, NULL);
     if (fd < 0) {
-        return -errno;
+        return -ENOENT;
     }
     if (close(fd) != 0) {
-        unlink(filename);
+        unlink(name);
         return -errno;
     }
+
+    *filename = g_steal_pointer(&name);
     return 0;
-#endif
 }
 
 /*
@@ -3717,8 +3704,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;
     int64_t total_size;
     QemuOpts *opts = NULL;
     BlockDriverState *bs_snapshot = NULL;
@@ -3737,7 +3723,7 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
     }
 
     /* Create the temporary image */
-    ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
+    ret = get_tmp_filename(&tmp_filename);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not get temporary filename");
         goto out;
diff --git a/block/vvfat.c b/block/vvfat.c
index d6dd919683..43f42fd1ea 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3146,8 +3146,7 @@ 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);
+    ret = get_tmp_filename(&s->qcow_filename);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "can't create temporary file");
         goto err;
-- 
2.34.1

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

[Daniel P. Berrangé]

On Sun, Sep 25, 2022 at 12:32:00AM +0800, Bin Meng wrote:
> 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
> of snprintf is really undesirable.
> 
> Refactor this routine by changing its signature to:
> 
>     int get_tmp_filename(char **filename)
> 
> and use g_file_open_tmp() for a consistent implementation.
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
> 
> 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                          | 36 ++++++++++----------------------
>  block/vvfat.c                    |  3 +--
>  3 files changed, 13 insertions(+), 28 deletions(-)
> 
> diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
> index 8947abab76..eb30193198 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);
> +int get_tmp_filename(char **filename);

Change it to:

   char *get_tmp_filename(Error **errp);


> @@ -3737,7 +3723,7 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
>      }
>  
>      /* Create the temporary image */
> -    ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
> +    ret = get_tmp_filename(&tmp_filename);
>      if (ret < 0) {
>          error_setg_errno(errp, -ret, "Could not get temporary filename");

And pass 'errp' straight into get_tmp_filename, thus avoid the
different error message text here vs below

eg

     tmp_filename = get_tmp_filename(errp);
     if (!tmp_filename) {
         goto out;
     }


>          goto out;
> diff --git a/block/vvfat.c b/block/vvfat.c
> index d6dd919683..43f42fd1ea 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -3146,8 +3146,7 @@ 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);
> +    ret = get_tmp_filename(&s->qcow_filename);
>      if (ret < 0) {
>          error_setg_errno(errp, -ret, "can't create temporary file");
>          goto err;

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|