> Date: Thu, 30 Mar 2023 10:38:23 +0200
>
> Some update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (4):
> Return directly for two checks
> Improve two size determinations
> Improve exception handling
> Move some assignments for the variable “err”
>
> fs/overlayfs/super.c | 72 ++++++++++++++++++++++++--------------------
> 1 file changed, 39 insertions(+), 33 deletions(-)
Is this patch series still in review queues?
See also:
https://lore.kernel.org/cocci/[email protected]/
https://sympa.inria.fr/sympa/arc/cocci/2023-03/msg00115.html
Regards,
Markus
On Wed, Jan 10, 2024 at 2:25 PM Markus Elfring <[email protected]> wrote:
>
> > Date: Thu, 30 Mar 2023 10:38:23 +0200
> >
> > Some update suggestions were taken into account
> > from static source code analysis.
> >
> > Markus Elfring (4):
> > Return directly for two checks
> > Improve two size determinations
> > Improve exception handling
> > Move some assignments for the variable “err”
> >
> > fs/overlayfs/super.c | 72 ++++++++++++++++++++++++--------------------
> > 1 file changed, 39 insertions(+), 33 deletions(-)
>
> Is this patch series still in review queues?
>
Sorry, this series was not on my radar.
> See also:
> https://lore.kernel.org/cocci/[email protected]/
> https://sympa.inria.fr/sympa/arc/cocci/2023-03/msg00115.html
>
I will queue cleanup patches 1-2, but I do not like patches 3/4 and 4/4.
I do not think that they make the code better to read or maintain.
Thanks,
Amir.
>> See also:
>> https://lore.kernel.org/cocci/[email protected]/
>> https://sympa.inria.fr/sympa/arc/cocci/2023-03/msg00115.html
>
> I will queue cleanup patches 1-2,
Thanks for this positive feedback.
> but I do not like patches 3/4 and 4/4.
> I do not think that they make the code better to read or maintain.
I would appreciate if the details for such change reluctance can be clarified better.
Regards,
Markus
On Wed, Jan 10, 2024 at 3:01 PM Markus Elfring <[email protected]> wrote:
>
> >> See also:
> >> https://lore.kernel.org/cocci/87b65f8e-abde-2aff-4da8-df6e0b464677@webde/
> >> https://sympa.inria.fr/sympa/arc/cocci/2023-03/msg00115.html
> >
> > I will queue cleanup patches 1-2,
>
> Thanks for this positive feedback.
Sorry, these patches do not apply to master branch and patch 1
is no longer correct in master branch and the new mount api changes.
>
>
> > but I do not like patches 3/4 and 4/4.
> > I do not think that they make the code better to read or maintain.
>
> I would appreciate if the details for such change reluctance can be clarified better.
patch 3:
I much rather a single error handling label that takes care of
all the cleanups - it is harder to make mistakes and jump to
the wrong label when adding new error conditions.
patch 4:
Overlayfs uses this coding style all over the place
err = -ENOMEM;
ofs->creator_cred = cred = prepare_creds();
if (!cred)
goto out_free_ofs;
I don't see the benefit in making err = -ENOMEM conditional.
I don't see the style after your patch as clearly better than before.
Thanks,
Amir.
>>>> See also:
>>>> https://lore.kernel.org/cocci/[email protected]/
>>>> https://sympa.inria.fr/sympa/arc/cocci/2023-03/msg00115.html
>>>
>>> I will queue cleanup patches 1-2,
>>
>> Thanks for this positive feedback.
>
> Sorry, these patches do not apply to master branch and patch 1
> is no longer correct in master branch and the new mount api changes.
Do you want that I adapt the linked development ideas to the current situation
a bit more?
>>> but I do not like patches 3/4 and 4/4.
>>> I do not think that they make the code better to read or maintain.
>>
>> I would appreciate if the details for such change reluctance can be clarified better.
>
> patch 3:
> I much rather a single error handling label that takes care of
> all the cleanups - it is harder to make mistakes and jump to
> the wrong label when adding new error conditions.
There are different coding style preferences involved.
See also:
https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources
> patch 4:
> Overlayfs uses this coding style all over the place
>
> err = -ENOMEM;
> ofs->creator_cred = cred = prepare_creds();
> if (!cred)
> goto out_free_ofs;
>
> I don't see the benefit in making err = -ENOMEM conditional.
> I don't see the style after your patch as clearly better than before.
Can it be nicer to set error codes only in exceptional data processing situations?
Regards,
Markus
On Wed, Jan 10, 2024 at 3:33 PM Markus Elfring <[email protected]> wrote:
>
> >>>> See also:
> >>>> https://lore.kernel.org/cocci/[email protected]/
> >>>> https://sympa.inria.fr/sympa/arc/cocci/2023-03/msg00115.html
> >>>
> >>> I will queue cleanup patches 1-2,
> >>
> >> Thanks for this positive feedback.
> >
> > Sorry, these patches do not apply to master branch and patch 1
> > is no longer correct in master branch and the new mount api changes.
>
> Do you want that I adapt the linked development ideas to the current situation
> a bit more?
>
No thanks.
Patch 1 just doesn't work for the new mount api.
>
> >>> but I do not like patches 3/4 and 4/4.
> >>> I do not think that they make the code better to read or maintain.
> >>
> >> I would appreciate if the details for such change reluctance can be clarified better.
> >
> > patch 3:
> > I much rather a single error handling label that takes care of
> > all the cleanups - it is harder to make mistakes and jump to
> > the wrong label when adding new error conditions.
>
> There are different coding style preferences involved.
>
> See also:
> https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources
>
As long as coding styles are not mandatory
I prefer what we have right now.
>
> > patch 4:
> > Overlayfs uses this coding style all over the place
> >
> > err = -ENOMEM;
> > ofs->creator_cred = cred = prepare_creds();
> > if (!cred)
> > goto out_free_ofs;
> >
> > I don't see the benefit in making err = -ENOMEM conditional.
> > I don't see the style after your patch as clearly better than before.
>
> Can it be nicer to set error codes only in exceptional data processing situations?
>
It's a matter of taste.
I'll stay with what we have.
Thanks,
Amir.