Re: [B.A.T.M.A.N.] [PATCH 2/2] batman-adv: Fix kernel-doc for timer functions

[Julia Lawall <julia.lawall@lip6.fr>]

[-- Attachment #1: Type: text/plain, Size: 3173 bytes --]



On Fri, 1 Dec 2017, Kees Cook wrote:

> On Fri, Dec 1, 2017 at 2:47 AM, Sven Eckelmann <sven@narfation.org> wrote:
> > The commit e99e88a9d2b0 ("treewide: setup_timer() -> timer_setup()")
> > changed the argument name and type of the timer function but didn't adjust
> > the kernel-doc of these functions.
> >
> > Signed-off-by: Sven Eckelmann <sven@narfation.org>
> > ---
> > Cc: Kees Cook <keescook@chromium.org>
>
> Acked-by: Kees Cook <keescook@chromium.org>
>
> I wonder if there is a way for Coccinelle to change kernel-doc?

It can't change it, but with some cleverness (ie python/ocaml code) it can
be used to find problems.  I've attached a semantic patch that I wrote for
this.  It gives reports like:

drivers/acpi/arm64/iort.c:864 dma_size doesn't appear in ids: dev dma_addr size

Indeed the code has:

/**
 * iort_dma_setup() - Set-up device DMA parameters.
 *
 * @dev: device to configure
 * @dma_addr: device DMA address result pointer
 * @size: DMA range size result pointer
 */
void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size)

So the kerneldoc has the wrong name.  There are also things like this:

drivers/acpi/acpica/utdebug.c:617 acpi_trace_point doesn't match preceding
comment: begin

Here the code is:

