Bin Meng <bmeng...@gmail.com> writes:
> Hi Markus,
>
> On Tue, Sep 27, 2022 at 2:22 PM Markus Armbruster <arm...@redhat.com> wrote:
>>
>> 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?
>
> How about create_tmp_file()?
Works for me!
[...]
