[v1] tests/qtest: Enable running qtest on Windows

[PATCH 03/51] block: Unify the get_tmp_filename() implementation

Posted by Bin Meng 3 years, 5 months ago

From: Bin Meng <bin.meng@windriver.com>

At present get_tmp_filename() has platform specific implementations
to get the directory to use for temporary files. Switch over to use
g_get_tmp_dir() which works on all supported platforms.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
---

 block.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/block.c b/block.c
index bc85f46eed..d06df47f72 100644
--- a/block.c
+++ b/block.c
@@ -864,21 +864,10 @@ int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo)
  */
 int get_tmp_filename(char *filename, int size)
 {
-#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
     int fd;
     const char *tmpdir;
-    tmpdir = getenv("TMPDIR");
-    if (!tmpdir) {
-        tmpdir = "/var/tmp";
-    }
+    tmpdir = g_get_tmp_dir();
+
     if (snprintf(filename, size, "%s/vl.XXXXXX", tmpdir) >= size) {
         return -EOVERFLOW;
     }
@@ -891,7 +880,6 @@ int get_tmp_filename(char *filename, int size)
         return -errno;
     }
     return 0;
-#endif
 }
 
 /*
-- 
2.34.1

Re: [PATCH 03/51] block: Unify the get_tmp_filename() implementation

Posted by Marc-André Lureau 3 years, 5 months ago

Hi Bin

On Wed, Aug 24, 2022 at 1:42 PM Bin Meng <bmeng.cn@gmail.com> wrote:

> From: Bin Meng <bin.meng@windriver.com>
>
> At present get_tmp_filename() has platform specific implementations
> to get the directory to use for temporary files. Switch over to use
> g_get_tmp_dir() which works on all supported platforms.
>
>
It "works" quite differently though. Is this patch really necessary here?

If yes, please explain why.

If not, I suggest you drop optional / rfc / "nice to have" patches from the
series. It will help to get it merged faster.

thanks



> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
>
>  block.c | 16 ++--------------
>  1 file changed, 2 insertions(+), 14 deletions(-)
>
> diff --git a/block.c b/block.c
> index bc85f46eed..d06df47f72 100644
> --- a/block.c
> +++ b/block.c
> @@ -864,21 +864,10 @@ int bdrv_probe_geometry(BlockDriverState *bs,
> HDGeometry *geo)
>   */
>  int get_tmp_filename(char *filename, int size)
>  {
> -#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
>      int fd;
>      const char *tmpdir;
> -    tmpdir = getenv("TMPDIR");
> -    if (!tmpdir) {
> -        tmpdir = "/var/tmp";
> -    }
> +    tmpdir = g_get_tmp_dir();
> +
>      if (snprintf(filename, size, "%s/vl.XXXXXX", tmpdir) >= size) {
>          return -EOVERFLOW;
>      }
> @@ -891,7 +880,6 @@ int get_tmp_filename(char *filename, int size)
>          return -errno;
>      }
>      return 0;
> -#endif
>  }
>
>  /*
> --
> 2.34.1
>
>
>

-- 
Marc-André Lureau

Re: [PATCH 03/51] block: Unify the get_tmp_filename() implementation

Posted by Bin Meng 3 years, 5 months ago

Hi Marc-André,

On Wed, Aug 31, 2022 at 8:54 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi Bin
>
> On Wed, Aug 24, 2022 at 1:42 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>
>> From: Bin Meng <bin.meng@windriver.com>
>>
>> At present get_tmp_filename() has platform specific implementations
>> to get the directory to use for temporary files. Switch over to use
>> g_get_tmp_dir() which works on all supported platforms.
>>
>
> It "works" quite differently though. Is this patch really necessary here?

Without this patch the qtest cases builds on Windows do not have any
problem. So it is optional. I put it in the same series as it has the
same context of using hardcoded /tmp directory name.

>
> If yes, please explain why.
>
> If not, I suggest you drop optional / rfc / "nice to have" patches from the series. It will help to get it merged faster.

I can drop this single patch and send another single patch if this is
the desired practice.

>
> thanks

Regards,
Bin

Re: [PATCH 03/51] block: Unify the get_tmp_filename() implementation

Posted by Daniel P. Berrangé 3 years, 5 months ago

On Wed, Aug 31, 2022 at 04:54:41PM +0400, Marc-André Lureau wrote:
> Hi Bin
> 
> On Wed, Aug 24, 2022 at 1:42 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> 
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > At present get_tmp_filename() has platform specific implementations
> > to get the directory to use for temporary files. Switch over to use
> > g_get_tmp_dir() which works on all supported platforms.
> >
> >
> It "works" quite differently though. Is this patch really necessary here?
> 
> If yes, please explain why.
> 
> If not, I suggest you drop optional / rfc / "nice to have" patches from the
> series. It will help to get it merged faster.
> 
> thanks
> 
> 
> 
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > ---
> >
> >  block.c | 16 ++--------------
> >  1 file changed, 2 insertions(+), 14 deletions(-)
> >
> > diff --git a/block.c b/block.c
> > index bc85f46eed..d06df47f72 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -864,21 +864,10 @@ int bdrv_probe_geometry(BlockDriverState *bs,
> > HDGeometry *geo)
> >   */
> >  int get_tmp_filename(char *filename, int size)
> >  {
> > -#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
> >      int fd;
> >      const char *tmpdir;
> > -    tmpdir = getenv("TMPDIR");
> > -    if (!tmpdir) {
> > -        tmpdir = "/var/tmp";
> > -    }
> > +    tmpdir = g_get_tmp_dir();
> > +
> >      if (snprintf(filename, size, "%s/vl.XXXXXX", tmpdir) >= size) {
> >          return -EOVERFLOW;
> >      }

I know this is pre-existing, but this use of snprintf is really
undesirable and should be culled while we're touching this code.

There are only two callers of get_tmp_filename and they're
inconsistent too

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

either the comment is wrong and adding "+1" to PATH_MAX is not
required, or the second caller is wrong on Windows. This may even
be totally irrelevant with the switch to g_get_tmp_dir. Whatever
the answer is, at least 1 of the callers needs updating.

It would be way better if the method signature was

  char *get_tmp_filename(void);

and we uses g_strdup_printf() instead of snprintf so the corret
size is allocated right away, removing the question about whether
we need PATH_MAX or PATH_MAX + 1 entirely.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|