Am 4/14/2014 15:54, schrieb Michael Haggerty:
> The function remove_lock_file_on_signal() is used as a signal handler.
> It is not realistic to make the signal handler conform strictly to the
> C standard, which is very restrictive about what a signal handler is
> allowed to do.  But let's increase the likelihood that it will work:
> 
> The lock_file_list global variable and several fields from struct
> lock_file are used by the signal handler.  Declare those values
> "volatile" to increase the chance that the signal handler will see a
> valid object state.

Yes, it's important that the signal handler sees a valid object state, and
"volatile" can help here. But I think the reason why it helps is not
obvious, and it should be mentioned in the commit message:

It is not so much that "volatile" forces the compiler to lay down each
access of the variable coded in C in the assembly code, but more
importantly, that "volatile" disallows any re-ordering of these accesses.
Then:

- 'lock->active = 1' must be the last assignment during setup

- 'lock->active = 0' must be the first assignment during tear-down.

- Ideally, all members of struct lock_file should be "volatile".

The last point is important because the compiler is allowed to re-order
accesses to non-"volatile" variables across "volatile" accesses. I say
"ideally" because if filename were defined "volatile filename[PATH_MAX]",
strcpy() could not be used anymore. OTOH, it is unlikely that a compiler
re-orders a strcpy() call across other expressions, and we can get away
without "volatile" in the "filename" case in practice.

> Suggested-by: Johannes Sixt <j.s...@viscovery.net>

Not a big deal, but just in case you re-roll again and you do not forget:

  Johannes Sixt <j...@kdbg.org>

is preferred.

> Signed-off-by: Michael Haggerty <mhag...@alum.mit.edu>
> ---
>  cache.h    | 6 +++---
>  lockfile.c | 2 +-
>  refs.c     | 5 +++--
>  3 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/cache.h b/cache.h
> index b7af173..9019c7d 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -538,10 +538,10 @@ extern int refresh_index(struct index_state *, unsigned 
> int flags, const struct
>  #define LOCK_SUFFIX_LEN 5
>  
>  struct lock_file {
> -     struct lock_file *next;
> +     struct lock_file *volatile next;
>       volatile sig_atomic_t active;
> -     int fd;
> -     pid_t owner;
> +     volatile int fd;
> +     volatile pid_t owner;
>       char on_list;
>       char filename[PATH_MAX];
>  };
> diff --git a/lockfile.c b/lockfile.c
> index 50a0541..fce53f1 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -69,7 +69,7 @@
>   * See Documentation/api-lockfile.txt for more information.
>   */
>  
> -static struct lock_file *lock_file_list;
> +static struct lock_file *volatile lock_file_list;
>  static const char *alternate_index_output;
>  
>  static void remove_lock_file(void)
> diff --git a/refs.c b/refs.c
> index cb2f825..db8057c 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2213,15 +2213,16 @@ int commit_packed_refs(void)
>       struct packed_ref_cache *packed_ref_cache =
>               get_packed_ref_cache(&ref_cache);
>       int error = 0;
> +     int fd;
>  
>       if (!packed_ref_cache->lock)
>               die("internal error: packed-refs not locked");
>       write_or_die(packed_ref_cache->lock->fd,
>                    PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));
>  
> +     fd = packed_ref_cache->lock->fd;
>       do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),
> -                              0, write_packed_entry_fn,
> -                              &packed_ref_cache->lock->fd);
> +                              0, write_packed_entry_fn, &fd);
>       if (commit_lock_file(packed_ref_cache->lock))
>               error = -1;
>       packed_ref_cache->lock = NULL;
> 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to