Re: [PATCH v2] core: use env variable instead of config var to turn on logging pack access

[Duy Nguyen <pclouds@gmail.com>]

On Tue, Jun 4, 2013 at 1:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>>  > Have you ever tested this?
>>  >
>>  > Once log_pack_access goes to NULL (e.g. when it sees the empty
>>  > string it was initialized to), this new test will happily
>>  > dereference NULL.
>>
>>  My bad. I did test when GIT_TRACE_PACK_ACCESS was set, not when it
>>  was set to an empty string. All cases tested now.
>>
>> @@ -1956,6 +1958,14 @@ static void write_pack_access_log(struct packed_git *p, off_t obj_offset)
>>  {
>>       static FILE *log_file;
>>
>> +     if (!*log_pack_access) {
>
> The first time, we will see the static empty string and come into
> this block...
>
>> +             log_pack_access = getenv("GIT_TRACE_PACK_ACCESS");
>> +             if (log_pack_access && !*log_pack_access)
>> +                     log_pack_access = NULL;
>> +             if (!log_pack_access)
>> +                     return;
>> +     }
>
> This may
>
>  (1) not find the env-var, in which case log_pack_access becomes
>      NULL here, and the function returns.
>
>  (2) find the env-var to be an empty string, in which case
>      log_pack_access becomes NULL in the next statement, and the
>      function returns.
>
>  (3) find the env-var to be a non-empty string, and the function
>      does not return.
>
> And the next time around, (3) above may work fine; the first if()
> will fail and we do not come in.  But in either (1) or (2), don't
> you keep checking the environment every time you come here?
>
> Oh, no, it is worse than that.  Either case you set the variable to
> NULL, so the very first "if (!*log_pack_access)" would dereference
> NULL.

You found it out already. The only caller does this

if (log_pack_access) write_pack_access_log(p, obj_offset);

so in (1) and (2), write_pack_access_log() will never be called again.
Originally I had "log_pack_access && !*log_pack_access" in the first
if(), but I dropped it because of the caller's condition. It's a bit
fragile though. Imagine a new callsite is added.. (to be continued)

> Why not start from NULL:
>
>     static const char *log_pack_access;
>
> treat that NULL as "unknown" state, and avoid running getenv over
> and over again by treating non-NULL value as "known"?  Perhaps like
> this?
>
>         if (!log_pack_access) {
>                 /* First time: is env set? */
>                 log_pack_access = getenv("GIT_TRACE_PACK_ACCESS");
>                 if (!log_pack_access)
>                         log_pack_access = "";

or set log_pack_access to SomePredefinedButUselessString here. I
wanted to do this but couldn't because of global variable
initialization.

>         }
>         /* Now GIT_TRACE_PACK_ACCESS is known */
>         if (!*log_pack_access)
>                 return; /* not wanted */
>
> As you can no longer rely on "config is read before we do anything
> else" by switching to lazy env checking, your guard at the caller
> needs to be updated, if you want to reduce unneeded call-return
> overhead:
>
>         if (!log_pack_access || *log_pack_access)
>                 write_pack_access_log(...);

and this turns to "if (!log_.. || log != SomePrede...)", no more
peeking into log_pack_access.

>
> But the guard is purely for performance, not correctness, because
> the function now does the "return no-op if we know we are not
> wanted" itself.

(continued here) ..so yeah this looks better.

> Also you no longer need to have an extern variable in environment.c

will fix.
--
Duy