On Mon, 2018-08-06 at 18:12 -0500, Benjamin Marzinski wrote: +AD4- On Sun, Aug 05, 2018 at 03:43:18PM +-0000, Bart Van Assche wrote: +AD4- +AD4- On Wed, 2018-08-01 at 15:57 -0500, Benjamin Marzinski wrote: +AD4- +AD4- +AD4- when tur+AF8-thread() was calling tur+AF8-check(), it was passing ct-+AD4-message as +AD4- +AD4- +AD4- the copy argument, but copy+AF8-msg+AF8-to+AF8-tcc() was assuming that it was +AD4- +AD4- +AD4- getting a tur+AF8-checker+AF8-context pointer. This means it was treating +AD4- +AD4- +AD4- ct-+AD4-message as ct. This is why the tur checker never printed checker +AD4- +AD4- +AD4- messages. Intead of simply changing the copy argument passed in, I just +AD4- +AD4- +AD4- removed all the copying code, since it is completely unnecessary. The +AD4- +AD4- +AD4- callers of tur+AF8-check() can just pass in a buffer that it is safe to +AD4- +AD4- +AD4- write to, and copy it later, if necessary. +AD4- +AD4- +AD4- +AFs- ... +AF0- +AD4- +AD4- +AD4- -+ACM-define TUR+AF8-MSG(fmt, args...) +AFw- +AD4- +AD4- +AD4- - do +AHs- +AFw- +AD4- +AD4- +AD4- - char msg+AFs-CHECKER+AF8-MSG+AF8-LEN+AF0AOw- +AFw- +AD4- +AD4- +AD4- - +AFw- +AD4- +AD4- +AD4- - snprintf(msg, sizeof(msg), fmt, +ACMAIw-args)+ADs- +AFw- +AD4- +AD4- +AD4- - copy+AF8-message(cb+AF8-arg, msg)+ADs- +AFw- +AD4- +AD4- +AD4- - +AH0- while (0) +AD4- +AD4- +AD4- - +AD4- +AD4- +AD4- +AFs- ... +AF0- +AD4- +AD4- +AD4- -static void copy+AF8-msg+AF8-to+AF8-tcc(void +ACo-ct+AF8-p, const char +ACo-msg) +AD4- +AD4- +AD4- -+AHs- +AD4- +AD4- +AD4- - struct tur+AF8-checker+AF8-context +ACo-ct +AD0- ct+AF8-p+ADs- +AD4- +AD4- +AD4- - +AD4- +AD4- +AD4- - pthread+AF8-mutex+AF8-lock(+ACY-ct-+AD4-lock)+ADs- +AD4- +AD4- +AD4- - strlcpy(ct-+AD4-message, msg, sizeof(ct-+AD4-message))+ADs- +AD4- +AD4- +AD4- - pthread+AF8-mutex+AF8-unlock(+ACY-ct-+AD4-lock)+ADs- +AD4- +AD4- +AD4- -+AH0- +AD4- +AD4- +AD4- +AD4- The above code was introduced because multiple threads can access ct-+AD4-message +AD4- +AD4- concurrently. Does your patch reintroduce the race conditions on the ct-+AD4-message +AD4- +AD4- buffer that were fixed by commit f63f4d115813 (+ACI-libmultipath/checkers/tur: Protect +AD4- +AD4- tur+AF8-checker+AF8-context.message changes+ACI-)? +AD4- +AD4- It shouldn't. Since the tur checker thread only has access to the +AD4- tur+AF8-checker+AF8-context, the only race we have to worry about here is on +AD4- access to ct-+AD4-message. The main checker code is free to pass c-+AD4-message +AD4- to tur+AF8-check(), since the tur checker thread doesn't have access to +AD4- that. With this patch, tur+AF8-thread() passes tur+AF8-check() a local variable +AD4- and copies that variable to ct-+AD4-message while holding the mutex. the +AD4- main checker code copies ct-+AD4-message to c-+AD4-message while holding the +AD4- same mutex. The main checker code even clears ct-+AD4-message before +AD4- starting the thread while holding the mutex. +AD4- +AD4- If you see something I've missed, please let me know. Hello Ben, Thanks for the additional clarification. I think this patch is a good improvement for the TUR checker since it simplifies the code. Bart.
-- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
