[PATCH v6 1/2] block: Ignore close() failure in get_tmp_filename()

[PATCH v6 1/2] block: Ignore close() failure in get_tmp_filename()

[Bin Meng]

The temporary file has been created and is ready for use. Checking
return value of close() does not seem useful. The file descriptor
is almost certainly closed; see close(2) under "Dealing with error
returns from close()".

Let's simply ignore close() failure here.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Bin Meng <bin.meng@windriver.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---

(no changes since v5)

Changes in v5:
- new patch: "block: Ignore close() failure in get_tmp_filename()"

 block.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/block.c b/block.c
index bc85f46eed..582c205307 100644
--- a/block.c
+++ b/block.c
@@ -886,10 +886,7 @@ int get_tmp_filename(char *filename, int size)
     if (fd < 0) {
         return -errno;
     }
-    if (close(fd) != 0) {
-        unlink(filename);
-        return -errno;
-    }
+    close(fd);
     return 0;
 #endif
 }
-- 
2.25.1

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

[Bin Meng]

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";
     }
-    if (snprintf(filename, size, "%s/vl.XXXXXX", tmpdir) >= size) {
-        return -EOVERFLOW;
-    }
-    fd = mkstemp(filename);
+#endif
+
+    filename = g_strdup_printf("%s/vl.XXXXXX", tmpdir);
+    fd = g_mkstemp(filename);
     if (fd < 0) {
-        return -errno;
+        error_setg_errno(errp, -errno, "Could not open temporary file '%s'",
+                         filename);
+        return NULL;
     }
     close(fd);
-    return 0;
-#endif
+
+    return g_steal_pointer(&filename);
 }
 
 /*
@@ -3714,8 +3721,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);
+    g_autofree char *tmp_filename = NULL;
     int64_t total_size;
     QemuOpts *opts = NULL;
     BlockDriverState *bs_snapshot = NULL;
@@ -3734,9 +3740,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;
     }
 
@@ -3770,7 +3775,6 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
 
 out:
     qobject_unref(snapshot_options);
-    g_free(tmp_filename);
     return bs_snapshot;
 }
 
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;
         goto err;
     }
 
-- 
2.25.1

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

[Bin Meng]

On Mon, Oct 10, 2022 at 12:05 PM Bin Meng <bin.meng@windriver.com> wrote:
>
> 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(-)
>

Any comments?

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

Re: [PATCH v6 1/2] block: Ignore close() failure in get_tmp_filename()

[Kevin Wolf]

Am 10.10.2022 um 06:04 hat Bin Meng geschrieben:
> The temporary file has been created and is ready for use. Checking
> return value of close() does not seem useful. The file descriptor
> is almost certainly closed; see close(2) under "Dealing with error
> returns from close()".
> 
> Let's simply ignore close() failure here.
> 
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks, series applied to the block branch.

Kevin