Re: [PATCH 07/14] exynos/fimg2d: add g2d_validate_xyz() functions

[Tobias Jakobi <tjakobi@math.uni-bielefeld.de>]

Hello,


Emil Velikov wrote:
> On 31 August 2015 at 14:18, Inki Dae <inki.dae@samsung.com> wrote:
>> On 2015년 08월 24일 23:14, Tobias Jakobi wrote:
>>> The G2D headers define a number of modes through enums
>>> (like e.g. color, select, repeat, etc.).
>>>
>>> This introduces g2d_validate_select_mode() and
>>> g2d_validate_blending_op() which validate a
>>> select mode or blending operation respectively.
>>>
>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>> ---
>>>  exynos/exynos_fimg2d.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 44 insertions(+)
>>>
>>> diff --git a/exynos/exynos_fimg2d.c b/exynos/exynos_fimg2d.c
>>> index 2e04f4a..781aff5 100644
>>> --- a/exynos/exynos_fimg2d.c
>>> +++ b/exynos/exynos_fimg2d.c
>>> @@ -119,6 +119,50 @@ static unsigned int g2d_check_space(const struct g2d_context *ctx,
>>>  }
>>>
>>>  /*
>>> + * g2d_validate_select_mode - validate select mode.
>>> + *
>>> + * @mode: the mode to validate
>>> + */
>>> +static unsigned int g2d_validate_select_mode(
>>> +     enum e_g2d_select_mode mode)
>>> +{
>>> +     switch (mode) {
>>> +     case G2D_SELECT_MODE_NORMAL:
>>> +     case G2D_SELECT_MODE_FGCOLOR:
>>> +     case G2D_SELECT_MODE_BGCOLOR:
>>> +             return 0;
>>> +     }
>>
>> It's strange use a bit. Just check the range like below,
>>
>> First, please add G2D_SELECT_MODE_MAX which has 3 << 0, and
>>
>> if (G2D_SELECT_MODE_MAX < mode) {
>>         fprintf(strerr, "invalid command(0x%x)\n", mode);
>>         return -EINVAL;
>> }
>>
> Listing every value might seem like an overkill, indeed. Yet I think
> it's the more robust approach.
that's my thinking as well. The overhead shouldn't be too high and the
compiler probably optimizes this anyway.



> By adding _MAX to the API we effectively lock it down. That can be
> avoided, plus the compiler will warn us when new values are added to
> the enum. If someone starts getting smart (due to the _MAX) and adds
> G2D_SELECT_MODE_FOO = -1, the above check will fail :(
> 
>> And I think it'd be better to change return type of this function to int,
>>
> Good idea.
What would be the benefit of this? We're just returning only 0 and 1
anyway. My first reaction was to use a bool here.


> 
> Cheers,
> Emil
> 


With best wishes,
Tobias