Re: [PATCH V2] fgetgrent.3: ATTRIBUTES: Note function that is thread-safe

[Ma Shimiao <mashimiao.fnst-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>]

On 02/12/2015 04:22 PM, Michael Kerrisk (man-pages) wrote:
> Hello Ma Shimiao,
> 
> On 02/12/2015 02:24 AM, Ma Shimiao wrote:
>> Hi Michael,
>> On 02/11/2015 06:20 PM, Michael Kerrisk (man-pages) wrote:
>>> Hi Ma Shimiao,
>>>
>>> On 11 February 2015 at 10:38, Ma Shimiao <mashimiao.fnst-BthXqXjhjHV4OK5fxMSSsQ@public.gmane.org.com> wrote:
>>>> On 02/11/2015 04:28 PM, Michael Kerrisk (man-pages) wrote:
>>>>> Hi Ma Shmiao,
>>>>>
>>>>> On 11 February 2015 at 09:20, Ma Shimiao <mashimiao.fnst-BthXqXjhjHU@public.gmane.orgsu.com> wrote:
>>>>>> Hi Michael,
>>>>>> On 02/11/2015 03:55 PM, Michael Kerrisk (man-pages) wrote:
>>>>>>> On 02/11/2015 08:35 AM, Ma Shimiao wrote:
>>>>>>>> The marking matches glibc marking.
>>>>>>>>
>>>>>>>> Signed-off-by: Ma Shimiao <mashimiao.fnst-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
>>>>>>>> ---
>>>>>>>>  man3/fgetgrent.3 | 12 ++++++++++++
>>>>>>>>  1 file changed, 12 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/man3/fgetgrent.3 b/man3/fgetgrent.3
>>>>>>>> index 57665dd..e599483 100644
>>>>>>>> --- a/man3/fgetgrent.3
>>>>>>>> +++ b/man3/fgetgrent.3
>>>>>>>> @@ -90,6 +90,18 @@ is set to indicate the cause.
>>>>>>>>  Insufficient memory to allocate
>>>>>>>>  .I group
>>>>>>>>  structure.
>>>>>>>> +.SH ATTRIBUTES
>>>>>>>> +For an explanation of the terms used in this section, see
>>>>>>>> +.BR attributes (7).
>>>>>>>> +.TS
>>>>>>>> +allbox;
>>>>>>>> +lb lb lb
>>>>>>>> +l l l.
>>>>>>>> +Interface   Attribute       Value
>>>>>>>> +T{
>>>>>>>> +.BR fgetgrent ()
>>>>>>>> +T}  Thread safety   MT-Unsafe race:fgrent
>>>>>>>
>>>>>>> Why the change here in the V2? What does "fgrent" refer to?
>>>>>>
>>>>>> race:fgrent is right mark, I made a mistake.
>>>>>> race:fgrent is similar to race:grent.
>>>>>> race:grent is used to indicate data race exists in getgrent(), setgrent() and endgrent().
>>>>>> race:fgrent is used to indicate data race exists when using fgetgrent() in multi-thread.
>>>>>
>>>>> My question then is: how does the reader know that "grent" refers to
>>>>> "getgrent(), setgrent() and endgrent()"?
>>>>
>>>> From definition of race in glibc manual, we get:
>>>> If data race exists and objects cause data race are not from user,
>>>> then the function should be annotated as MT-Unsafe and marked with race.
>>>> And we need a colon and an identifier follows race to tell user what causes data race.
>>>>
>>>> getgrent(), setgrent() and endgrent() are usually used together.
>>>> Because they need to share an iterator, then data race occurs between them.
>>>> We want users to know when using them together in multi-thread, data race makes them
>>>> unsafe in multi-thread. But, we can't definitely write which internal object causes data race.
>>>> So, we extract the common string 'grent' from functions' name as a identifier which groups
>>>> getgrent(), setgrent() and endgrent() to tell users that the group of *grent() functions
>>>> can't be used together in multi-thread.
>>>
>>> All of the above is fine. But:
>>>
>>>> If a reader understand the definition of race, I think he can know that "grent" refers to
>>>> getgrent(), setgrent() and endgrent().
>>>
>>> I think many readers will not be able to deduce this. (And I think
>>> readers of the glibc manual will have exactly the same problem.) I
>>> think we somehow need to make this a bit clearer, perhaps in a
>>> sentence following the table. Would you have a proposal for such a
>>> sentence?
>>
>> How about the following sentence?
>> "grent" in "race:grent" is an identifier which groups functions xxxgrent() used to remind that
>> if functions xxxgrent() were used together in multi-thread, data race would occur.
> 
> I think that's a good start, but I'd prefer something a little more explicit:
> 
> [[
> In the above table,
> .I grent in
> .I race:grent
> signifies that if any of the functions
> .BR setgrent (),
> .BR getgrent (),
> or
> .BR endgrent ()
> are used in parallel in different threads of a program, then data races could occur.
> ]]
> 
> How would that be?

That looks fine. I will add them in new patch.
> 
> Also, what is the analogous text for fgetgrent() / "fgrent"? Is the problem
> races between different threads using fgetgrent() or is it races with another 
> thread using setgrent/getgrent/sendgrent? If the former, I don't understand 
> why we need the identifier "fgrent" instead of just using "fgetgrent".
The problem is the former.
At first, I chose fgetgrent as identifier.
Then, I found fgrent is used in glib manual.
As fgetgrent looks like getgrent(), I thought make identifier of race to be
similar is not a bad idea.
At the end, I modified fgetgrent to fgrent.

Now, after thinking for while, fgetgrent seems more suitable.
I‘m sorry for sending patches in haste.
I haven't asked glibc community why they choose to use fgrent.
After talking with them, I will send a new patch.
Is it OK?


Thanks
> 
> Thanks,
> 
> Michael
> 


-- 
Ma Shimiao
Development Dept.I
Nanjing Fujitsu Nanda Software Tech. Co., Ltd.(FNST)
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html