Add gssspi_mech_invoke method to turn on debugging #55
Conversation
| gssntlm_debug_initialized = true; | ||
|
|
||
| if (value->length > PATH_MAX - 1) { | ||
| return EINVAL; |
There was a problem hiding this comment.
In case if error is returned here, init was not done but gssntlm_debug_initialized is still set to true (few lines above).
Should we move setting gssntlm_debug_initialized below this error handling?
src/debug.c
Outdated
| vfprintf(debug_fd, fmt, ap); | ||
| va_end(ap); | ||
| fflush(debug_fd); | ||
| pthread_mutex_lock(&debug_mutex); |
There was a problem hiding this comment.
Using a mutex per each call to gssntlm_debug_printf is a performance regression...
I understand that you need to avoid the race between changes of gssntlm_debug_initialized and debug_fd, but the price for that is too high as for me.
Can't we use ONLY debug_fd variable in debug-related code and assume that debug_fd == NULL is the indication of gssntlm_debug_enabled? In that case a pointer always fits into machine word and is always updated atomically.
Also to avoid changes of debug_fd between calls to vfprintf and fflush we can copy the value of debug_fd into local on-stack variable. Yes, I know that theoretically we still can have a race condition when old debug_fd is already closed on the moment of calling vfprintf/fflush (because gssntlm_debug_disable was called just before) BUT in the worst-case we just have the error for vfprintf/fflush and we ignore it (operation with already closed fd).
There was a problem hiding this comment.
I just recognized that debug_fd is a pointer (not int fd) so when file is closed, we can't access this memory anymore even if pointer itself is copied to local variable. Probably we can work-around it by setting debug_fd to NULL before actually calling fclose for it. However if debug_fd was changed few times (like using disabling + enabling) we probably can't avoid the race completely... So probably this way is not robust enough...
There was a problem hiding this comment.
The mutex kicks in only during debugging, so I thought it would be ok to trade a bit of performance in this case.
Can you confirm that you are still concerned about performance while debugging ?
There was a problem hiding this comment.
Note that passing NULL to vfprintf() will cause the process to crash, so I cannot just use debug_fd as an atomic, I will definitely have to copy it ... but then we can still have a crash if we point to a freed pointer... perhaps I should convert this code to use a file descriptor instead, as trying to operate on a bad fd will indeed simply cause an error and no crashes ...
There was a problem hiding this comment.
We were going to always use gssntlmssp debugging feature in our product - to be able to troubleshoot issues when they happened. I agree that it's possible to implement on-demand debugging (debugging is disabled by default but can be temporary enabled manually), but issues are often hard to reproduce so this way of troubleshooting is not robust.
There was a problem hiding this comment.
Yes, I reviewed new code with numerical debug_fd descriptor and no mutex in gssntlm_debug_printf - this is exactly what I wanted to get! No mutex overhead in the main debug flow and no pointers make it safe, simple and robust. Thank you very much!
|
@kvv81 please check this new version, I use pthread mutexes only to change debug_fd, but use it as it is for actual debugging. debug_fd is also now a file descriptor and not a file stream pointer anymore. |
This allows to set a file of own chosing as well as turn on and off debugging as needed. Mostly thread safe, and applies to all threads at once. There is a very small window for a race condition where a thread modifies the debug_fd at the same time another thread is actually trying to print a debug message. In order to handle this case without the performance hit of a mutex on every debug message, we change the implementation to use file descriptors instead. printing to a closed file descriptor or to a -1 file descriptor returns an error but does not crash the process as it happens when a file stream is NULL or pointing to freed memory. Signed-off-by: Simo Sorce <[email protected]>
|
This looks exactly like what we need! We will use public API function 'gssspi_mech_invoke' in our init stage to start collection of troubleshooting info. Also it is possible to stop debugging if SMB server is disabled and NTLM is not in use anymore. Additionally in case if we will need to implement logs rotation from our side, we can use re-enabling debug log with different file name to achieve this effect. Thank you very much for your efforts, your help is really appreciated! |
|
My pleasure, the features you request/bugs you find are often fun to implement :) |
simo5
changed the title
This allows to set a file of own chosing as well as turn on and off
debugging as needed.
Thread safe, and applies to all threads at once.
resolves #53