[v5,2/2] block: Refactor get_tmp_filename()

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 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 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                          | 45 ++++++++++++--------------------
 block/vvfat.c                    |  7 +++--
 3 files changed, 20 insertions(+), 34 deletions(-)

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 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>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Am 28.09.2022 um 16:41 hat Bin Meng geschrieben:
> 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 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 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                          | 45 ++++++++++++--------------------
>  block/vvfat.c                    |  7 +++--
>  3 files changed, 20 insertions(+), 34 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..bd3006d85d 100644
> --- a/block.c
> +++ b/block.c
> @@ -860,35 +860,25 @@ 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)

We're using different prefixes on Windows and on Linux. This patch
unifies both paths to use the Linux name. Nobody should rely on the name
of temporary files, so there is hope it won't break anything.

> -            ? 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);

This implicitly reverts commit 69bef7931e8, g_file_open_tmp() uses /tmp
as the default instead of /var/tmp as this function does before the
patch.

This is probably not a good idea, we should keep the /var/tmp default.

But if we did want to do this, it's definitely a change in behaviour
that should be mentioned in the commit message at least.

Kevin

Hi Kevin,

On Fri, Sep 30, 2022 at 6:13 PM Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 28.09.2022 um 16:41 hat Bin Meng geschrieben:
> > 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 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 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                          | 45 ++++++++++++--------------------
> >  block/vvfat.c                    |  7 +++--
> >  3 files changed, 20 insertions(+), 34 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..bd3006d85d 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -860,35 +860,25 @@ 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)
>
> We're using different prefixes on Windows and on Linux. This patch
> unifies both paths to use the Linux name. Nobody should rely on the name
> of temporary files, so there is hope it won't break anything.
>
> > -            ? 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);
>
> This implicitly reverts commit 69bef7931e8, g_file_open_tmp() uses /tmp
> as the default instead of /var/tmp as this function does before the
> patch.

Oops, thanks for the pointer. Commit message of 69bef7931e8 does not
explicitely explain why to change from /tmp to /var/tmp. Is that
because QEMU block codes write a huge size of data to this file in
/tmp?

> This is probably not a good idea, we should keep the /var/tmp default.
>
> But if we did want to do this, it's definitely a change in behaviour
> that should be mentioned in the commit message at least.
>

If we have to keep /var/tmp, how about this?

diff --git a/block.c b/block.c
index bd3006d85d..d964ceaeac 100644
--- a/block.c
+++ b/block.c
@@ -24,6 +24,7 @@
  */

 #include "qemu/osdep.h"
+#include <glib/gstdio.h>
 #include "block/trace.h"
 #include "block/block_int.h"
 #include "block/blockjob.h"
@@ -878,7 +879,20 @@ char *create_tmp_file(Error **errp)
         return NULL;
     }
     close(fd);
+#ifdef _WIN32
     return g_steal_pointer(&name);
+#else
+    g_autofree char *base = NULL;
+    char *newname;
+
+    base = g_path_get_basename(name);
+    newname = g_strdup_printf("/var/tmp/%s", base);
+    if (g_rename(name, newname) < 0) {
+        error_setg_errno(errp, -errno, "Could not create file");
+        return NULL;
+    }
+    return newname;
+#endif
 }

Regards,
Bin

Am 02.10.2022 um 11:38 hat Bin Meng geschrieben:
> Hi Kevin,
> 
> On Fri, Sep 30, 2022 at 6:13 PM Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > Am 28.09.2022 um 16:41 hat Bin Meng geschrieben:
> > > 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 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 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                          | 45 ++++++++++++--------------------
> > >  block/vvfat.c                    |  7 +++--
> > >  3 files changed, 20 insertions(+), 34 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..bd3006d85d 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -860,35 +860,25 @@ 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)
> >
> > We're using different prefixes on Windows and on Linux. This patch
> > unifies both paths to use the Linux name. Nobody should rely on the name
> > of temporary files, so there is hope it won't break anything.
> >
> > > -            ? 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);
> >
> > This implicitly reverts commit 69bef7931e8, g_file_open_tmp() uses /tmp
> > as the default instead of /var/tmp as this function does before the
> > patch.
> 
> Oops, thanks for the pointer. Commit message of 69bef7931e8 does not
> explicitely explain why to change from /tmp to /var/tmp. Is that
> because QEMU block codes write a huge size of data to this file in
> /tmp?

Yes, this function is used to create temporary disk images (like for
-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.

> > This is probably not a good idea, we should keep the /var/tmp default.
> >
> > But if we did want to do this, it's definitely a change in behaviour
> > that should be mentioned in the commit message at least.
> >
> 
> If we have to keep /var/tmp, how about this?
> 
> diff --git a/block.c b/block.c
> index bd3006d85d..d964ceaeac 100644
> --- a/block.c
> +++ b/block.c
> @@ -24,6 +24,7 @@
>   */
> 
>  #include "qemu/osdep.h"
> +#include <glib/gstdio.h>
>  #include "block/trace.h"
>  #include "block/block_int.h"
>  #include "block/blockjob.h"
> @@ -878,7 +879,20 @@ char *create_tmp_file(Error **errp)
>          return NULL;
>      }
>      close(fd);
> +#ifdef _WIN32
>      return g_steal_pointer(&name);
> +#else
> +    g_autofree char *base = NULL;
> +    char *newname;
> +
> +    base = g_path_get_basename(name);
> +    newname = g_strdup_printf("/var/tmp/%s", base);
> +    if (g_rename(name, newname) < 0) {
> +        error_setg_errno(errp, -errno, "Could not create file");
> +        return NULL;
> +    }
> +    return newname;
> +#endif
>  }

I don't think this works correctly. It first finds an unused filename in
/tmp, and then uses that filename in /var/tmp without checking if it's
already in use there. It's also not simpler than the original code.

We should probably stick with g_mkstemp(), which however can be shared
between Windows and Linux. Just finding the directory is still going to
use an #ifdef.

But the more important point of your patch was the external interface of
the function anyway, and you can keep that as it is.

Kevin