List:Commits« Previous MessageNext Message »
From:Ingo Strüwing Date:February 24 2009 8:01am
Subject:Re: bzr commit into mysql-6.0 branch (ingo.struewing:2777) Bug#43086
View as plain text  
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
Thread
bzr commit into mysql-6.0 branch (ingo.struewing:2777) Bug#43086Ingo Struewing23 Feb
  • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2777) Bug#43086Sergey Vojtovich23 Feb
    • Re: bzr commit into mysql-6.0 branch (ingo.struewing:2777) Bug#43086Ingo Strüwing24 Feb