[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v5 2/2] block: Refactor get_tmp_filename()
From: |
Kevin Wolf |
Subject: |
Re: [PATCH v5 2/2] block: Refactor get_tmp_filename() |
Date: |
Tue, 4 Oct 2022 10:48:05 +0200 |
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