Re: [MTASCsft PATCH WIP6 01/33] MT-, AS- and AC-Safety docs

["Carlos O'Donell" <carlos at redhat dot com>]

On 01/16/2014 07:17 PM, Alexandre Oliva wrote:
> On Jan 15, 2014, "Carlos O'Donell" <carlos@redhat.com> wrote:
> 
>> *
>>   Text
> 
>> This is because of the newline between @item and the text.
> 
> *nod*, I'm taking the newlines out.
 
Thanks.
 
>> @code{MT-Safe}
> 
>> Thread-Safe [...]
> 
>> Like you did with the other properties.
> 
> Unlike the sections that define safety keywords, in which each bullet
> defines a separate term, in this list we have a single bullet for all of
> the *-Unsafe terms, so I didn't think this would be appropriate.

Right, and I think it's OK to have all one bullet with 3 entries.

> I ended up changing it to:
> 
> @item
> [...]
> @code{MT-Safe} or Thread-Safe functions are safe to call in the presence
> [...]
> @item
> [...]
> @code{MT-Unsafe}, @code{AS-Unsafe}, @code{AC-Unsafe} functions are not
 
That's fine except that `@item\nText' still gives a bullet without any
text beside it and that confusingly looks like missing text.
 
>>> +In some cases, even expanding MT-Safe functions inline could cause
>>> +combinations to become MT-Unsafe due to reordering that this could
>>> +enable; this is never the case of @glibcadj{} functions defined in
>>> +user-visible headers, because these are meant to be inlined, but it may
>>> +be for functions that are exported by @glibcadj{} shared libraries;
>>> +whole-program optimizations that might inline functions across library
>>> +interfaces might expose this sort of problem, so performing inlining
>>> +across the @glibcadj{} interface is not recommended, nor is the
>>> +documented MT-Safety status guaranteed under such arrangements.
> 
>> OK, though that paragraph is a single sentence it makes and is hard to
>> split without a lot of duplication.
> 
> How about replacing the semicolons with periods?  Would that help at
> all?  (I wouldn't think so, but then, I don't even count the
> semicolon-separated sentences as a single sentence ;-)

It would not help to add periods. You'd have to duplicate a lot of text.

Please leave it as is for now. 
 
>>> +AC-Safe or Async-Cancel-Safe functions are safe to call when
>>> +asynchronous cancellation is enabled.  AC in AC-Safe stands for
>>> +Asynchronous Cancellation.
> 
>> Do you think we need a little more here?
> 
> Hmm, I don't know.  If I had to add more, I'd probably explain the
> cancellation modes, rather than list the mandated AC-Safe functions and
> duplicate the preliminary status of the docs, but I wouldn't go as far
> as objecting to the paragraph you proposed, so...  added.

Thanks.

>> At present there is no dlopen documentation.
> 
>> Is this a placeholder of all dlopen uses?
> 
> Yeah.

Perfect.

>> Can we make that clearer?
> 
>> Suggest the simpler:
>> ~~~
>> The locks are enough for these functions to be AS- and AC-Unsafe, but
>> other issues may arise. At present this is a placeholder for all
>> potential safety issues raised by @code{dlopen}.
>> ~~~
> 
> Thanks, taken.

OK.

>> s/nss/name service switch (NSS)/g.
> 
>> Since `nss' can actually be confused with `network security services'
>> which glibc actually uses in some configurations to implement secure
>> hashing.
> 
>> Use @cindex NSS?
> 
> 'k.  I also mentioned character set conversion back-ends, with @cindex
> iconv.

Perfect.

>>> +@c But what if, instead of marking modifiers with const:id and readers
>>> +@c with just id, we marked writers with race:id and readers with ro:id?
> 
>> This comment needs expanding. What would you gain by doing it this way?
> 
> Some clarity, I think.  Instead of having to define each instance of
> âidâ, we'd have a general pattern governing all such âidâs, wherein
> race:id would suggest the need for an exclusive/write lock to make the
> function safe, whereas ro:id would indicate âidâ is expected to be
> read-only, but if any modifiers are called (while holding an exclusive
> lock), then ro:id-marked functions ought to be guarded with a read lock
> for safe operation.  ro:env or ro:locale, for example, seems to convey
> more clearly the expectations and the meaning, than just env or locale.

