On 12/05/17 08:58, Geert Uytterhoeven wrote:
> Hi Frank,
>
> On Tue, Dec 5, 2017 at 2:45 PM, Frank Rowand <[email protected]> wrote:
>> On 12/05/17 03:01, Geert Uytterhoeven wrote:
>>> On Tue, Dec 5, 2017 at 3:07 AM, Frank Rowand <[email protected]> wrote:
>>>> Also, the previous version of the patch, and the discussion around the
>>>> resulting
>>>> bug make me think that I should not have moved 'kfree(ovcs)' into
>>>> free_overlay_changeset(), because that kfree is then not very visible in
>>>> the
>>>> error path of of_overlay_apply(). Could you remove 'kfree(ovcs)' from
>>>> free_overlay_changeset(), and instead call it immediately after each call
>>>> to free_overlay_changeset()?
>>>
>>> Actually I like that free_overlay_changeset() takes care of the
>>> deallocation,
>>> especially in light of the kojectification op top from bbb-overlays, which
>>> means you cannot just call kfree(ovcs) anymore (I know this won't go
>>> upstream
>>> anytime soon, but I need overlay configfs for my development and testing).
>>
>> OK, knowing that kobjectification is being considered I am willing to leave
>> the
>> kfree(ovcs) where it is for now.
>>
>>> Perhaps the allocation of ovcs should be moved into
>>> free_overlay_changeset(),
>>
>> I think this
>> ^^^^^^^^^^^^^^^^^^^^^^^
>> is a typo, and you meant init_overlay_changeset().
>
> Yes it is.
>
>>> and the latter being renamed to alloc_overlay_changeset()?
>>> That way allocation and freeing become symmetrical.
>>> It would move the allocation under the mutexes, though.
>>
>> I considered moving the kzalloc() into init_overlay_changeset() when I
>> created it, but decided not to because the type of the first argument of
>> init_overlay_changeset() would change from
>> struct overlay_changeset *
>> to
>> struct overlay_changeset **,
>> and usage of ovcs would become _slightly_ more ugly and complex in
>> init_overlay_changeset().
>
> I would let alloc_overlay_changeset() return struct overlay_changeset *
> instead.
>
> If you care about why it failed, it can return ERR_PTR(error) instead of
> NULL ;-)
Yes, it should continue to return the error reason.
Thanks,
Frank
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds
>
