Thomas Zimmermann <tzimmermann@xxxxxxx> writes:
> Hi
>
> Am 17.02.23 um 09:37 schrieb Javier Martinez Canillas:
>> Thomas Zimmermann <tzimmermann@xxxxxxx> writes:
>>
>>> In fb_get_options(), always duplicate the returned option string and
>>> transfer ownership of the memory to the function's caller.
>>>
>>> Until now, only the global option string got duplicated and transferred
>>> to the caller; the per-driver options were owned by fb_get_options().
>>> In the end, it was impossible for the function's caller to detect if
>>> it had to release the string's memory buffer. Hence, all calling drivers
>>> leak the memory buffer. The leaks have existed ever since, but drivers
>>> only call fb_get_option() once as part of module initialization. So the
>>> amount of leaked memory is not significant.
>>>
>>> Fix the semantics of fb_get_option() by unconditionally transferring
>>> ownership of the memory buffer to the caller. Later patches can resolve
>>> the memory leaks in the fbdev drivers.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
>>> ---
>>
>> [...]
>>
>>> + if (option) {
>>> + if (options)
>>> + *option = kstrdup(options, GFP_KERNEL);
>>> + else
>>> + *option = NULL;
>>> + }
>>>
>>
>> I know the old code wasn't checking if kstrdup() succeeded, but you should
>
> Kstrdup uses kmalloc, which already warns about failed allocations. I
> think it's discouraged to warn again. (Wasn't there a warning in sparse
> or checkpatch?) So I'd rather leave it as is.
>
I didn't mean to warn but to return an error code.
>> do it here and let the caller know. And same if (!options). So I guess the
>> following check can be added (to be consistent with the rest of the code):
>>
>> if (!*option)
>> retval = 1;
>
> Why is that needed for consistency?
>
> Retval is the state of the output: enabled or not. If there are no
> options, retval should be 0(=enabled). 1(=disabled) is only set by
> video=off or that ofonly thing.
>
Ah, I see. I misundertood what retval was about. Forget this comment then.
Maybe while you are there could have another patch to document the return
value in the fb_get_options() kernel-doc?
And this patch looks good to me too after your explanations.
Reviewed-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
Best regards,
Javier
