[PATCH] block: honor $TMPDIR in create_tmp_file()

[PATCH] block: honor $TMPDIR in create_tmp_file()

[Michael Tokarev]

Current code uses g_get_tmp_dir() to get temporary directory,
and if the returned *value* is "/tmp", the code changes it to
"/var/tmp".  This is wrong, - we should use "/var/tmp" only if
$TMPDIR is not set, not if it is set to some specific value.
In particular, the code doesn't let us to use TMPDIR=/tmp.

Fix this by using g_get_tmp_dir() only on windows platform as
before, and open-code $TMPDIR usage on everything else.
g_get_tmp_dir() checks $TMP and $TEMP too, but these variables
are windows-specific and should not be used on *nix.

Fixes: 69fbfff95e84 "block: Refactor get_tmp_filename()"
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1626
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 block.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 8848e9a7ed..f86fc9db35 100644
--- a/block.c
+++ b/block.c
@@ -853,8 +853,6 @@ char *create_tmp_file(Error **errp)
     const char *tmpdir;
     g_autofree char *filename = NULL;
 
-    tmpdir = g_get_tmp_dir();
-#ifndef _WIN32
     /*
      * See commit 69bef79 ("block: use /var/tmp instead of /tmp for -snapshot")
      *
@@ -862,7 +860,12 @@ char *create_tmp_file(Error **errp)
      * 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.
      */
-    if (!g_strcmp0(tmpdir, "/tmp")) {
+
+#ifdef _WIN32
+    tmpdir = g_get_tmp_dir();
+#else
+    tmpdir = getenv("TMPDIR");
+    if (!tmpdir) {
         tmpdir = "/var/tmp";
     }
 #endif
-- 
2.47.2

Re: [PATCH] block: honor $TMPDIR in create_tmp_file()

[Richard Henderson]

On 8/31/25 21:48, Michael Tokarev wrote:
>       /*
>        * See commit 69bef79 ("block: use /var/tmp instead of /tmp for -snapshot")
>        *
> @@ -862,7 +860,12 @@ char *create_tmp_file(Error **errp)
>        * 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 going to cause other failures, per the tmpfs reason given in the comment.

r~

Re: [PATCH] block: honor $TMPDIR in create_tmp_file()

[Michael Tokarev]

On 01.09.2025 01:39, Richard Henderson wrote:
> On 8/31/25 21:48, Michael Tokarev wrote:
>>       /*
>>        * See commit 69bef79 ("block: use /var/tmp instead of /tmp for 
>> -snapshot")
>>        *
>> @@ -862,7 +860,12 @@ char *create_tmp_file(Error **errp)
>>        * 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 going to cause other failures, per the tmpfs reason given in the 
> comment.

This is the comment:

      * This function is used to create temporary disk images (like 
-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.

It does not give reasons for "other failures".

Are you saying the user, who decided to explicitly specify TMPDIR,
is wrong, and qemu should use /var/tmp which does not even exist
(see the bug report this patch is fixing)?

The original code (before 69fbfff95e84) was correct.  Current code
is not.  My change fixes current wrong code.

Thanks,

/mjt

Re: [PATCH] block: honor $TMPDIR in create_tmp_file()

[Richard Henderson]

On 9/1/25 15:31, Michael Tokarev wrote:
> On 01.09.2025 01:39, Richard Henderson wrote:
>> On 8/31/25 21:48, Michael Tokarev wrote:
>>>       /*
>>>        * See commit 69bef79 ("block: use /var/tmp instead of /tmp for -snapshot")
>>>        *
>>> @@ -862,7 +860,12 @@ char *create_tmp_file(Error **errp)
>>>        * 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 going to cause other failures, per the tmpfs reason given in the comment.
> 
> This is the comment:
> 
>       * This function is used to create temporary disk images (like -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.
> 
> It does not give reasons for "other failures".

It does gloss over the implications of "tmpfs".  But off the top of my head:

(1) tmpfs is generally smaller than any other disk-based tmpdir,
     so ENOSPC is easier to trigger, and
(2) tmpfs does not support several O_FOO.

> 
> Are you saying the user, who decided to explicitly specify TMPDIR,
> is wrong, and qemu should use /var/tmp which does not even exist
> (see the bug report this patch is fixing)?

I think this is a very strong interpretation since the non-existence of /var/tmp is, IMO, 
new and rare.  /var/tmp most certainly *does* exist with all of the major distros.


> The original code (before 69fbfff95e84) was correct.  Current code
> is not.  My change fixes current wrong code.

If this goes in as-is, you are on the hook for any reported testsuite regression.


r~

Re: [PATCH] block: honor $TMPDIR in create_tmp_file()

[Michael Tokarev]

On 02.09.2025 16:25, Richard Henderson wrote:
> On 9/1/25 15:31, Michael Tokarev wrote:

>> It does not give reasons for "other failures".
> 
> It does gloss over the implications of "tmpfs".  But off the top of my 
> head:
> 
> (1) tmpfs is generally smaller than any other disk-based tmpdir,
>      so ENOSPC is easier to trigger, and
> (2) tmpfs does not support several O_FOO.

O_DIRECT, yes.  I tried to fix this in mid-2000s, but weren't able to
convince Linus at the time.  Now, in current kernels, O_DIRECT is
finally supported, so this is a non-issue today.

But all this is beyond the point.  The point is that we must not
pretend we know better than the user who set $TMPDIR before invoking
qemu, and override user's choice silently.

Please note: in my case, /var/tmp is the default value, when the user
didn't specify anything.  If the user did specify $TMPDIR, it is what
we should use.

If we want some other mechanism to set the temp directory, we should
(or might) implement it.  But in the new mechanism, we, again, should
not silently substitute /var/tmp for /tmp, as we currently do.  This
is just plain wrong.

>> Are you saying the user, who decided to explicitly specify TMPDIR,
>> is wrong, and qemu should use /var/tmp which does not even exist
>> (see the bug report this patch is fixing)?
> 
> I think this is a very strong interpretation since the non-existence 
> of /var/tmp is, IMO, new and rare.  /var/tmp most certainly *does* exist 
> with all of the major distros.

Please see the referenced issue,
https://gitlab.com/qemu-project/qemu/-/issues/1626 , -
myself, I'm not a fan of such systems which violate basic principles
(and I already closed that bug report in the past).

But once again, this is not the point.  The point is that we override
user's choice by our "better" value.  *This* is what I'm trying to fix,
-- we should not pretend we know better than the user.

>> The original code (before 69fbfff95e84) was correct.  Current code
>> is not.  My change fixes current wrong code.
> 
> If this goes in as-is, you are on the hook for any reported testsuite 
> regression.

Sure.  If there will be regressions, it means we didn't honor our own
choices somewhere, - when we did set TMPDIR but our qemu being tested
used some different place.

Thanks,

/mjt