On Mon, 16 May 2011, Jim Cromie wrote:
> On Sun, May 15, 2011 at 2:04 AM, Julia Lawall <[email protected]> wrote:
> > What are the two places you want to change?
>
> 2 kinds of changes: (both to drivers/tty/tty_io.c: tty_init)
>
> 1 register_chardev_region --> register_chardev_ids
> with that was MKDEV --> &devt_X
>
> That transform is working.
> using ++ instead of +, I was able to get this out,
> which is useful as starting point for hand editing.
>
> One thing Id like to add is a _0 and _1 suffix
> to the 2 new varnames, using the bound values of minor
> would be useful in this case.
>
> int __init tty_init(void)
> {
> + dev_t devt;
> + devt = MKDEV(TTYAUX_MAJOR, 0);
> + dev_t devt;
> + devt = MKDEV(TTYAUX_MAJOR, 1);
> + dev_t devt;
> + devt = devt;
> + dev_t devt;
> + devt = devt;
It's a bit unfortunate that the declaration and the initialization come
out interleaved like this. Perhaps what you want is the following
@@
identifier f;
fresh identifier devt = "devt";
statement S,S1;
@@
f(...) {
++ dev_t devt;
... when != S
++ devt = ...; // ... is whatever you want the initial value to be
S1
...
}
The only problem is that if s1 is going to represent the first statement
of the function, then you can't match the first statement of the function
in another way, if that is necessary for your rule. You may need to have
one rule to do the matching, and another that inherits various information
to do this part of the transformation. That might be useful anyway for
making a fresh metavaraible with the right name.
>
>
> 2 the 2nd KIND of change: MKDEV(...) --> devt
>
> there are other occurrences of MKDEV in tty_init(),
> which shouldnt have '&' in the replacement
>
> In the patch below, these added changes were done by a 2nd,
> separate rule which was not constrained by 'register_chrdev_region',
> but depended upon it, so it wasnt touching things inappropriately.
> This is probably the better way to do it.
>
> cdev_init(&tty_cdev, &tty_fops);
> - if (cdev_add(&tty_cdev, MKDEV(TTYAUX_MAJOR, 0), 1) ||
> - register_chrdev_region(MKDEV(TTYAUX_MAJOR, 0), 1, "/dev/tty") < 0)
> + if (cdev_add(&tty_cdev, devt, 1) ||
> + register_chrdev_ids(&devt, 1, "/dev/tty") < 0)
> panic("Couldn't register /dev/tty driver\n");
> - device_create(tty_class, NULL, MKDEV(TTYAUX_MAJOR, 0), NULL, "tty");
> + device_create(tty_class, NULL, devt, NULL, "tty");
>
> cdev_init(&console_cdev, &console_fops);
> - if (cdev_add(&console_cdev, MKDEV(TTYAUX_MAJOR, 1), 1) ||
> - register_chrdev_region(MKDEV(TTYAUX_MAJOR, 1), 1, "/dev/console") <
> 0)
> + if (cdev_add(&console_cdev, devt, 1) ||
> + register_chrdev_ids(&devt, 1, "/dev/console") < 0)
> panic("Couldn't register /dev/console driver\n");
> - consdev = device_create(tty_class, NULL, MKDEV(TTYAUX_MAJOR, 1), NULL,
> + consdev = device_create(tty_class, NULL, devt, NULL,
> "console");
> if (IS_ERR(consdev))
> consdev = NULL;
> Note: processing took 93.4s: drivers/tty/tty_io.c
>
>
> Again, the possible improvements are
> - single var decl at top,
> - multiple assignments to it, just above 1st use.
> Making this not do harm is nice but ultimately overkill,
> I need to review before applying anyway.
>
>
> > It is never useful to put <+... ...+> around an entire rule. That is what
> > the semantics of rule application already does naturally.
>
> OK. that makes sense.
> The reason they were in there was due to my attempt to also convert
> the 2nd Kind of MKDEV in the same rule.
>
> the inner <+... ...+> was trying to change them, using the outer
> transform as an anchor. Part of my thinking here was that I wanted
> to insert assgments to the devt var just above the 2 if()s where
> the MKDEVs were.
This would be fine if you know there is going to be an if there. Perhaps
you could have that as a special case, and then for other cases just put
the initialization at the top of the function.
julia
> ie to produce this:
>
> int __init tty_init(void)
> {
> + dev_t devt;
> cdev_init(&tty_cdev, &tty_fops);
> + devt = MKDEV(TTYAUX_MAJOR, 0);
> - if (cdev_add(&tty_cdev, MKDEV(TTYAUX_MAJOR, 0), 1) ||
> - register_chrdev_region(MKDEV(TTYAUX_MAJOR, 0), 1, "/dev/tty") < 0)
> + if (cdev_add(&tty_cdev, devt, 1) ||
> + register_chrdev_ids(&devt, 1, "/dev/tty") < 0)
> panic("Couldn't register /dev/tty driver\n");
> - device_create(tty_class, NULL, MKDEV(TTYAUX_MAJOR, 0), NULL, "tty");
> + device_create(tty_class, NULL, devt, NULL, "tty");
>
>
> cdev_init(&console_cdev, &console_fops);
> + devt = MKDEV(TTYAUX_MAJOR, 1);
> - if (cdev_add(&console_cdev, MKDEV(TTYAUX_MAJOR, 1), 1) ||
> - register_chrdev_region(MKDEV(TTYAUX_MAJOR, 1), 1, "/dev/console") <
> 0)
> + if (cdev_add(&console_cdev, devt, 1) ||
> + register_chrdev_ids(&devt, 1, "/dev/console") < 0)
> panic("Couldn't register /dev/console driver\n");
> - consdev = device_create(tty_class, NULL, MKDEV(TTYAUX_MAJOR, 1), NULL,
> + consdev = device_create(tty_class, NULL, devt, NULL,
> "console");
> if (IS_ERR(consdev))
>
> >
> > If you want to add at declaration at the very top of a function, make a
> > rule that looks like the function definition and put the declaration
> > there. Eg:
>
> ack. I used this , with ++, to insert multiple declarations above.
>
> > Unfortunately, I'm not on line very much today.
> >
> > julia
>
> Thanks for all your help.
>
> At this point, Im happy with my results. While further refinements to my
> rules are likely possible, and useful for my spatch-fu, I can quickly
> hand edit the useful changes, reset the others.
>
> If anything about my former (current?) confusion is interesting,
> please ask, otherwise thanks for a powerful tool, and the personal help.
>
> jimc
>
> btw, my cocci input
>
> @ rcr_md @
> identifier f;
> expression major, minor;
> expression ct, name;
> @@
>
> f(...) {
> ++ dev_t devt;
> ++ devt = MKDEV(major,minor);
>
> <+...
> - register_chrdev_region
> + register_chrdev_ids
> (
> - MKDEV(major,minor),
> + &devt,
> ct, name)
> ...+>
>
> }
>
> @ all_md depends on rcr_md @ // where above changes made, also do
> identifier f;
> expression major, minor;
> @@
>
> f(...) {
> dev_t devt;
> devt = MKDEV(major,minor);
>
> <+...
> - MKDEV(major,minor)
> + devt
> ...+>
> }
> _______________________________________________
Cocci mailing list
[email protected]
http://lists.diku.dk/mailman/listinfo/cocci
(Web access from inside DIKUs LAN only)
