Re: [PATCH 27/36] attr: convert to new threadsafe API

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Stefan Beller <sbeller@xxxxxxxxxx> writes:

> This revamps the API of the attr subsystem to be thread safe.
> Before we had the question and its results in one struct type.
> The typical usage of the API was
>
>     static struct git_attr_check *check;
>
>     if (!check)
>         check = git_attr_check_initl("text", NULL);
>
>     git_check_attr(path, check);
>     act_on(check->value[0]);
>
> This has a couple of issues when it comes to thread safety:
>
> * the initialization is racy in this implementation. To make it
>   thread safe, we need to acquire a mutex, such that only one
>   thread is executing the code in git_attr_check_initl.
>   As we do not want to introduce a mutex at each call site,
>   this is best done in the attr code. However to do so, we need
>   to have access to the `check` variable, i.e. the API has to
>   look like
>     git_attr_check_initl(struct git_attr_check*, ...);

Make that a double-asterisk.  The same problem appears in an updated
example in technical/api-gitattributes.txt doc, but the example in
the commit log message (below) is correct.

> The usage of the new API will be:
>
>     /*
>      * The initl call will thread-safely check whether the
>      * struct git_attr_check has been initialized. We only
>      * want to do the initialization work once, hence we do
>      * that work inside a thread safe environment.
>      */
>     static struct git_attr_check *check;
>     git_attr_check_initl(&check, "text", NULL);
>
>     /*
>      * Obtain a pointer to a correctly sized result
>      * statically allocated on the stack; this macro:
>      */
>     GIT_ATTR_RESULT_INIT_FOR(myresult, 1);

Are you sure about this?  We've called attr_check_initl() already so
if this is declaring myresult, it would be decl-after-stmt.

>     /* Perform the check and act on it: */
>     git_check_attr(path, check, myresult);
>     act_on(myresult->value[0]);
>
>     /*
>      * No need to free the check as it is static, hence doesn't leak
>      * memory. The result is also static, so no need to free there either.
>      */

The latter half is questionable.  If it is "static" it wouldn't be
thread safe, no?  I think the diff in this patch for archive.c shows
that we only expect

	struct git_attr_result result[2];

upfront without RESULT_INIT_FOR(), and the reason why there is no
need to free the result[] is because it is on the stack.  And each
element in result[] may point at a string, but the string belongs to
the attr subsystem and must not be freed.




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]