qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Philippe Mathieu-Daudé
Subject: Re: [PATCH v2] block: Refactor get_tmp_filename()
Date: Sat, 24 Sep 2022 14:29:35 +0200
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.13.1

On 24/9/22 13:45, 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:

     char *get_tmp_filename(void)

and use g_file_open_tmp() for a consistent implementation.

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

Changes in v2:
- Use g_autofree and g_steal_pointer

  include/block/block_int-common.h |  2 +-
  block.c                          | 42 ++++++++++----------------------
  block/vvfat.c                    |  8 +++---
  3 files changed, 18 insertions(+), 34 deletions(-)

diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 8947abab76..ea69a9349c 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 *get_tmp_filename(void);
  void bdrv_parse_filename_strip_prefix(const char *filename, const char 
*prefix,
                                        QDict *options);
diff --git a/block.c b/block.c
index bc85f46eed..4e7a795566 100644
--- a/block.c
+++ b/block.c
@@ -860,38 +860,23 @@ 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 name used upon success, otherwise NULL.
+ * The called function is responsible for freeing it.

s/called/caller/.

   */
-int get_tmp_filename(char *filename, int size)
+char *get_tmp_filename(void)
  {
-#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 *filename = 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", &filename, NULL);
      if (fd < 0) {
-        return -errno;
+        return NULL;
      }
      if (close(fd) != 0) {
          unlink(filename);
-        return -errno;
+        return NULL;
      }
-    return 0;
-#endif
+    return g_steal_pointer(&filename);
  }

diff --git a/block/vvfat.c b/block/vvfat.c
index d6dd919683..135fafb166 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3146,10 +3146,10 @@ 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 = get_tmp_filename();
+    if (!s->qcow_filename) {
+        error_setg_errno(errp, errno, "can't create temporary file");
+        ret = -errno;

I don't think we can reuse errno here. Wouldn't it be better to pass a Error* to get_tmp_filename, and use the error parameter to g_file_open_tmp()? That would match our Error* style use.

          goto err;
      }




reply via email to

[Prev in Thread] Current Thread [Next in Thread]