Re: [Cocci] [PATCH v3] coccinelle: semantic patch for missing put_device()

Re: [Cocci] [PATCH v3] coccinelle: semantic patch for missing put_device()

[Markus Elfring]

> Reviewed-by: Markus Elfring <Markus.Elfring@web.de>

I have got doubts that my code review comments qualify already
for this tag.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=1f947a7a011fcceb14cb912f5481a53b18f1879a#n565


> Cc: cocci@systeme.lip6.fr

I assume that Masahiro Yamada will also need to get explicitly informed
for a possible patch integration.


> v3->v2:
> - reduction of a bit of redundant C code within SmPL search specifications.
> - consider the message construction without using the extra Python variable “msg”

I find it nice that you would like to take this information into account.


> +    when != e4 = (T1)platform_get_drvdata(id)
> +(
> +

Should a blank line be omitted at such a source code place?


> +  return
> +(    id
> +|    (T2)dev_get_drvdata(&id->dev)
> +|    (T3)platform_get_drvdata(id)
> +);

It seems that you would like to express a different coding style.
Would anybody like to reconsider the position once more for semicolons
in nested disjunctions for such a SmPL search specification?


> +coccilib.report.print_report(p2[0], "ERROR: missing put_device; of_find_device_by_node on line " + p1[0].line + " and return without releasing.")

Your willingness for such a rearrangement is interesting.
How do you think about to move the long message parameter to a subsequent line?

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v3] Coccinelle: semantic patch for missing put_device()

[Markus Elfring]

> The of_find_device_by_node() takes a reference to the underlying device
> structure, we should release that reference.

I have got another concern for further software development considerations.

How do you think about to describe here if it can be determined
by source code analysis that the desired release should be performed
only in the same function implementation (or not)?

How much does this aspect influence the source code search confidence?


> +    when != e1 = (T)id
> +    when != e2 = &id->dev
> +    when != e3 = get_device(&id->dev)
> +    when != e4 = (T1)platform_get_drvdata(id)

I have got another idea for a bit of software fine-tuning at such a place.
I am unsure if it can become relevant to reduce the number of metavariables
here by introducing a SmPL disjunction.

+    when != ex = \( (T)id \| &id->dev \| get_device(&id->dev) \| (T1)platform_get_drvdata(id) \)


Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH v3] Coccinelle: semantic patch for missing put_device()

[Julia Lawall]

On Wed, 13 Feb 2019, Markus Elfring wrote:

> > The of_find_device_by_node() takes a reference to the underlying device
> > structure, we should release that reference.
>
> I have got another concern for further software development considerations.
>
> How do you think about to describe here if it can be determined
> by source code analysis that the desired release should be performed
> only in the same function implementation (or not)?
>
> How much does this aspect influence the source code search confidence?
>
>
> > +    when != e1 = (T)id
> > +    when != e2 = &id->dev
> > +    when != e3 = get_device(&id->dev)
> > +    when != e4 = (T1)platform_get_drvdata(id)
>
> I have got another idea for a bit of software fine-tuning at such a place.
> I am unsure if it can become relevant to reduce the number of metavariables
> here by introducing a SmPL disjunction.
>
> +    when != ex = \( (T)id \| &id->dev \| get_device(&id->dev) \| (T1)platform_get_drvdata(id) \)

There is no need for the disjunction.  There is also no need for the
different variables.  Different variables are only needed when the when
conditions are on different ...s

julia

>
>
> Regards,
> Markus
>
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [v3] Coccinelle: semantic patch for missing put_device()

[Markus Elfring]

>> +    when != ex = \( (T)id \| &id->dev \| get_device(&id->dev) \| (T1)platform_get_drvdata(id) \)
>
> There is no need for the disjunction.  There is also no need for the
> different variables.

Really?

Would you like to distinguish the shown assignment expressions anyhow?


> Different variables are only needed when the when conditions are on
> different ...s

I find that this information will need further clarification.
Will any additional adjustments become relevant for such SmPL when specifications?

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [v3] coccinelle: semantic patch for missing put_device()

[Markus Elfring]

> However, when we tried the following style, we encountered a parse error.

I am sorry that my refactoring proposal did not completely fit to the applied
software version.
I am still curious if a development topic like “Support for SmPL disjunctions
on every token” will evolve further.
https://github.com/coccinelle/coccinelle/issues/40

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci