Re: [Maria-developers] c56c966: MDEV-10563 Crash during shutdown in Master_info_index::any_slave_sql_running
Vicențiu Ciorbaru
This is undefined behavior: // In GCC v6 this function _always_ returns 1. ClassA::member_function() { if (!this) return 0; return 1; }
Yeah, we shouldn't do this, agree.
make use of this construct. I have not changed the logic of the code, however I am not sure whether "give_error_if_slave_running()" is supposed to return TRUE if master_info_index is deleted (aka, we are performing a shutdown).
Probably there is no good reason to return TRUE. Unfortunately, there is a lot of code like this in replication, where it is not clear what the purpose is, but also very hard to be sure it's not needed. My guess would be - without knowing for sure - that the return value doesn't matter. This should only happen during shutdown anyway, and it seems to be used only for changing the values of some system-variables. If server is shutting down, then values of replication-related system-variables do not really matter. As to the patch, you're basically moving the check for NULL into all the callers. I'm not sure if the check is even needed for all callers, but then, I also do not think it's worth it to try and determine where it is needed and where not. But wouldn't it be nicer to instead change the give_error_if_slave_running() to be a normal function that takes the Master_info_index pointer as an argument (or a static method, if you want)? This way, you can keep the null-pointer check in the function. Also helps avoid a new bug if another caller is added. - Kristian.
Hi Kristian, Thanks for the quick and thorough reply. As to the patch, you're basically moving the check for NULL into all the
callers. I'm not sure if the check is even needed for all callers, but then, I also do not think it's worth it to try and determine where it is needed and where not.
Looking at the code, I am about 90% certain that we do need the checks. Most of the code paths can execute with the pointer being null.
But wouldn't it be nicer to instead change the give_error_if_slave_running() to be a normal function that takes the Master_info_index pointer as an argument (or a static method, if you want)? This way, you can keep the null-pointer check in the function. Also helps avoid a new bug if another caller is added.
The reason why I'm asking this about the return TRUE value is that it is something that Sergei wrote not too long ago (if 2 years can be considered not "too long" ago). I see how converting that to a function would be able to make us implement the guards better, but I feel that the semantics of it are best left as a member function. I would, however, wrap the null pointer check. Personally, I would've allocated this object as a global variable, adding the pesky LOCK code inside it and hiding it. I would have its members be pointers instead. That's a bit too much work for little gain however. I am inclined to push it _as is_ for now, but I'll wait for Sergei's opinion as well (he's on vacation right now, so it might take a bit). Thanks, Vicentiu
participants (2)
-
Kristian Nielsen -
Vicențiu Ciorbaru