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