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

[Timothy Arceri]

On 22/05/18 04:40, Benedikt Schemmer wrote:
> Ok, small update. Please ignore this rfc.
> I finally got appveyor to build my repo.
> 
> There are at least these dependencies that would need to addressed (for anybody wanting tor try):
> 
> #include <dlfcn.h> // can be eliminated sometimes by replacing with __TIMESTAMP__ or maybe better __DATE__ __TIME__
> #include <sys/mman.h>
> #include <unistd.h>
> #include <pwd.h>
> #include <dirent.h>
> #include "zlib.h" // complains about redefinition vsnprintf (I used version 1.23)
> 
> 
> So for now: code duplication it is ;)
> 
> 
> appveyor has mingw available, maybe I will give that a try sometime
> https://www.appveyor.com/docs/build-environment/
> 
> and they even have an ubuntu image available
> https://www.appveyor.com/docs/getting-started-with-appveyor-for-linux/
> 
> wonder why that isnt being used instead of msvc 2015 with llvm 3.3.1?

Because VMWare uses that setup to builds its drivers, appveyor was setup 
to avoid breaking those builds on windows.


> 
> 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? 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,
>>>