Re: [PATCH 3/4] block: add IOC_PR_READ_KEYS ioctl

Krzysztof Kozlowski
2025-12-01T16:26:32Z

On 01/12/2025 16:06, Stefan Hajnoczi wrote:
> On Sat, Nov 29, 2025 at 03:32:35PM +0100, Krzysztof Kozlowski wrote:
>> On 27/11/2025 08:07, Hannes Reinecke wrote:
>>>
>>>> + size_t keys_info_len = struct_size(keys_info, keys, inout.num_keys);
>>>> +
>>>> + keys_info = kzalloc(keys_info_len, GFP_KERNEL);
>>>> + if (!keys_info)
>>>> + return -ENOMEM;
>>>> +
>>>> + keys_info->num_keys = inout.num_keys;
>>>> +
>>>> + ret = ops->pr_read_keys(bdev, keys_info);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + /* Copy out individual keys */
>>>> + u64 __user *keys_ptr = u64_to_user_ptr(inout.keys_ptr);
>>>> + u32 num_copy_keys = min(inout.num_keys, keys_info->num_keys);
>>>> + size_t keys_copy_len = num_copy_keys * sizeof(keys_info->keys[0]);
>>>
>>> We just had the discussion about variable declarations on the ksummit
>>> lists; I really would prefer to have all declarations at the start of
>>> the scope (read: at the start of the function here).
>>
>> Then also cleanup.h should not be used here.
>
> Hi Krzysztof,
> The documentation in cleanup.h says:
>
> * Given that the "__free(...) = NULL" pattern for variables defined at
> * the top of the function poses this potential interdependency problem
> * the recommendation is to always define and assign variables in one
> ^^^^^^^^^^^^^^
> * statement and not group variable definitions at the top of the
> * function when __free() is used.
>
> This is a recommendation, not mandatory. It is also describing a
> scenario that does not apply here.

If you have actual argument, so allocation in some if branch, the of course.

>
> There are many examples of existing users of __free() initialized to
> NULL:
>
> $ git grep '__free(' | grep ' = NULL' | wc -l
> 491
>

There is tons of incorrect usage, some I started already fixing.
Maintainers were changing when applying the correct code (correct patch)
into incorrect declaration without constructor and separate assignment.

Then contributors started adding patches at least making NULL
assignment, but still not following recommendation.

Then contributors started adding patches mixing cleanup.h with gotos,
also clearly discouraged.

Sorry, but please never use that argument. People did not read cleanup.h
and produced poor code. Maintainers did not read cleanup.h and changed
correct code into poor code.

Existing poor code, just its existence, is never the actual argument in
discussion.

> To me it seems like it is okay to use cleanup.h in this fashion. Did I
> miss something?

Best regards,
Krzysztof