Re: [Tarantool-patches] [PATCH v1 1/2] sql: properly check bind variable names

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi! Thanks for the fixes!

>>>>>>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>>>>>>> index eb169aeb8..74a98c550 100644
>>>>>>> --- a/src/box/sql/expr.c
>>>>>>> +++ b/src/box/sql/expr.c
>>>>>>> @@ -1314,6 +1314,52 @@ sqlExprAssignVarNumber(Parse * pParse, Expr * pExpr, u32 n)
>>>>>>>  	}
>>>>>>>  }
>>>>>>>  
>>>>>>> +struct Expr *
>>>>>>> +expr_variable(struct Parse *parse, struct Token *spec, struct Token *id)
>>>>>>
>>>>>> 1. You might want to call it expr_new_variable(). Or sql_expr_new_variable().
>>>>>> To be consistent with our naming policy for constructors allocating memory
>>>>>> and for consistency with with sql_expr_new_column(), sql_expr_new(),
>>>>>> sql_expr_new_dequoted(), sql_expr_new_named(), sql_expr_new_anon().
>>>>>>
>>>>> Thank you! I renamed it to expr_new_variable(). I believe we should drop 'sql_'
>>>>> prefix for functions that only accessible in SQL.
>>>>
>>>> It would work for static functions. But if a function is visible in other
>>>> modules as a symbol, then you would get a conflict during linking if we
>>>> ever introduce another 'struct expr' somewhere. Even if they do not interest
>>>> anywhere in the code. However I don't mind leaving it as is. It can be fixed
>>>> later if ever needed.
>>>>
>>> I agree. However, I think we need to rework all the places where BOX uses
>>> internal SQL functions and structures. In this case, the struct expr should
>>> never be available in the BOX, so there should be no conflicts.
>>
>> It is a misunderstanding. It does not matter if you use a function in box or
>> not. If it is not static and is defined in 2 places - you will get a conflict
>> during link stage. Try to add a function with the same name to any file in
>> sql and to any file in box. Something like
>>
>> 	void
>> 	link_test123(void)
>> 	{
>> 		printf("in link test\n");
>> 	}
>>
>> (in a .cc file you would need to add 'extern "C"' for it). It will raise an
>> error during build. I added it to expr.c and to box.cc (with 'extern "C"'):
>>
>> 	duplicate symbol '_link_test123' in:
>> 	    ../../src/box/libbox.a(box.cc.o)
>> 	    ../../src/box/libbox.a(expr.c.o)
>> 	ld: 1 duplicate symbol for architecture x86_64
>>
> Got it, thanks for the explanation. It might be better to rename "struct Expr"
> to "struct sql_expr", in which case we will naturally use the sql_expr_ * prefix
> for such functions. How do you think?

Sounds good to me.