On Thu, Aug 4, 2016 at 2:22 PM, Barret Rhoden <[email protected]> wrote:
> On 2016-08-04 at 13:58 Dan Cross <[email protected]> wrote: > > On line 116 of user/parlib/dtls.c, could you ever encounter a situation > > where you have to call dtls_cache_init() as is done in > > __allocate_dtls_key() ? > > Maybe. That'd be a user bug for calling a dtls operation on a key that > wasn't created. Fair enough. > On line 199, what purpose does the assignment of NULL to v->dtls serve? Is > > the idea that the dtor could inadvertently reference it? > > set/get_dtls() both look to the uninitiated like they could be racy; > would > > it make sense to add a comment of some kind explaining how they are not > > (e.g., because they are inherently thread-local: the TL in DTLS). > > DTLS is basically a 2LS-independent service that is analogous to > pthread_key_create(). > > http://linux.die.net/man/3/pthread_key_create > > The setting of NULL is based on this: > At thread exit, if a key value has a non-NULL destructor > pointer, and the thread has a non-NULL value associated with > that key, the value of the key is set to NULL, and then the > function pointed to is called with the previously associated > value as its sole argument. Ok. We should document that DTLS is meant to follow pthread TLS in this regard somewhere.... > > On lines 13-15 of user/parlib/include/parlib/dtls.h, __GNUC__ is sadly > > defined by several compilers (GCC, clang, and the Intel C compiler). Is > it > > really that we care about GCC, or is it just that we care about being > > GCC-compatible? (I assume the latter.) > > You'd have to ask Kevin. This code originally came from Linux Parlib. I guess a larger question is if we'd ever try to compile this with a compiler for which this wouldn't work. Perhaps we should remove the '#error' and ifdef's, since they don't actually test for exactly GCC. > On line 20 of user/utest/dtls.c I think it would be slightly more > > informative to say, "Expected NULL ..." instead of "expected 0" to > > accentuate that it's looking at a pointer, not an int. > I don't see a change on github; did you push it up? > > On line 33, it may be nice to get the value before dtls_destroy() to > > validate that it WAS set before resetting. > I don't see a change on github; did you push it up? > > On lines 64-65, what's the expected behavior of setting twice? Last one > > wins? It would be nice if the test asserted that. > I don't see a change on github; did you push it up? > > On line 74 how does one pronounce "f00"? :-D > > Noted. That last one was meant to be a joke. :-) > On lines 77-85, it's unclear to me what the expected behavior of setting > > from a dtor is; could you add an assert to probe that? > > It is expected to not loop infinitely or to lose storage. > > Calling pthread_setspecific() from a thread-specific data > destructor routine may result either in lost storage (after at > least PTHREAD_DESTRUCTOR_ITERATIONS attempts at destruction) or > in an infinite loop. > > Kevin designed it to not fail in that manner. We can notice the > infinite loop, but not the storage loss. Ah. It is worth adding a comment to the test case to explain that. > On line 96 perhaps use ARRAY_SIZE() or COUNT_OF() or whatever we're > calling > > it now? > > On line 104; what's the behavior when there are no arguments so the > > program, so that main() gets argc == 1 and argv == {"testname", NULL} => > > whitelist == a pointer a NULL pointer? > > This is boilerplate from the utests infrastructure. Sure, that's obvious. Looking at the code it appears that the test does nothing if a whitelist is not explicitly set. - Dan C. -- You received this message because you are subscribed to the Google Groups "Akaros" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. For more options, visit https://groups.google.com/d/optout.