/******************************************************************************\
*
 *
 * FUNCTION:    acpi_trace_point
 *
 * PARAMETERS:  type                - Trace event type
 *              begin               - TRUE if before execution
 *              aml                 - Executed AML address
 *              pathname            - Object path
 *              pointer             - Pointer to the related object
 *
 * RETURN:      None
 *
 * DESCRIPTION: Interpreter execution trace.
 *

******************************************************************************\
/

void
acpi_trace_point(acpi_trace_event_type type, u8 begin, u8 *aml, char *pathname)

So the rule doesn't seem to know about this kind of documentation.

julia

>
> -Kees
>
> > ---
> >  net/batman-adv/tp_meter.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/batman-adv/tp_meter.c b/net/batman-adv/tp_meter.c
> > index 15cd213..ebc4e22 100644
> > --- a/net/batman-adv/tp_meter.c
> > +++ b/net/batman-adv/tp_meter.c
> > @@ -482,7 +482,7 @@ static void batadv_tp_reset_sender_timer(struct batadv_tp_vars *tp_vars)
> >
> >  /**
> >   * batadv_tp_sender_timeout - timer that fires in case of packet loss
> > - * @arg: address of the related tp_vars
> > + * @t: address to timer_list inside tp_vars
> >   *
> >   * If fired it means that there was packet loss.
> >   * Switch to Slow Start, set the ss_threshold to half of the current cwnd and
> > @@ -1106,7 +1106,7 @@ static void batadv_tp_reset_receiver_timer(struct batadv_tp_vars *tp_vars)
> >  /**
> >   * batadv_tp_receiver_shutdown - stop a tp meter receiver when timeout is
> >   *  reached without received ack
> > - * @arg: address of the related tp_vars
> > + * @t: address to timer_list inside tp_vars
> >   */
> >  static void batadv_tp_receiver_shutdown(struct timer_list *t)
> >  {
> > --
> > 2.11.0
> >
>
>
>
> --
> Kees Cook
> Pixel Security
>

[-- Attachment #2: Type: text/plain, Size: 4348 bytes --]

@initialize:ocaml@
@@

let tbl = ref []
let fnstart = ref []
let success = Hashtbl.create 101
let thefile = ref ""
let parsed = ref []
let nea = ref []

let parse file =
  thefile := List.nth (Str.split (Str.regexp "linux-next/") file) 1;
  let i = open_in file in
  let startline = ref 0 in
  let fn = ref "" in
  let ids = ref [] in
  let rec inside n =
    let l = input_line i in
    let n = n + 1 in
    match Str.split_delim (Str.regexp_string "*/") l with
      before::after::_ ->
	(if not (!fn = "")
	then tbl := (!startline,n,!fn,List.rev !ids)::!tbl);
	startline := 0;
	fn := "";
	ids := [];
	outside n
    | _ ->
	(match Str.split (Str.regexp "[ \t]+") l with
	  "*"::name::rest ->
	    let len = String.length name in
	    (if !fn = "" && len > 2 && String.sub name (len-2) 2 = "()"
	    then fn := String.sub name 0 (len-2)
	    else if !fn = "" && (not (rest = [])) && List.hd rest = "-"
	    then
	      if String.get name (len-1) = ':'
	      then fn := String.sub name 0 (len-1)
	      else fn := name
	    else if not(!fn = "") && len > 2 &&
	      String.get name 0 = '@' && String.get name (len-1) = ':'
	    then ids := (String.sub name 1 (len-2)) :: !ids);
	| _ -> ());
	inside n
  and outside n =
    let l = input_line i in
    let n = n + 1 in
    if String.length l > 2 && String.sub l 0 3 = "/**"
    then
      begin
	startline := n;
	inside n
      end
    else outside n in
  try outside 0 with End_of_file -> ()

let hashadd tbl k v =
  let cell =
    try Hashtbl.find tbl k
    with Not_found ->
      let cell = ref [] in
      Hashtbl.add tbl k cell;
      cell in
  cell := v :: !cell

@script:ocaml@
@@

tbl := [];
fnstart := [];
Hashtbl.clear success;
parsed := [];
nea := [];
parse (List.hd (Coccilib.files()))

@r@
identifier f;
position p;
@@

f@p(...) { ... }

@script:ocaml@
p << r.p;
f << r.f;
@@

parsed := f :: !parsed;
fnstart := (List.hd p).line :: !fnstart

@param@
identifier f;
type T;
identifier i;
parameter list[n] ps;
parameter list[n1] ps1;
position p;
@@

f@p(ps,T i,ps1) { ... }

@script:ocaml@
@@

tbl := List.rev (List.sort compare !tbl)

@script:ocaml@
p << param.p;
f << param.f;
@@

let myline = (List.hd p).line in
let prevline =
  List.fold_left
    (fun prev x ->
      if x < myline
      then max x prev
      else prev)
    0 !fnstart in
let _ =
  List.exists
    (function (st,fn,nm,ids) ->
      if prevline < st && myline > st && prevline < fn && myline > fn
      then
	begin
	  (if not (String.lowercase f = String.lowercase nm)
	  then
	    Printf.printf "%s:%d %s doesn't match preceding comment: %s\n"
	      !thefile myline f nm);
	  true
	end
      else false)
    !tbl in
()

@script:ocaml@
p << param.p;
n << param.n;
n1 << param.n1;
i << param.i;
f << param.f;
@@

let myline = (List.hd p).line in
let prevline =
  List.fold_left
    (fun prev x ->
      if x < myline
      then max x prev
      else prev)
    0 !fnstart in
let _ =
  List.exists
    (function (st,fn,nm,ids) ->
      if prevline < st && myline > st && prevline < fn && myline > fn
      then
	begin
	  (if List.mem i ids then hashadd success (st,fn,nm) i);
	  (if ids = [] (* arg list seems not obligatory *)
	  then ()
	  else if not (List.mem i ids)
	  then
	    Printf.printf "%s:%d %s doesn't appear in ids: %s\n"
	      !thefile myline i (String.concat " " ids)
	  else if List.length ids <= n || List.length ids <= n1
	  then
	    (if not (List.mem f !nea)
	    then
	      begin
		nea := f :: !nea;
		Printf.printf "%s:%d %s not enough args\n" !thefile myline f;
	      end)
	  else
	    let foundid = List.nth ids n in
	    let efoundid = List.nth (List.rev ids) n1 in
	    if not(foundid = i || efoundid = i)
	    then
	      Printf.printf "%s:%d %s wrong arg in position %d: %s\n"
		!thefile myline i n foundid);
	  true
	end
      else false)
    !tbl in
()

@script:ocaml@
@@
List.iter
  (function (st,fn,nm,ids) ->
    if List.mem nm !parsed
    then
      let entry =
	try !(Hashtbl.find success (st,fn,nm))
	with Not_found -> [] in
      List.iter
	(fun id ->
	  if not (List.mem id entry) && not (id = "...")
	  then Printf.printf "%s:%d %s not used\n" !thefile st id)
	ids)
  !tbl