Hi Sergey,
thanks for being attentive.
Sergey Vojtovich, 23.02.2009 18:40:
...
> On Mon, Feb 23, 2009 at 03:50:53PM +0000, Ingo Struewing wrote:
...
>> - my_errno= 0;
>> -
>> /* Get child list item. */
>> child_l= ha_myrg->next_child_attach;
>> if (!child_l)
> I believe we're mostly interested in what happens in the one-line-above
> if branch. According to the comment: @retval NULL, my_errno == 0
> Ok, no more child tables.
>
> So I'd say we should set my_errno to zero here, not below.
Right, I missed the DBUG_RETURN. Usually I try to avoid multiple returns
in a function, especially when using DBUG_RETURN.
Yes, I need to set my_errno here too. But as you rightfully mentioned in
your first review, one should not set my_errno before doing a lot of
actions. So it is probably better to set it explicitly short before
returning, even if that repeats the assignment.
>
>> @@ -544,11 +542,17 @@ static MI_INFO *myisammrg_attach_childre
>> child));
>> my_errno= HA_ERR_WRONG_MRG_TABLE_DEF;
>> }
>> + else
>> + {
>> + /* Assure that my_errno is set correctly on return. */
>> + my_errno= 0;
>> + }
>> +
> And we shouldn't do this here anymore, because we do not rely on
> my_errno value, when we return from this function.
At first glance, it sounds like a contradiction to the above (@retval
NULL, my_errno == 0 Ok, no more child tables).
But yes, in the 'else' branch, we did successfully retrieve 'myisam'. So
we return success, in which case my_errno is not specified.
>
>> DBUG_PRINT("myrg", ("MyISAM handle: %p my_errno: %d",
>> myisam, my_errno));
>>
>> err:
>> - DBUG_RETURN(my_errno ? NULL : myisam);
>> + DBUG_RETURN(myisam);
> As you fixed it here.
Regards
Ingo
--
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder, Wolfgang Engels, Dr. Roland Bömer
Vorsitzender des Aufsichtsrates: Martin Häring HRB München 161028
