[PATCH]Upstream: delete temp file when error in ngx_http_upstream_store().

[flygoast]

Thanks for reply.
When ngx_ext_rename_file() return NGX_ERROR, it should has called ngx_delete_file().
So I think maybe also should change "ext.delete_file = 1;" to "ext.delete_file = 0;".




At 2014-07-08 00:22:23,"Maxim Dounin" <mdounin at mdounin.ru> wrote:

>Hello!
>
>On Mon, Jul 07, 2014 at 05:19:45PM +0800, flygoast wrote:
>
>> # HG changeset patch
>> # User FengGu <flygoast at 126.com>
>> # Date 1404723967 -28800
>> #      Mon Jul 07 17:06:07 2014 +0800
>> # Node ID d1d597fbf6d8e08059a308d8900e5f90def5377a
>> # Parent  a680bf4dddd5c4b106419e3dfb0264815c401275
>> Upstream: delete temp file when error in ngx_http_upstream_store().
>> 
>> 
>> Previously, didn't process the case when ngx_http_map_uri_to_path() returned
>> NULL. And when error occured, didn't delete the temp file.
>> 
>> 
>> diff -r a680bf4dddd5 -r d1d597fbf6d8 src/http/ngx_http_upstream.c
>> --- a/src/http/ngx_http_upstream.cSat Jul 05 23:29:47 2014 +0400
>> +++ b/src/http/ngx_http_upstream.cMon Jul 07 17:06:07 2014 +0800
>> @@ -3375,14 +3375,16 @@
>>  
>>      if (u->conf->store_lengths == NULL) {
>>  
>> -        ngx_http_map_uri_to_path(r, &path, &root, 0);
>> +        if (ngx_http_map_uri_to_path(r, &path, &root, 0) == NULL) {
>> +            goto error;
>> +        }
>>  
>>      } else {
>>          if (ngx_http_script_run(r, &path, u->conf->store_lengths->elts, 0,
>>                                  u->conf->store_values->elts)
>>              == NULL)
>>          {
>> -            return;
>> +            goto error;
>>          }
>>      }
>>  
>> @@ -3393,6 +3395,16 @@
>>                     tf->file.name.data, path.data);
>>  
>>      (void) ngx_ext_rename_file(&tf->file.name, &path, &ext);
>> +
>> +    return;
>> +
>> +error:
>> +
>> +    if (ngx_delete_file(tf->file.name.data) == NGX_FILE_ERROR) {
>> +        ngx_log_error(NGX_LOG_CRIT, r->connection->log, ngx_errno,
>> +                      ngx_delete_file_n " \"%s\" failed",
>> +                      tf->file.name.data);
>> +    }
>>  }
>>  
>
>Thanks for noting this.  What do you think about the following patch 
>instead?
>
>There is a code in ngx_http_upstream_finalize_request() which 
>normally removes temporary files after errors with proxy_store, 
>but in this particular case it won't be triggered due to u->store 
>set to 0 even in case of errors.  With this patch, u->store is set 
>to 0 only in if there are no errors, and thus temporary files are 
>properly removed in case of errors in ngx_http_upstream_store(). 
>
>diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
>--- a/src/http/ngx_http_upstream.c
>+++ b/src/http/ngx_http_upstream.c
>@@ -3253,7 +3253,6 @@ ngx_http_upstream_process_request(ngx_ht
>                         || u->headers_in.content_length_n == tf->offset))
>                 {
>                     ngx_http_upstream_store(r, u);
>-                    u->store = 0;
>                 }
>             }
>         }
>@@ -3375,7 +3374,9 @@ ngx_http_upstream_store(ngx_http_request
> 
>     if (u->conf->store_lengths == NULL) {
> 
>-        ngx_http_map_uri_to_path(r, &path, &root, 0);
>+        if (ngx_http_map_uri_to_path(r, &path, &root, 0) == NULL) {
>+            return;
>+        }
> 
>     } else {
>         if (ngx_http_script_run(r, &path, u->conf->store_lengths->elts, 0,
>@@ -3392,7 +3393,11 @@ ngx_http_upstream_store(ngx_http_request
>                    "upstream stores \"%s\" to \"%s\"",
>                    tf->file.name.data, path.data);
> 
>-    (void) ngx_ext_rename_file(&tf->file.name, &path, &ext);
>+    if (ngx_ext_rename_file(&tf->file.name, &path, &ext) != NGX_OK) {
>+        return;
>+    }
>+
>+    u->store = 0;
> }
> 
> 
>-- 
>Maxim Dounin
>http://nginx.org/
>
>_______________________________________________
>nginx-devel mailing list
>nginx-devel at nginx.org
>http://mailman.nginx.org/mailman/listinfo/nginx-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20140708/795c36a5/attachment.html>