mesa: don't overwrite existing shader files with MESA_SHADER_CAPTURE_PATH

Submitted by Marek Olšák on April 11, 2019, 12:32 a.m.

Details

Message ID 20190411003218.30875-1-maraeo@gmail.com
State New
Headers show
Series "mesa: don't overwrite existing shader files with MESA_SHADER_CAPTURE_PATH" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Marek Olšák April 11, 2019, 12:32 a.m.
From: Marek Olšák <marek.olsak@amd.com>

---
 src/mesa/main/shaderapi.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
index 01342c04e8f..6b73e6c7e7a 100644
--- a/src/mesa/main/shaderapi.c
+++ b/src/mesa/main/shaderapi.c
@@ -1233,24 +1233,38 @@  link_program(struct gl_context *ctx, struct gl_shader_program *shProg,
          if (shProg->_LinkedShaders[stage])
             prog = shProg->_LinkedShaders[stage]->Program;
 
          _mesa_use_program(ctx, stage, shProg, prog, ctx->_Shader);
       }
    }
 
    /* Capture .shader_test files. */
    const char *capture_path = _mesa_get_shader_capture_path();
    if (shProg->Name != 0 && shProg->Name != ~0 && capture_path != NULL) {
-      FILE *file;
-      char *filename = ralloc_asprintf(NULL, "%s/%u.shader_test",
+      /* Find an unused filename. */
+      char *filename = NULL;
+      for (unsigned i = 0;; i++) {
+         if (i) {
+            filename = ralloc_asprintf(NULL, "%s/%u-%u.shader_test",
+                                       capture_path, shProg->Name, i);
+         } else {
+            filename = ralloc_asprintf(NULL, "%s/%u.shader_test",
                                        capture_path, shProg->Name);
-      file = fopen(filename, "w");
+         }
+         FILE *file = fopen(filename, "r");
+         if (!file)
+            break;
+         fclose(file);
+         ralloc_free(filename);
+      }
+
+      FILE *file = fopen(filename, "w");
       if (file) {
          fprintf(file, "[require]\nGLSL%s >= %u.%02u\n",
                  shProg->IsES ? " ES" : "",
                  shProg->data->Version / 100, shProg->data->Version % 100);
          if (shProg->SeparateShader)
             fprintf(file, "GL_ARB_separate_shader_objects\nSSO ENABLED\n");
          fprintf(file, "\n");
 
          for (unsigned i = 0; i < shProg->NumShaders; i++) {
             fprintf(file, "[%s shader]\n%s\n",

Comments

On 4/11/19 3:32 AM, Marek Olšák wrote:
> From: Marek Olšák <marek.olsak@amd.com>
> 
> ---
>   src/mesa/main/shaderapi.c | 20 +++++++++++++++++---
>   1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
> index 01342c04e8f..6b73e6c7e7a 100644
> --- a/src/mesa/main/shaderapi.c
> +++ b/src/mesa/main/shaderapi.c
> @@ -1233,24 +1233,38 @@ link_program(struct gl_context *ctx, struct gl_shader_program *shProg,
>            if (shProg->_LinkedShaders[stage])
>               prog = shProg->_LinkedShaders[stage]->Program;
>   
>            _mesa_use_program(ctx, stage, shProg, prog, ctx->_Shader);
>         }
>      }
>   
>      /* Capture .shader_test files. */
>      const char *capture_path = _mesa_get_shader_capture_path();
>      if (shProg->Name != 0 && shProg->Name != ~0 && capture_path != NULL) {
> -      FILE *file;
> -      char *filename = ralloc_asprintf(NULL, "%s/%u.shader_test",
> +      /* Find an unused filename. */
> +      char *filename = NULL;
> +      for (unsigned i = 0;; i++) {
> +         if (i) {
> +            filename = ralloc_asprintf(NULL, "%s/%u-%u.shader_test",
> +                                       capture_path, shProg->Name, i);
> +         } else {
> +            filename = ralloc_asprintf(NULL, "%s/%u.shader_test",
>                                          capture_path, shProg->Name);

How about just having the counter always there, to simplify a bit and 
have consistent filename scheme? Just a suggestion.

> -      file = fopen(filename, "w");
> +         }
> +         FILE *file = fopen(filename, "r");
> +         if (!file)
> +            break;

I'm surprised we don't have some helper like 'util_path_exists' but this 
works, I guess then we should have 'util_path_isdir|isfile' and others 
as well.

With or without the suggestion;
Reviewed-by: Tapani Pälli <tapani.palli@intel.com>

> +         fclose(file);
> +         ralloc_free(filename);
> +      }
> +
> +      FILE *file = fopen(filename, "w");
>         if (file) {
>            fprintf(file, "[require]\nGLSL%s >= %u.%02u\n",
>                    shProg->IsES ? " ES" : "",
>                    shProg->data->Version / 100, shProg->data->Version % 100);
>            if (shProg->SeparateShader)
>               fprintf(file, "GL_ARB_separate_shader_objects\nSSO ENABLED\n");
>            fprintf(file, "\n");
>   
>            for (unsigned i = 0; i < shProg->NumShaders; i++) {
>               fprintf(file, "[%s shader]\n%s\n",
>

On Friday, 2019-04-12 11:00:56 -0400, Marek Olšák wrote:
> On Thu, Apr 11, 2019 at 2:53 AM Tapani Pälli <tapani.palli@intel.com> wrote:
> > On 4/11/19 3:32 AM, Marek Olšák wrote:
> > > -      file = fopen(filename, "w");
> > > +         }
> > > +         FILE *file = fopen(filename, "r");
> > > +         if (!file)
> > > +            break;
> >
> > I'm surprised we don't have some helper like 'util_path_exists' but this
> > works, I guess then we should have 'util_path_isdir|isfile' and others
> > as well.
> >
> 
> There is no standard API for checking whether a file exists. fopen is the
> only standard way to do it.

What about `access(filename, F_OK)` ?

On Friday, 2019-04-12 11:50:11 -0400, Marek Olšák wrote:
> On Fri, Apr 12, 2019 at 11:41 AM Eric Engestrom <eric.engestrom@intel.com>
> wrote:
> 
> > On Friday, 2019-04-12 11:00:56 -0400, Marek Olšák wrote:
> > > On Thu, Apr 11, 2019 at 2:53 AM Tapani Pälli <tapani.palli@intel.com>
> > wrote:
> > > > On 4/11/19 3:32 AM, Marek Olšák wrote:
> > > > > -      file = fopen(filename, "w");
> > > > > +         }
> > > > > +         FILE *file = fopen(filename, "r");
> > > > > +         if (!file)
> > > > > +            break;
> > > >
> > > > I'm surprised we don't have some helper like 'util_path_exists' but
> > this
> > > > works, I guess then we should have 'util_path_isdir|isfile' and others
> > > > as well.
> > > >
> > >
> > > There is no standard API for checking whether a file exists. fopen is the
> > > only standard way to do it.
> >
> > What about `access(filename, F_OK)` ?
> >
> 
> That's not standard C API.

Right, that's POSIX and that can't be used on windows, which this file
can be built on.
My bad :)