Bin Meng <bmeng...@gmail.com> writes: > On Mon, Sep 26, 2022 at 6:13 PM Markus Armbruster <arm...@redhat.com> wrote: >> >> Bin Meng <bmeng...@gmail.com> writes: >> >> > From: Bin Meng <bin.m...@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.m...@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. >> > */ >> > -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); >> > } >> >> Oh my, what a lovely mess you're messing with! >> >> The function creates a temporary *file*, not just a filename. Makes >> sense, as creating a unique filename is inherently racy. The contract >> is clear enough ("Create a uniquely-named empty temporary file"), but >> the function name is actively misleading. > > Agreed that the name is misleading.
Care to fix that? >> Of course, creating a temporary file for the caller to (re)open is also >> racy. By the time the caller gets around to it, the filename could name >> anything. Return an open file file descriptor is a better idea. It's >> basically g_file_open_tmp(). Could we rework the two users of >> get_tmp_filename() accept a file descriptor? > > I looked at the 2 callers, and it looks like we need to do more than > these 2 callers to teach them to accept a file descriptor. :( Looks like it requires surgery to bdrv_create() at least. I'm not demanding you do that now. [...]