Add this to the comment :-)
 
>>> +@c fixme: at least sync cancellation should get it right, and would
>>> +@c obviate the restoring bit below, and the qualifier above.
> 
>> What do you mean by "sync cancellation?" Deferred cancellation?
> 
> I meant sync as in the opposite of async; deferred is not a perfect
> opposite IMHO.  E.g., I'd ragard
> pthread_cancel(pthread_self()),pthread_testcancel() as immediate (rather
> than deferred) synchronous cancellation, even though it happens to rely
> on both async or deferred cancellation machinery to get immediate
> cancellation.
> 
> In the context above, I guess it makes sense to write deferred, to avoid
> having to define an additional term, since immediate synchronous
> cancellation is not a possibility.  Both occurrences have been changed.

Thanks. The goal here is to harmonize our language with POSIX.
 
>>> +Functions marked with @code{term} as an AC-Safety issue are supposed to
>>> +restore terminal settings to their original state, after temporarily
>>> +changing them, but they may fail to do so if cancelled, even
>>> +synchronously.
> 
>> s/, even synchronously//g.
> 
> Do you mean to take it out because (a) âcancelledâ covers both async and
> deferred cancellation, (b) bugs in handling deferred cancellation are to
> be fixed rather than turned into (mis)features by means of
> documentation, or (c) because it was not clear that we have a bug that's
> more serious than AC-Unsafe?  I assume you meant (a), but I'm concerned
> people might fail to realize this is a case of C-Unsafe (even deferred
> cancellation is mishandled), not just AC-Unsafe.

I meant (a).

I don't see the need to clarify and it simplifies the wording.
 
>>> +Functions that allocate or deallocate file descriptors will generally be
>>> +marked as such, because even if they attempted to protect the file
>>> +descriptor allocation and deallocation with cleanup regions, allocating
>>> +a new descriptor and storing its number where the cleanup region could
>>> +release it cannot be performed as a single atomic operation, just like
>>> +releasing it and taking it out of the data structure normally
>>> +responsible for releasing it cannot be performed atomically, always
>>> +leaving a window in which the descriptor cannot be released because it
>>> +was not stored in the cleanup handler argument yet, or in which it was
>>> +already taken out of it before releasing it in the normal flow (we
>>> +cannot keep it there because, in case of cancellation, we would not be
>>> +able to tell whether it was already released, and the same number could
>>> +have been already assigned to another descriptor by another thread, so
>>> +we could not just release it again).
> 
>> Run on sentence. Please split this up into 2 or 3 sentences.

Great work splitting that into multiple sentences! :-)

> Functions that allocate or deallocate file descriptors will generally be
> marked as such.  Even if they attempted to protect the file descriptor
> allocation and deallocation with cleanup regions, allocating a new
> descriptor and storing its number where the cleanup region could release
> it cannot be performed as a single atomic operation.  Similarly,
> releasing the descriptor and taking it out of the data structure
> normally responsible for releasing it cannot be performed atomically.
> There will always be a window in which the descriptor cannot be released
> because it was not stored in the cleanup handler argument yet, or in
> which it was already taken out before releasing it in the normal flow.

s/in which//g

s/in the normal flow//g

> It cannot be taken out after release either: an open descriptor could
> mean either that the descriptor still has to be closed, or that normal
> flow already did so but the descriptor was reallocated by another thread
> or signal handler.
 
s/normal flow/it/g

OK with those simplifications.
 
>>> +@item @code{posix}
>>> +@cindex posix
> 
>> This seems to violate the principle of least surprise in that "posix"
>> seems like it should indicate POSIX compliance.
> 
> How about rendering it as !posix (to be read not posix)?  I've used
> e.g. âwhatever/!bsdâ to indicate the whatever keyword applies to kernels
> other than BSD's.

I like "!posix" :-)
 
> With this, I've completed the changes you've suggested.  Still pending
> are the few questions above and the one in the separate email.
> 
> I have a few other small changes to integrate that I'd kept in separate
> patches in the patchset; I'll post them shortly, so that we can complete
> the review cycle and hopefully the next round of review of this patch
> will yield an âok to installâ :-)

Almost there.

Cheers,
Carlos.