From: <wen.yang99@zte.com.cn>
To: <Markus.Elfring@web.de>
Cc: wang.yi59@zte.com.cn, michal.lkml@markovi.net,
nicolas.palix@imag.fr, linux-kernel@vger.kernel.org,
cocci@systeme.lip6.fr
Subject: [Cocci] 答复: Re: [PATCH v2] coccinelle: semantic code search for missingof_node_put
Date: Thu, 4 Jul 2019 10:41:05 +0800 (CST) [thread overview]
Message-ID: <201907041041053843118@zte.com.cn> (raw)
In-Reply-To: <904b9362-cd01-ffc9-600b-0c48848617a0@web.de>
[-- Attachment #1.1: Type: text/plain, Size: 5980 bytes --]
> > The counter must be decremented after the last usage of a device node.
>
> Thanks for your next try to improve the software situation
> also in this area.
>
>
> > We find these functions by using the following SmPL:
>
> Would it be nicer to use the word “script” also here?
>
OK, we will replace "SmPL" with "scrilt" later.
>
> > <SmPL>
> > @initialize:ocaml@
> > @@
>
> How do you think about to describe the chosen algorithm
> in a way for contributors who might not be so familiar with
> this programming language?
>
> Will any information from previous discussions become relevant
> for a better commit description?
>
We will also provide an example written in Python later.
>
> > let relevant_str = "use of_node_put() on it when done"
>
> Will such a literal need further development and software documentation considerations?
>
>
> > let contains s1 s2 =
> > let re = Str.regexp_string s2
> > in
> > try ignore (Str.search_forward re s1 0); true
> > with Not_found -> false
> >
> > let relevant_functions = ref []
> >
> > let add_function f c =
> > if not (List.mem f !relevant_functions)
> > then
> > begin
> > let s = String.concat " "
>
> I find such a concatenation suspicious after the space character
> is used also for a string splitting before.
> Can this delimiter be omitted for the combination?
>
We first divide the comment by word, then use spaces to concatenate
them into a string, and finally find the substring (use of_node_put() on it when done)
in this string.
>
> > (
> > (List.map String.lowercase_ascii
> > (List.filter
> > (function x ->
> > Str.string_match
> > (Str.regexp "[a-zA-Z_\\(\\)][-a-zA-Z0-9_\\(\\)]*$")
> > x 0) (Str.split (Str.regexp "[ .;\t\n]+") c)))) in
> > if contains s relevant_str
>
> I would prefer to use the string constant in the called function directly
> instead of passing it as another parameter.
Thanks. Here the relevant_str is a string constant.
> > then
> > Printf.printf "Found relevant function: %s\n" f;
> > relevant_functions := f :: !relevant_functions;
> > end
>
> I find your choice for an output format unclear at the moment.
> I imagine that the corresponding data processing of these function names
> will need fine-tuning.
> I am missing the software component for the conversion of this
> identifier list into a disjunction for the SmPL rule “r1”.
Thanks.
We first use this script to find out all the function names to be processed,
and then copy these function names into r1.
> > And this patch also looks for places where an of_node_put()
>
> Does a patch or a script perform an action?
>
OK, Thanks.
We'll fix it soon.
> > call is on some paths but not on others.
>
> Let us look at mentioned implementation details.
>
>
> > +@initialize:python@
> > +@@
> > +
> > +seen = set()
> > +
> > +def add_if_not_present (p1, p2):
>
> It seems that you would like to use iteration functionality.
> https://github.com/coccinelle/coccinelle/blob/99e081e9b89d49301b7bd2c5e5aac88c66eaaa6a/docs/manual/cocci_syntax.tex#L1826
>
> How will it matter here?
>
>
> > +def display_report(p1, p2):
> > + if add_if_not_present(p1[0].line, p2[0].line):
> > + coccilib.report.print_report(p2[0],
> > + "ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line "
> > + + p1[0].line
> > + + ", but without a corresponding object release within this function.")
> > +
> > +def display_org(p1, p2):
> > + cocci.print_main("acquired a node pointer with refcount incremented", p1)
> > + cocci.print_secs("needed of_node_put", p2)
>
> Can it be helpful to specify SmPL dependencies for these functions
> according to the applied operation mode?
>
>
> > +x = @p1\(of_find_all_nodes\|
>
> I would find this SmPL disjunction easier to read without the usage
> of extra backslashes.
>
> +x =
> +(of_…
> +|of_…
> +)@p1(...);
>
>
> Which sort criteria were applied for the generation of the shown
> function name list?
As julia pointed out, your current writing is not compiled.
> > +if (x == NULL || ...) S
> > +... when != e = (T)x
> > + when != true x == NULL
>
> I wonder if this code exclusion specification is really required
> after a null pointer was checked before.
Thanks.
Our previous version used the "when any" clause, so we need
"when != true x == NULL". We can delete this code exclusion
specification for this version.
We will fix it later.
> > +|
> > +return x;
> > +|
> > +return of_fwnode_handle(x);
>
> Can a nested SmPL disjunction be helpful at such places?
>
> +|return
> +(x
> +|of_fwnode_handle(x)
> +);
We are more in agreement with julia's comments:
The original code is much more readable. The internal representation will be the same.
> > + when != v4l2_async_notifier_add_fwnode_subdev(<...x...>)
>
> Would the specification variant “<+... x ...+>” be relevant
> for the parameter selection?
Thanks.
We use <... x ...> instead of <+... x ...+> here to eliminate the following false positives:
./drivers/media/platform/qcom/camss/camss.c:504:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 479, but without a corresponding object release within this function.
465 static int camss_of_parse_ports(struct camss *camss)
466 {
...
479 remote = of_graph_get_remote_port_parent(node);
...
486 asd = v4l2_async_notifier_add_fwnode_subdev(
487 &camss->notifier, of_fwnode_handle(remote), ---> v4l2_async_notifier_add_fwnode_subdev will pass remote to camss->notifier.
488 sizeof(*csd));
...
504 return num_subdevs;
--
Regards,
Wen
[-- Attachment #2: Type: text/plain, Size: 136 bytes --]
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
next prev parent reply other threads:[~2019-07-04 2:42 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-28 2:58 [Cocci] [PATCH v2] coccinelle: semantic code search for missing of_node_put Wen Yang
2019-06-28 9:38 ` Markus Elfring
2019-06-28 11:07 ` Julia Lawall
2019-06-28 14:16 ` [Cocci] [v2] " Markus Elfring
2019-06-28 14:16 ` Markus Elfring
2019-07-04 3:03 ` [Cocci] [PATCH v2] coccinelle: semantic code search for missingof_node_put wen.yang99
2019-07-04 6:28 ` [Cocci] [v2] coccinelle: semantic code search for missing of_node_put Markus Elfring
2019-07-05 5:29 ` [Cocci] [PATCH v2] coccinelle: semantic code search for missingof_node_put Julia Lawall
2019-07-05 5:57 ` [Cocci] [PATCH v2] coccinelle: semantic code search formissingof_node_put wen.yang99
2019-07-05 6:17 ` Julia Lawall
2019-07-05 6:45 ` [Cocci] [v2] coccinelle: semantic code search for missing of_node_put Markus Elfring
2019-06-29 7:40 ` Markus Elfring
2019-06-29 7:49 ` Julia Lawall
2019-06-29 8:35 ` Markus Elfring
2019-06-29 19:30 ` [Cocci] [v2] Coccinelle: Testing SmPL constraints Markus Elfring
2019-07-04 2:41 ` wen.yang99 [this message]
2019-07-04 5:40 ` [Cocci] [v2] coccinelle: semantic code search for missing of_node_put Markus Elfring
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201907041041053843118@zte.com.cn \
--to=wen.yang99@zte.com.cn \
--cc=Markus.Elfring@web.de \
--cc=cocci@systeme.lip6.fr \
--cc=linux-kernel@vger.kernel.org \
--cc=michal.lkml@markovi.net \
--cc=nicolas.palix@imag.fr \
--cc=wang.yi59@zte.com.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).