maria-developers team mailing list archive

[Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>]

Vicențiu Ciorbaru <vicentiu@xxxxxxxxxxx> writes:

> 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.