[v2] block: Refactor get_tmp_filename()

Commit Message

Bin Meng Sept. 24, 2022, 11:45 a.m. UTC

From: Bin Meng <[e-mail]>

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 <[e-mail]>
---

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

Comments

Philippe Mathieu-Daudé Sept. 24, 2022, 12:29 p.m. UTC | #1

On 24/9/22 13:45, Bin Meng wrote:
> From: Bin Meng <[e-mail]>
> 
> 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 <[e-mail]>
> ---
> 
> 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;
>       }
>

Markus Armbruster Sept. 26, 2022, 10:13 a.m. UTC | #2

Bin Meng <[e-mail]> writes:

> From: Bin Meng <[e-mail]>
>
> 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 <[e-mail]>
> ---
>
> 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.

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?

Anyway, code after the patch:

   /*
    * Create a uniquely-named empty temporary file.
    * Return the actual name used upon success, otherwise NULL.
    * The called function is responsible for freeing it.
    */
   char *get_tmp_filename(void)
   {
       g_autofree char *filename = NULL;
       int fd;

       fd = g_file_open_tmp("vl.XXXXXX", &filename, NULL);
       if (fd < 0) {

As Philippe wrote, this throws away possibly useful error information.

           return NULL;
       }
       if (close(fd) != 0) {
           unlink(filename);
           return NULL;
       }
       return g_steal_pointer(&filename);
   }

Other than that, it's an improvement within the limits of a flawed
interface.

[...]

Bin Meng Sept. 26, 2022, 3:28 p.m. UTC | #3

On Mon, Sep 26, 2022 at 6:13 PM Markus Armbruster <[e-mail]> wrote:
>
> Bin Meng <[e-mail]> writes:
>
> > From: Bin Meng <[e-mail]>
> >
> > 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 <[e-mail]>
> > ---
> >
> > 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.

> 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. :(

>
> Anyway, code after the patch:
>
>    /*
>     * Create a uniquely-named empty temporary file.
>     * Return the actual name used upon success, otherwise NULL.
>     * The called function is responsible for freeing it.
>     */
>    char *get_tmp_filename(void)
>    {
>        g_autofree char *filename = NULL;
>        int fd;
>
>        fd = g_file_open_tmp("vl.XXXXXX", &filename, NULL);
>        if (fd < 0) {
>
> As Philippe wrote, this throws away possibly useful error information.
>
>            return NULL;
>        }
>        if (close(fd) != 0) {
>            unlink(filename);
>            return NULL;
>        }
>        return g_steal_pointer(&filename);
>    }
>
> Other than that, it's an improvement within the limits of a flawed
> interface.
>
> [...]
>

Regards,
Bin

Markus Armbruster Sept. 27, 2022, 6:22 a.m. UTC | #4

Bin Meng <[e-mail]> writes:

> On Mon, Sep 26, 2022 at 6:13 PM Markus Armbruster <[e-mail]> wrote:
>>
>> Bin Meng <[e-mail]> writes:
>>
>> > From: Bin Meng <[e-mail]>
>> >
>> > 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 <[e-mail]>
>> > ---
>> >
>> > 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.

[...]

Bin Meng Sept. 27, 2022, 6:29 a.m. UTC | #5

Hi Markus,

On Tue, Sep 27, 2022 at 2:22 PM Markus Armbruster <[e-mail]> wrote:
>
> Bin Meng <[e-mail]> writes:
>
> > On Mon, Sep 26, 2022 at 6:13 PM Markus Armbruster <[e-mail]> wrote:
> >>
> >> Bin Meng <[e-mail]> writes:
> >>
> >> > From: Bin Meng <[e-mail]>
> >> >
> >> > 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 <[e-mail]>
> >> > ---
> >> >
> >> > 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()?

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

Yes, big surgery to struct CreateCo::filename, and various ops in
struct CreateCo::drv that takes the filename as an argument :(

Regards,
Bin

Markus Armbruster Sept. 27, 2022, 7:24 a.m. UTC | #6

Bin Meng <[e-mail]> writes:

> Hi Markus,
>
> On Tue, Sep 27, 2022 at 2:22 PM Markus Armbruster <[e-mail]> wrote:
>>
>> Bin Meng <[e-mail]> writes:
>>
>> > On Mon, Sep 26, 2022 at 6:13 PM Markus Armbruster <[e-mail]> wrote:
>> >>
>> >> Bin Meng <[e-mail]> writes:
>> >>
>> >> > From: Bin Meng <[e-mail]>
>> >> >
>> >> > 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 <[e-mail]>
>> >> > ---
>> >> >
>> >> > 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!

[...]

Patch

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);
 }
 
 /*
@@ -3717,8 +3702,7 @@  static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
                                                    QDict *snapshot_options,
                                                    Error **errp)
 {
-    /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
-    char *tmp_filename = g_malloc0(PATH_MAX + 1);
+    char *tmp_filename = NULL;
     int64_t total_size;
     QemuOpts *opts = NULL;
     BlockDriverState *bs_snapshot = NULL;
@@ -3737,9 +3721,9 @@  static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
     }
 
     /* Create the temporary image */
-    ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
-    if (ret < 0) {
-        error_setg_errno(errp, -ret, "Could not get temporary filename");
+    tmp_filename = get_tmp_filename();
+    if (!tmp_filename) {
+        error_setg_errno(errp, errno, "Could not get temporary filename");
         goto out;
     }
 
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;
         goto err;
     }