[Mesa-dev] [RFC PATCH] Replace an flock with a random filename to evade some very ugly system dependent code

[Benedikt Schemmer]

Am 21.05.2018 um 01:19 schrieb Timothy Arceri:
> 
> 
> On 20/05/18 22:16, Benedikt Schemmer wrote:
>> There is exactly one flock in mesa and it caused mesa not to build
>> on windows when shader cache was enabled.
>>
>> It should be possible to revert 9f8dc3bf03ec825bae7041858dda6ca2e9a34363
>> "utils: build sha1/disk cache only with Android/Autoconf" currently
>> guarding the offending code with ENABLE_SHADER_CACHE
>>
>> This would allow shader cache to work on windows I think.
> 
> NAK. rand() is not thread safe.
> 
> Why are you trying to make this build on windows? 

Appveyor
https://ci.appveyor.com/project/mesa3d/mesa/build/3186

was the reason why large parts are guarded by ENABLE_SHADER_CACHE
which prevents me from using a simple function in the patch I mentioned

I can of course circumvent that by just duplicating the code I
need.

It just seems cleaner to have no system dependent code here.
But you're right: its quite convoluted and every time I think I got it
something else pops up ;)

There are also other dependencies (dlfcn.h) that need to be
removed. I already have, at least to some degree. I dont know how to eliminate
the runtime check for the llvm build though, but that is only used by amd code
so I moved it to ac_llvm_util.h for the moment which seems to work.

si_pipe.c somewhere around line 800 now looks like:
---
	if (ac_get_function_timestamp(LLVMInitializeAMDGPUTargetInfo,
					      &llvm_timestamp)) {
		res = asprintf(&timestamp_str, "%s_%u",
			       __TIMESTAMP__, llvm_timestamp);
	}
---

>There is no use case for this on windows currently so it won't even be tested. There are also other calls such as getuid() etc that >will fail on windows.
> 
> If someone want this to work on windows they should just write windows specific paths IMO.
> 
>>
>> I dont have a test system with windows though.
>> This builds on linux and is tested with Deus Ex:MD and Metro 2033 Redux
>> both with cold shader cache.
>>
>> Really
>> Fixes: d1efa09d342bff3e5def2978a0bef748d74f9c82
>>
>> CC: Tapani Pälli <tapani.palli at intel.com>
>> CC: "Marek Olšák" <maraeo at gmail.com>
>> CC: Emil Velikov <emil.l.velikov at gmail.com>
>> CC: Timothy Arceri <tarceri at itsqueeze.com>
>> CC: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>> ---
>> This enables the patch
>> [PATCH 1/3] mesa/main/shaderapi: Use generate_sha1() unconditionally
>>
>>   src/util/disk_cache.c | 48 +++++++++++++++++++++++++++++++++++-------------
>>   1 file changed, 35 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/util/disk_cache.c b/src/util/disk_cache.c
>> index 4a762eff20..ca47bb15fb 100644
>> --- a/src/util/disk_cache.c
>> +++ b/src/util/disk_cache.c
>> @@ -28,7 +28,6 @@
>>   #include <string.h>
>>   #include <stdlib.h>
>>   #include <stdio.h>
>> -#include <sys/file.h>
>>   #include <sys/types.h>
>>   #include <sys/stat.h>
>>   #include <sys/mman.h>
>> @@ -848,6 +847,29 @@ struct cache_entry_file_data {
>>      uint32_t uncompressed_size;
>>   };
>>   +static char *
>> +generate_random_string(int length) {
>> +   static const char a[] = "0123456789abcdef";
>> +
>> +   if (length > 16)
>> +      return NULL;
>> +
>> +   char buf[16];
>> +   char *rndstr;
>> +
>> +   for (int i = 0; i < length - 1; ++i) {
>> +       // assign a random element from the lookup table
>> +       buf[i] = a[rand() % (sizeof(a) - 1)];
>> +   }
>> +
>> +   buf[length - 1] = 0;
>> +
>> +   if (asprintf(&rndstr, "%s", buf) == -1)
>> +      return NULL;
>> +
>> +   return rndstr;
>> +}
>> +
>>   static void
>>   cache_put(void *job, int thread_index)
>>   {
>> @@ -855,7 +877,7 @@ cache_put(void *job, int thread_index)
>>        int fd = -1, fd_final = -1, err, ret;
>>      unsigned i = 0;
>> -   char *filename = NULL, *filename_tmp = NULL;
>> +   char *filename = NULL, *filename_tmp = NULL, *random = NULL;
>>      struct disk_cache_put_job *dc_job = (struct disk_cache_put_job *) job;
>>        filename = get_cache_file(dc_job->cache, dc_job->key);
>> @@ -873,7 +895,16 @@ cache_put(void *job, int thread_index)
>>       * final destination filename, (to prevent any readers from seeing
>>       * a partially written file).
>>       */
>> -   if (asprintf(&filename_tmp, "%s.tmp", filename) == -1)
>> +
>> +   /* This next part used to be an flock(), which would prevent windows systems
>> +    * to build. 4 hex characters should be enough to prevent filename race
>> +    * conditions for now.
>> +   */
>> +   random = generate_random_string(4);
>> +   if (random == NULL)
>> +      goto done;
>> +
>> +   if (asprintf(&filename_tmp, "%s_%s.tmp", filename, random) == -1)
>>         goto done;
>>        fd = open(filename_tmp, O_WRONLY | O_CLOEXEC | O_CREAT, 0644);
>> @@ -890,16 +921,7 @@ cache_put(void *job, int thread_index)
>>            goto done;
>>      }
>>   -   /* With the temporary file open, we take an exclusive flock on
>> -    * it. If the flock fails, then another process still has the file
>> -    * open with the flock held. So just let that file be responsible
>> -    * for writing the file.
>> -    */
>> -   err = flock(fd, LOCK_EX | LOCK_NB);
>> -   if (err == -1)
>> -      goto done;
>> -
>> -   /* Now that we have the lock on the open temporary file, we can
>> +   /* Now that we have the open temporary file, we can
>>       * check to see if the destination file already exists. If so,
>>       * another process won the race between when we saw that the file
>>       * didn't exist and now. In this case, we don't do anything more,
>>