From: Márton Németh <[email protected]>
The id_table field of the struct usb_device_id is constant in <linux/usb.h>
so it is worth to make the initialization data also constant.
The semantic match that finds this kind of pattern is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@r@
disable decl_init,const_decl_init;
identifier I1, I2, x;
@@
struct I1 {
...
const struct I2 *x;
...
};
@s@
identifier r.I1, y;
identifier r.x, E;
@@
struct I1 y = {
.x = E,
};
@c@
identifier r.I2;
identifier s.E;
@@
const struct I2 E[] = ... ;
@depends on !c@
identifier r.I2;
identifier s.E;
@@
+ const
struct I2 E[] = ...;
// </smpl>
Signed-off-by: Márton Németh <[email protected]>
Cc: Julia Lawall <[email protected]>
Cc: [email protected]
---
diff -u -p a/drivers/uwb/hwa-rc.c b/drivers/uwb/hwa-rc.c
--- a/drivers/uwb/hwa-rc.c 2009-12-03 04:51:21.000000000 +0100
+++ b/drivers/uwb/hwa-rc.c 2010-01-08 11:17:49.000000000 +0100
@@ -891,7 +891,7 @@ static int hwarc_post_reset(struct usb_i
}
/** USB device ID's that we handle */
-static struct usb_device_id hwarc_id_table[] = {
+static const struct usb_device_id hwarc_id_table[] = {
/* D-Link DUB-1210 */
{ USB_DEVICE_AND_INTERFACE_INFO(0x07d1, 0x3d02, 0xe0, 0x01, 0x02),
.driver_info = WUSB_QUIRK_WHCI_CMD_EVT },
diff -u -p a/drivers/uwb/i1480/dfu/usb.c b/drivers/uwb/i1480/dfu/usb.c
--- a/drivers/uwb/i1480/dfu/usb.c 2010-01-07 19:08:47.000000000 +0100
+++ b/drivers/uwb/i1480/dfu/usb.c 2010-01-08 11:18:26.000000000 +0100
@@ -430,7 +430,7 @@ error:
/** USB device ID's that we handle */
-static struct usb_device_id i1480_usb_id_table[] = {
+static const struct usb_device_id i1480_usb_id_table[] = {
i1480_USB_DEV(0x8086, 0xdf3b),
i1480_USB_DEV(0x15a9, 0x0005),
i1480_USB_DEV(0x07d1, 0x3802),
Németh Márton wrote:
> From: Márton Németh <[email protected]>
>
> The id_table field of the struct usb_device_id is constant in <linux/usb.h>
> so it is worth to make the initialization data also constant.
Thanks.
> The semantic match that finds this kind of pattern is as follows:
> (http://coccinelle.lip6.fr/)
Can you please not include this as part of the changelog in future? You
may include it after the '---' if you wish.
David
--
David Vrabel, Senior Software Engineer, Drivers
CSR, Churchill House, Cambridge Business Park, Tel: +44 (0)1223 692562
Cowley Road, Cambridge, CB4 0WZ http://www.csr.com/
Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
David Vrabel írta:
> Németh Márton wrote:
>> From: Márton Németh <[email protected]>
>>
>> The id_table field of the struct usb_device_id is constant in <linux/usb.h>
>> so it is worth to make the initialization data also constant.
>
> Thanks.
>
>> The semantic match that finds this kind of pattern is as follows:
>> (http://coccinelle.lip6.fr/)
>
> Can you please not include this as part of the changelog in future? You
> may include it after the '---' if you wish.
It is common for patches generated by spatch that they include the SmPL
which was used to generate the patch.
See http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Ftorvalds%2Flinux-2.6.git&a=search&h=HEAD&st=commit&s=%3Csmpl%3E
But you can leave it out, if you wish.
Regards,
Márton Németh
Németh Márton wrote:
> David Vrabel írta:
>> Can you please not include this as part of the changelog in future? You
>> may include it after the '---' if you wish.
>
> It is common for patches generated by spatch that they include the SmPL
> which was used to generate the patch.
> See http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Ftorvalds%2Flinux-2.6.git&a=search&h=HEAD&st=commit&s=%3Csmpl%3E
>
> But you can leave it out, if you wish.
I for one agree with David that this does not belong into the change
log. Other maintainers just don't care or disagree.
If it is commonly seen, then only because Julia started it that way and
others blindly mimic her changelogs without giving it any thought, and
because many committers didn't edit or reject that for one reason or
another --- e.g. commit volume and work flow.
When you write a changelog, keep your audience in mind:
- Developers, distributors, advanced users want to learn what a new
release brings. (OK, this audience stops reading after the initial
headline of a "make XYZ table constant" commit. Which just means
that all the rest of the changelog is fluff that can be omitted.)
- Developers, maintainers etc. want to understand years later why the
code is how it is. (For them, a commit like that is sufficiently
described by the headline as well.)
- Reviewers and committers want to quickly see whether a submission
is worthwhile and can work the way it is proposed. (For them, a
patch submission like this one is well described by the headline
plus the hint that struct usb_driver.id_table is a const *. BTW,
the respective sentence in your changelog got the types wrong.)
The only people who may be interested in your find-and-replace macro are
those who write and use such macros themselves. But for them, the more
appropriate source of information are the mailing lists and list
archives rather than the SCM's changelog.
The idea to note in the changelog with which editor or by which
find-and-replace macro a patch was generated is so way out there that it
is not even found as a negative example in "The Perfect Patch" section 4:
http://web.archive.org/web/20080722025711/http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt
End rant. :-)
--
Stefan Richter
-=====-==-=- ---= -==-=
http://arcgraph.de/sr/
> When you write a changelog, keep your audience in mind:
>
> - Developers, distributors, advanced users want to learn what a new
> release brings. (OK, this audience stops reading after the initial
> headline of a "make XYZ table constant" commit. Which just means
> that all the rest of the changelog is fluff that can be omitted.)
>
> - Developers, maintainers etc. want to understand years later why the
> code is how it is. (For them, a commit like that is sufficiently
> described by the headline as well.)
Not surprisingly, I don't agree about this one. I recall a series of
patches that said something like "used a script to change down/up to
mutexes". The script wasn't included, not all down/ups were changed to
mutexes, and in short there was no understandable trace of why the change
was made where it was. Perhaps that is a pathological example, but it is
not necessarily obvious in advance what needs precise documentation and
what does not.
julia
Julia Lawall wrote:
>> When you write a changelog, keep your audience in mind:
>>
>> - Developers, distributors, advanced users want to learn what a new
>> release brings. (OK, this audience stops reading after the initial
>> headline of a "make XYZ table constant" commit. Which just means
>> that all the rest of the changelog is fluff that can be omitted.)
>>
>> - Developers, maintainers etc. want to understand years later why the
>> code is how it is. (For them, a commit like that is sufficiently
>> described by the headline as well.)
>
> Not surprisingly, I don't agree about this one. I recall a series of
> patches that said something like "used a script to change down/up to
> mutexes". The script wasn't included,
This "used a script to change down/up to mutexes" surely was meant to
convey that the submitter performed a mass change on code that he is not
particularly familiar with or frequently works with himself, i.e. it was
a mostly mechanical change in contrast to how proposed changes are
usually developed.
Semaphore to mutex conversions are/have been not as trivial as for
example a marking of an ID table as const. Hence those hints that this
was a mostly mechanical conversion (certainly with some checking
afterwards) usefully put the submission into perspective for reviewers.
This information may also be useful after commit, hence it is good for
the SCM's changelog.
The find-and-replace macro itself and the processor which ran that macro
during the development of such a sem2mutex change is entirely uninteresting.
> not all down/ups were changed to mutexes,
If that was unexplained, the it was probably an unintended result of the
mechanical conversion by someone who does not have a deeper personal
investment in the affected code, or simply missed something for whatever
reason.
If it was by mistake, inclusion of the find-and-replace script into the
patch posting *after the --- delimiter* might have increased the chance
that a patch reviewer becomes aware of a possible error source
(inadequate match patterns...). So that could be useful during review
before commit, but not so much if the change is revisited some time
after commit.
> and in short there was no understandable trace of why the change
> was made where it was.
Then that changelog was bad since it missed to document the "Why", which
is one of the most important parts of a changelog.
> Perhaps that is a pathological example, but it is
> not necessarily obvious in advance what needs precise documentation and
> what does not.
Granted, it is subjective. The perfect changelog which is ideal for
every possible audience does not exist.
--
Stefan Richter
-=====-==-=- ---= -==-=
http://arcgraph.de/sr/
On Wed, 13 Jan 2010, Stefan Richter wrote:
> If it was by mistake, inclusion of the find-and-replace script into the
> patch posting *after the --- delimiter* might have increased the chance
> that a patch reviewer becomes aware of a possible error source
> (inadequate match patterns...). So that could be useful during review
> before commit, but not so much if the change is revisited some time
> after commit.
Somewhat tangentially, it's worth mentioning that the comments
appearing after the "---" delimiter exist only in the original patch
submissions, not in the final commits. Hence they are not available to
anyone reviewing the changes after acceptance.
It would be nice if there was a way to link automatically a git commit
to an archived copy of the email message in which it was originally
submitted.
Alan Stern
On Wed, Jan 13, 2010 at 12:29:07PM -0500, Alan Stern wrote:
> On Wed, 13 Jan 2010, Stefan Richter wrote:
>
> > If it was by mistake, inclusion of the find-and-replace script into the
> > patch posting *after the --- delimiter* might have increased the chance
> > that a patch reviewer becomes aware of a possible error source
> > (inadequate match patterns...). So that could be useful during review
> > before commit, but not so much if the change is revisited some time
> > after commit.
>
> Somewhat tangentially, it's worth mentioning that the comments
> appearing after the "---" delimiter exist only in the original patch
> submissions, not in the final commits. Hence they are not available to
> anyone reviewing the changes after acceptance.
>
> It would be nice if there was a way to link automatically a git commit
> to an archived copy of the email message in which it was originally
> submitted.
Ingo has been doing this on some patches by putting a message-id field
in the signed-off-by area showing what lkml message a patch came from.
See git commit id 6432e734c99ed685e3cad72f7dcae4c65008fcab in Linus's
tree as an example of this.
I have no objection if people want to do this for any patches going
through my tree as well.
thanks,
greg k-h
On Wednesday 13 January 2010 06:29:07 pm Alan Stern wrote:
> On Wed, 13 Jan 2010, Stefan Richter wrote:
>
> > If it was by mistake, inclusion of the find-and-replace script into the
> > patch posting *after the --- delimiter* might have increased the chance
> > that a patch reviewer becomes aware of a possible error source
> > (inadequate match patterns...). So that could be useful during review
> > before commit, but not so much if the change is revisited some time
> > after commit.
>
> Somewhat tangentially, it's worth mentioning that the comments
> appearing after the "---" delimiter exist only in the original patch
> submissions, not in the final commits. Hence they are not available to
> anyone reviewing the changes after acceptance.
>
> It would be nice if there was a way to link automatically a git commit
> to an archived copy of the email message in which it was originally
> submitted.
AFAIK x86 folks are using "LKML-Reference:" tag with the message ID for
exactly that purpose (just do 'git log arch/x86' to find such commits).
BTW Personally I see nothing wrong with too verbose commit changelogs,
too sparse changelogs are a much bigger annoyance..
--
Bartlomiej Zolnierkiewicz
On Wed, 13 Jan 2010, Greg KH wrote:
> > It would be nice if there was a way to link automatically a git commit
> > to an archived copy of the email message in which it was originally
> > submitted.
>
> Ingo has been doing this on some patches by putting a message-id field
> in the signed-off-by area showing what lkml message a patch came from.
>
> See git commit id 6432e734c99ed685e3cad72f7dcae4c65008fcab in Linus's
> tree as an example of this.
>
> I have no objection if people want to do this for any patches going
> through my tree as well.
If it has to be included manually by the patch submitter then it isn't
automatic. Not to mention that the submitter would need to know the
email's message-id before the email message was sent!
Something like this should belong in a maintainer's script.
Alan Stern
Bartlomiej Zolnierkiewicz wrote:
> BTW Personally I see nothing wrong with too verbose commit changelogs,
> too sparse changelogs are a much bigger annoyance..
Verbosity on its own does not ensure that the important bits happen to
be in the log though.
If somebody is deliberately verbose, please structure the message so
that the important bits can still be quickly spotted. (Don't waste
people's time.)
However, a submission which can be adequately changelogged by one or two
lines, coupled with something like 30 lines of "and here is the script
that generated it for me" is just plain strange. How is that fitting
for the final immutable history? It's just noise.
--
Stefan Richter
-=====-==-=- ---= -==-=
http://arcgraph.de/sr/
On Wed, Jan 13, 2010 at 18:44, Greg KH <[email protected]> wrote:
> On Wed, Jan 13, 2010 at 12:29:07PM -0500, Alan Stern wrote:
>> On Wed, 13 Jan 2010, Stefan Richter wrote:
>>
>> > If it was by mistake, inclusion of the find-and-replace script into the
>> > patch posting *after the --- delimiter* might have increased the chance
>> > that a patch reviewer becomes aware of a possible error source
>> > (inadequate match patterns...). So that could be useful during review
>> > before commit, but not so much if the change is revisited some time
>> > after commit.
>>
>> Somewhat tangentially, it's worth mentioning that the comments
>> appearing after the "---" delimiter exist only in the original patch
>> submissions, not in the final commits. Hence they are not available to
>> anyone reviewing the changes after acceptance.
>>
>> It would be nice if there was a way to link automatically a git commit
>> to an archived copy of the email message in which it was originally
>> submitted.
>
> Ingo has been doing this on some patches by putting a message-id field
> in the signed-off-by area showing what lkml message a patch came from.
>
> See git commit id 6432e734c99ed685e3cad72f7dcae4c65008fcab in Linus's
> tree as an example of this.
>
> I have no objection if people want to do this for any patches going
> through my tree as well.
While the idea behind this is definitely nice, I don't like the
current implementation: it's not really an `lkml reference', but just
the message ID of the email that contained the patch.
Which means it doesn't contain any reference to an lkml archive, but
instead it casts into `git-stone' the host and domain names of my
private machines at home ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Wed, Jan 13, 2010 at 01:04:52PM -0500, Alan Stern wrote:
> On Wed, 13 Jan 2010, Greg KH wrote:
>
> > > It would be nice if there was a way to link automatically a git commit
> > > to an archived copy of the email message in which it was originally
> > > submitted.
> >
> > Ingo has been doing this on some patches by putting a message-id field
> > in the signed-off-by area showing what lkml message a patch came from.
> >
> > See git commit id 6432e734c99ed685e3cad72f7dcae4c65008fcab in Linus's
> > tree as an example of this.
> >
> > I have no objection if people want to do this for any patches going
> > through my tree as well.
>
> If it has to be included manually by the patch submitter then it isn't
> automatic. Not to mention that the submitter would need to know the
> email's message-id before the email message was sent!
>
> Something like this should belong in a maintainer's script.
Yeah, that would get messy, as usually a patch happens in a different
thread than the original problem occurs, so it would take a lot more
work on my part to try to match things up. If I notice it, I will try
to in the future.
thanks,
greg k-h
Greg KH wrote:
> On Wed, Jan 13, 2010 at 01:04:52PM -0500, Alan Stern wrote:
>> On Wed, 13 Jan 2010, Greg KH wrote:
>>
>>>> It would be nice if there was a way to link automatically a git commit
>>>> to an archived copy of the email message in which it was originally
>>>> submitted.
>>> Ingo has been doing this on some patches by putting a message-id field
>>> in the signed-off-by area showing what lkml message a patch came from.
>>>
>>> See git commit id 6432e734c99ed685e3cad72f7dcae4c65008fcab in Linus's
>>> tree as an example of this.
>>>
>>> I have no objection if people want to do this for any patches going
>>> through my tree as well.
>> If it has to be included manually by the patch submitter then it isn't
>> automatic. Not to mention that the submitter would need to know the
>> email's message-id before the email message was sent!
>>
>> Something like this should belong in a maintainer's script.
>
> Yeah, that would get messy, as usually a patch happens in a different
> thread than the original problem occurs, so it would take a lot more
> work on my part to try to match things up. If I notice it, I will try
> to in the future.
What about introducing a new tag like 'SmPL-Used:'? The used SmPL could
be placed somewhere in the web. This would require only one line in
the changelog: e.g.:
SmPL-Used: http://coccinelle.lip6.fr/sp.php#usb_submit_urb
Usually when an SmPL is created in a general manner it can easily
match 100+ places. These can be combined in groups which for example
belong to one maintainter. That was what I did with adding const to
the different ID tables.
Regards,
M?rton N?meth
On Thu, 14 Jan 2010, N?meth M?rton wrote:
> Greg KH wrote:
> > On Wed, Jan 13, 2010 at 01:04:52PM -0500, Alan Stern wrote:
> >> On Wed, 13 Jan 2010, Greg KH wrote:
> >>
> >>>> It would be nice if there was a way to link automatically a git commit
> >>>> to an archived copy of the email message in which it was originally
> >>>> submitted.
> >>> Ingo has been doing this on some patches by putting a message-id field
> >>> in the signed-off-by area showing what lkml message a patch came from.
> >>>
> >>> See git commit id 6432e734c99ed685e3cad72f7dcae4c65008fcab in Linus's
> >>> tree as an example of this.
> >>>
> >>> I have no objection if people want to do this for any patches going
> >>> through my tree as well.
> >> If it has to be included manually by the patch submitter then it isn't
> >> automatic. Not to mention that the submitter would need to know the
> >> email's message-id before the email message was sent!
> >>
> >> Something like this should belong in a maintainer's script.
> >
> > Yeah, that would get messy, as usually a patch happens in a different
> > thread than the original problem occurs, so it would take a lot more
> > work on my part to try to match things up. If I notice it, I will try
> > to in the future.
>
> What about introducing a new tag like 'SmPL-Used:'? The used SmPL could
> be placed somewhere in the web. This would require only one line in
> the changelog: e.g.:
>
> SmPL-Used: http://coccinelle.lip6.fr/sp.php#usb_submit_urb
There is no guarantee that this URL will be valid 5 or 10 years from now.
julia
Julia Lawall wrote:
> On Thu, 14 Jan 2010, N?meth M?rton wrote:
>> What about introducing a new tag like 'SmPL-Used:'? The used SmPL could
>> be placed somewhere in the web.
[...]
> There is no guarantee that this URL will be valid 5 or 10 years from now.
N?meth et al, what is it with your obsession with your scripts? Just
look them up in your dedicated mailinglist.
The topic of the kernel source changelog is the kernel source.
Also, everybody and his dog seems to be keen on inventing new tag lines
for the changelog. We already have all tag lines that we need for the
kernel development and maintenance processes: Sign-off, Ack, and Cc to
stable.
--
Stefan Richter
-=====-==-=- ---= -===-
http://arcgraph.de/sr/
On Wednesday 13 January 2010 09:24:03 pm N?meth M?rton wrote:
> Greg KH wrote:
> > On Wed, Jan 13, 2010 at 01:04:52PM -0500, Alan Stern wrote:
> >> On Wed, 13 Jan 2010, Greg KH wrote:
> >>>> It would be nice if there was a way to link automatically a git commit
> >>>> to an archived copy of the email message in which it was originally
> >>>> submitted.
> >>>
> >>> Ingo has been doing this on some patches by putting a message-id field
> >>> in the signed-off-by area showing what lkml message a patch came from.
> >>>
> >>> See git commit id 6432e734c99ed685e3cad72f7dcae4c65008fcab in Linus's
> >>> tree as an example of this.
> >>>
> >>> I have no objection if people want to do this for any patches going
> >>> through my tree as well.
> >>
> >> If it has to be included manually by the patch submitter then it isn't
> >> automatic. Not to mention that the submitter would need to know the
> >> email's message-id before the email message was sent!
> >>
> >> Something like this should belong in a maintainer's script.
> >
> > Yeah, that would get messy, as usually a patch happens in a different
> > thread than the original problem occurs, so it would take a lot more
> > work on my part to try to match things up. If I notice it, I will try
> > to in the future.
>
> What about introducing a new tag like 'SmPL-Used:'? The used SmPL could
> be placed somewhere in the web. This would require only one line in
> the changelog: e.g.:
>
> SmPL-Used: http://coccinelle.lip6.fr/sp.php#usb_submit_urb
>
> Usually when an SmPL is created in a general manner it can easily
> match 100+ places. These can be combined in groups which for example
> belong to one maintainter. That was what I did with adding const to
> the different ID tables.
>
Hm, I usually am only marginally interested in how patch was generated,
you may have used a script to 'ed' to generate it for all I care.
I do care however about rationale behind the change and that is what
should go into changelogs.
So if these scripts could go below the "---" separator I'm all for it.
Thanks.
--
Dmitry
On Wed, Jan 13, 2010 at 04:38:22PM +0100, Julia Lawall wrote:
> > When you write a changelog, keep your audience in mind:
> >
> > - Developers, distributors, advanced users want to learn what a new
> > release brings. (OK, this audience stops reading after the initial
> > headline of a "make XYZ table constant" commit. Which just means
> > that all the rest of the changelog is fluff that can be omitted.)
> >
> > - Developers, maintainers etc. want to understand years later why the
> > code is how it is. (For them, a commit like that is sufficiently
> > described by the headline as well.)
>
> Not surprisingly, I don't agree about this one. I recall a series of
> patches that said something like "used a script to change down/up to
> mutexes". The script wasn't included, not all down/ups were changed to
> mutexes, and in short there was no understandable trace of why the change
> was made where it was. Perhaps that is a pathological example, but it is
> not necessarily obvious in advance what needs precise documentation and
> what does not.
I agree with Julia, at least in some cases. Having the semantic patch
present when it's 5 or 10 lines long and clearly explains the intended
change is incredibly valuable for review. Sometimes, the script is the
clearest way to indicate your intent -- I'd much rather see a changelog
that says "s/FOO/BAR/g" than "Change FOO to BAR everywhere".
For example, look at 5d66fe92. The semantic patch is blissfully,
incredibly clear. It makes the sun shine down through the clouds, the
birds sing, and the kzalloc align. It's 10 lines long and is quite
intuitive and explanatory. In that case, I think it's clear that the
spatch is worth the changelog space it takes up, even though the
changelog is larger than the diff by quite a margin.
On the other hand, the patch that started this thread is a
counterexample, to me. The correctness of the constification is only
tenuously attested, for me at least, by the semantic patch. The spatch
is really long compared to the diff, and complicated to read.
So, I think it's a balancing act. I've been very happy to see spatches
in some of Julia's posts, and I've also found some of the more
voluminous spatches in changelogs to be more noise than signal.
-andy
On 15.1.2010 17:49, Németh Márton wrote:
> Hi Marek,
s/Marek/Michal/ :)
> there was a discussion about patches which are generated using the
> tool called spatch. In the changelog the SmPL script was usually
> included, see http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Ftorvalds%2Flinux-2.6.git&a=search&h=HEAD&st=commit&s=%3Csmpl%3E .
> It is useful to store the SmPL scripts because they may find
> problems in the newcoming code also.
>
> In order to run a "check" the spatch tool and the SmPL is also necessary.
> There was an idea to place the used SmPL scripts under the Linux
> kernel source tree so it can move from the changelog but still remain
> for later use. The "check" could be run similar to the tools checkpatch,
> sparse or lockdep.
>
> What do you think where the SmPL scripts can be placed?
Documentation/smpl/$name_of_problem_fixed.cocci? Or maybe better
scripts/smlp/..., if you are going to add some wrapper that runs the
semantic patches on the source tree. Or something like that, a dedicated
subdirectory to store the semantic patches in individual files.
> What do you think the best way would be to introduce some check like this
> in the build environment?
I've only heard about the tool, I haven't used it yet. Does it need to
preprocess and parse source files like sparse does, or can it check C
files without expanding macros and includes? If the former, then let's
extend make C=... to also support spatch. If the latter, then a script
that runs spatch on all *.c files found the tree should be enough. But
as I said, I haven't used the tool, so I don't know what it needs and
what it can offer.
Michal
> Documentation/smpl/$name_of_problem_fixed.cocci? Or maybe better
> scripts/smlp/..., if you are going to add some wrapper that runs the
> semantic patches on the source tree. Or something like that, a dedicated
> subdirectory to store the semantic patches in individual files.
The idea is to make it possible to run them, assuming the user has
installed Coccinelle, so scripts seems better.
> > What do you think the best way would be to introduce some check like this
> > in the build environment?
>
> I've only heard about the tool, I haven't used it yet. Does it need to
> preprocess and parse source files like sparse does, or can it check C
> files without expanding macros and includes?
It doesn't need to preprocess the source files.
thanks,
julia
Andy Isaacson wrote:
> For example, look at 5d66fe92. The semantic patch is blissfully,
> incredibly clear. It makes the sun shine down through the clouds, the
> birds sing, and the kzalloc align. It's 10 lines long and is quite
> intuitive and explanatory. In that case, I think it's clear that the
> spatch is worth the changelog space it takes up, even though the
> changelog is larger than the diff by quite a margin.
5d66fe92 is another example of what I was talking about. The changelog
could have read:
drivers/net : Correct the size argument to kzalloc
lp->rx_skb is for an array of pointers, not of structs.
Found using coccinelle.
Signed-off-by: ...
Tell me again in what way the script is of any importance or of any help
in the permanent SCM changelog after commit.
I agree with you that it is very good to have the script in the patch
posting, but I disagree about the changelog.
--
Stefan Richter
-=====-==-=- ---= -====
http://arcgraph.de/sr/
From: Stefan Richter <[email protected]>
Date: Fri, 15 Jan 2010 09:13:26 +0100
> Tell me again in what way the script is of any importance or of any help
> in the permanent SCM changelog after commit.
I find them extremely useful when going through old as well
as new changes.
> I agree with you that it is very good to have the script in the patch
> posting, but I disagree about the changelog.
Thanks for making me go comb the mailing list archives instead of
having the information readily available when looking at a change in
the git history.
I frankly think all of your objections to this kind of thing as well
as putting patchwork URLs into the MAINTAINERS file quite rediculious.
Putting something 100 times (or more) into the kernel tree
documentation or commit messages is a good thing, because people are
going to find the information they need faster.
And likewise here, I want to see what the hell found the bug by
reading the changelog message.
I want more information in changelog messages, not less. A commit
log message is never too long and never gives too much information
about the change, ever...
David Miller wrote:
> And likewise here, I want to see what the hell found the bug by
> reading the changelog message.
Coccinelle found it. In contrast to parsers like sparse, it had to be
programmed to do so. If you want to repeat that process, you have to
get coccinelle and the script. Both can be found quickly without having
them both in the SCM.
> I want more information in changelog messages, not less. A commit
> log message is never too long and never gives too much information
> about the change, ever...
If the changelog carries a lot of unimportant or even unrelated
information, then only a careful structure of the text can prevent that
the important information is buried in noise, and thus lost. (Julia's
logs are structured respectively, so this is OK.)
--
Stefan Richter
-=====-==-=- ---= -====
http://arcgraph.de/sr/
From: Stefan Richter <[email protected]>
Date: Fri, 15 Jan 2010 09:50:51 +0100
> David Miller wrote:
>> And likewise here, I want to see what the hell found the bug by
>> reading the changelog message.
>
> Coccinelle found it. In contrast to parsers like sparse, it had to be
> programmed to do so. If you want to repeat that process, you have to
> get coccinelle and the script. Both can be found quickly without having
> them both in the SCM.
I still don't see how this can argue for not putting the script into
the commit message.
And you overstate how "easily" the script can be found.
What button do I push in gitk when viewing a commit to find the
script?
If I have to go out of the tool, that's the beginning down the long
road of potential distractions.
On 15 Jan, David Miller wrote:
> What button do I push in gitk when viewing a commit to find the
> script?
>
> If I have to go out of the tool, that's the beginning down the long
> road of potential distractions.
[There is a button in gitk which launches coccinelle and feeds the
copy-an-paste buffer into it?]
So somebody scripted a test which finds a certain type of problem, ran
it on the tree, and submitted fixes. Later on, somebody wants to re-run
the test to check whether this problem was reintroduced in new code. So
you are arguing that this somebody should, among else, find one of the
parts required for the test in the kernel source repository's changelog
metadata when he is in the process to add that test to his toolkit.
I on the other hold the opinion that the changelog has one purpose, to
describe the code change.
--
Stefan Richter
-=====-==-=- ---= -====
http://arcgraph.de/sr/
From: Stefan Richter <[email protected]>
Date: Fri, 15 Jan 2010 10:17:08 +0100 (CET)
> I on the other hold the opinion that the changelog has one purpose, to
> describe the code change.
%99 of a bug fix is the process and means by which the bug was found,
therefore the "how was it found" is at least as important as how it is
fixed.
If the script in the commit bothers you, nobody forces you to read it
or use it.
If you want more clear markers around the script so you can skim
past it more efficiently when reading the commit message, then ask
for that.
> If you want more clear markers around the script so you can skim
> past it more efficiently when reading the commit message, then ask
> for that.
If there were markers that would cause tools to hide some information
under a +, that would be great. Sometimes I have simplified a script
beyond what would really be reusable just to not create an over-large
changelog. I hope that the simplified version is at least understandable,
so that the reader can get an idea of what considerations went into the
change, but I recall in one case having messed up on that as well, and
ended up with something that really gave no information whatsoever.
julia
Hi Julia,
On Fri, Jan 15, 2010 at 11:43 AM, Julia Lawall <[email protected]> wrote:
>> If you want more clear markers around the script so you can skim
>> past it more efficiently when reading the commit message, then ask
>> for that.
>
> If there were markers that would cause tools to hide some information
> under a +, that would be great. ?Sometimes I have simplified a script
> beyond what would really be reusable just to not create an over-large
> changelog. ?I hope that the simplified version is at least understandable,
> so that the reader can get an idea of what considerations went into the
> change, but I recall in one case having messed up on that as well, and
> ended up with something that really gave no information whatsoever.
It seems to me that the scripts are kernel specific so why don't we
put those useful scripts _within_ the kernel source tree and introduce
a "make coccinelle-check" target?
Pekka
On Fri, 15 Jan 2010, Pekka Enberg wrote:
> Hi Julia,
>
> On Fri, Jan 15, 2010 at 11:43 AM, Julia Lawall <[email protected]> wrote:
> >> If you want more clear markers around the script so you can skim
> >> past it more efficiently when reading the commit message, then ask
> >> for that.
> >
> > If there were markers that would cause tools to hide some information
> > under a +, that would be great. ?Sometimes I have simplified a script
> > beyond what would really be reusable just to not create an over-large
> > changelog. ?I hope that the simplified version is at least understandable,
> > so that the reader can get an idea of what considerations went into the
> > change, but I recall in one case having messed up on that as well, and
> > ended up with something that really gave no information whatsoever.
>
> It seems to me that the scripts are kernel specific so why don't we
> put those useful scripts _within_ the kernel source tree and introduce
> a "make coccinelle-check" target?
Indeed, perhaps a solution that would satisfy everyone would be to make a
place for scripts, perhaps with subdirectories for various tools, and then
when one submits a patch for tool X, one could then submit the script at
the same time (if it wasn't there already) and just refer to the script
that was used. That way, if someone wants to know more about how the
change was made, they could look up the information, and if one does not,
then one would not be bother by having to scroll down to see the actual
patch.
julia
On Fri, Jan 15, 2010 at 11:05:21AM +0100, Julia Lawall wrote:
> On Fri, 15 Jan 2010, Pekka Enberg wrote:
> > It seems to me that the scripts are kernel specific so why don't we
> > put those useful scripts _within_ the kernel source tree and introduce
> > a "make coccinelle-check" target?
> Indeed, perhaps a solution that would satisfy everyone would be to make a
> place for scripts, perhaps with subdirectories for various tools, and then
> when one submits a patch for tool X, one could then submit the script at
> the same time (if it wasn't there already) and just refer to the script
> that was used. That way, if someone wants to know more about how the
> change was made, they could look up the information, and if one does not,
> then one would not be bother by having to scroll down to see the actual
> patch.
This would also mean that other people could run and re-run the scripts
during development much more easily which would help improve the
coverage of new code.
On Fri, 15 Jan 2010, Mark Brown wrote:
> On Fri, Jan 15, 2010 at 11:05:21AM +0100, Julia Lawall wrote:
> > On Fri, 15 Jan 2010, Pekka Enberg wrote:
>
> > > It seems to me that the scripts are kernel specific so why don't we
> > > put those useful scripts _within_ the kernel source tree and introduce
> > > a "make coccinelle-check" target?
>
> > Indeed, perhaps a solution that would satisfy everyone would be to make a
> > place for scripts, perhaps with subdirectories for various tools, and then
> > when one submits a patch for tool X, one could then submit the script at
> > the same time (if it wasn't there already) and just refer to the script
> > that was used. That way, if someone wants to know more about how the
> > change was made, they could look up the information, and if one does not,
> > then one would not be bother by having to scroll down to see the actual
> > patch.
>
> This would also mean that other people could run and re-run the scripts
> during development much more easily which would help improve the
> coverage of new code.
On the other hand, one has to take into account the fact that at least in
my case, the patches that are submitted are the ones that I have carefully
checked for correctness. Having a make target in the kernel might give
some suggestion of quality that is perhaps not appropriate?
julia
Hi Julia,
On Fri, Jan 15, 2010 at 2:06 PM, Julia Lawall <[email protected]> wrote:
> On the other hand, one has to take into account the fact that at least in
> my case, the patches that are submitted are the ones that I have carefully
> checked for correctness. ?Having a make target in the kernel might give
> some suggestion of quality that is perhaps not appropriate?
I don't see the problem. No tool is perfect so the only thing that
matters is whether or not the benefits outweigh the costs. The problem
I see with Coccinelle is that the scripts seem to be lost in a black
hole (even if they are part of the changelog) which also means that
your efforts don't scale.
There seems to be a universal law regarding kernel development: if
something is out-of-tree, it simply doesn't matter from "people
scalability" point of view. It doesn't really matter if you like it or
not but that's the way things are right now. I, for one, would love to
run Coccinelle on the tree I maintain but unfortunately the tool fails
my "can I set it up in two minutes" rule so I haven't gotten around to
give it a try. And I suspect I am not the only one.
So why not try it out? If the signal to noise ratio is too low and we
can't fix that, we can always just remove the damn thing from the
tree. Speculating whether or not something will work seems pretty
pointless to me when you don't have any hard data.
Pekka
Julia Lawall wrote:
> On Fri, 15 Jan 2010, Mark Brown wrote:
>> On Fri, Jan 15, 2010 at 11:05:21AM +0100, Julia Lawall wrote:
>>> On Fri, 15 Jan 2010, Pekka Enberg wrote:
>>>> It seems to me that the scripts are kernel specific so why don't we
>>>> put those useful scripts _within_ the kernel source tree and introduce
>>>> a "make coccinelle-check" target?
I guess many of these scripts are also applicable to other C programs.
But having such tests bundled with the kernel source and ready to be run
from make sounds good to me too.
>>> Indeed, perhaps a solution that would satisfy everyone would be to make a
>>> place for scripts, perhaps with subdirectories for various tools, and then
>>> when one submits a patch for tool X, one could then submit the script at
>>> the same time (if it wasn't there already) and just refer to the script
>>> that was used. That way, if someone wants to know more about how the
>>> change was made, they could look up the information, and if one does not,
>>> then one would not be bother by having to scroll down to see the actual
>>> patch.
>> This would also mean that other people could run and re-run the scripts
>> during development much more easily which would help improve the
>> coverage of new code.
>
> On the other hand, one has to take into account the fact that at least in
> my case, the patches that are submitted are the ones that I have carefully
> checked for correctness. Having a make target in the kernel might give
> some suggestion of quality that is perhaps not appropriate?
>
> julia
Surely some tests are more suitable for automation than others. Those
who use these tests are probably aware though that the resulting reports
or patches require careful review. This is similar as with reports from
checkpatch, sparse, lockdep etc., except that coccinelle already creates
a fix patch rather than just an error log.
--
Stefan Richter
-=====-==-=- ---= -====
http://arcgraph.de/sr/
Hi Stefan,
On Fri, Jan 15, 2010 at 2:45 PM, Stefan Richter
<[email protected]> wrote:
> I guess many of these scripts are also applicable to other C programs.
Well, most of the ones I've seen are related to kernel APIs or idioms
which are not meaningful to other projects so I don't know if that's
true or not. I don't have a full solution here but I'm just trying to
find a middle ground because I tend to agree with both of you. Yes,
it's damn ugly to have the scripts embedded in the changelog but on
the other hand I _really_ want to see "what the hell found the bug"
too.
Pekka
On Fri, 15 Jan 2010, Pekka Enberg wrote:
> Hi Julia,
>
> On Fri, Jan 15, 2010 at 2:06 PM, Julia Lawall <[email protected]> wrote:
> > On the other hand, one has to take into account the fact that at least in
> > my case, the patches that are submitted are the ones that I have carefully
> > checked for correctness. ?Having a make target in the kernel might give
> > some suggestion of quality that is perhaps not appropriate?
>
> I don't see the problem. No tool is perfect so the only thing that
> matters is whether or not the benefits outweigh the costs. The problem
> I see with Coccinelle is that the scripts seem to be lost in a black
> hole (even if they are part of the changelog) which also means that
> your efforts don't scale.
>
> There seems to be a universal law regarding kernel development: if
> something is out-of-tree, it simply doesn't matter from "people
> scalability" point of view. It doesn't really matter if you like it or
> not but that's the way things are right now. I, for one, would love to
> run Coccinelle on the tree I maintain but unfortunately the tool fails
> my "can I set it up in two minutes" rule so I haven't gotten around to
> give it a try. And I suspect I am not the only one.
>
> So why not try it out? If the signal to noise ratio is too low and we
> can't fix that, we can always just remove the damn thing from the
> tree. Speculating whether or not something will work seems pretty
> pointless to me when you don't have any hard data.
We would be happy to go ahead with it, but would like to hear from others
to be sure to be setting things up in a good way.
For other software there is a tool on our web page called coccicheck that
runs a collection of scripts on a given directory. But we have only put
scripts of general interest in there.
julia
David Miller wrote:
> From: Stefan Richter <[email protected]>
> Date: Fri, 15 Jan 2010 10:17:08 +0100 (CET)
>
>> I on the other hold the opinion that the changelog has one purpose, to
>> describe the code change.
>
> %99 of a bug fix is the process and means by which the bug was found,
> therefore the "how was it found" is at least as important as how it is
> fixed.
Well, granted. The most important part of the changelog is perhaps the
"why", i.e. what problem is being fixed. How this problem was found is
often worth to describe as well. It may e.g. help to avoid future
occurrences or hint at further as yet unsolved issues. But how detailed
to document it is of course subjective; we could argue all day what is
signal and what is noise. (Which reminds me to stop being noisy and get
back to signal generation.)
> If the script in the commit bothers you, nobody forces you to read it
> or use it.
>
> If you want more clear markers around the script so you can skim
> past it more efficiently when reading the commit message, then ask
> for that.
I won't ask for that. The changelog format should not evolve into a
markup language.
--
Stefan Richter
-=====-==-=- ---= -====
http://arcgraph.de/sr/
On Fri, Jan 15, 2010 at 01:06:04PM +0100, Julia Lawall wrote:
> On Fri, 15 Jan 2010, Mark Brown wrote:
> > This would also mean that other people could run and re-run the scripts
> > during development much more easily which would help improve the
> > coverage of new code.
> On the other hand, one has to take into account the fact that at least in
> my case, the patches that are submitted are the ones that I have carefully
> checked for correctness. Having a make target in the kernel might give
> some suggestion of quality that is perhaps not appropriate?
This is a general problem with any tool that generates warnings - we
already have to educate people about how to use the GCC and sparse
warnings appropriately, I can't see that these warnings would be any
different.
Hi Marek,
there was a discussion about patches which are generated using the
tool called spatch. In the changelog the SmPL script was usually
included, see http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Ftorvalds%2Flinux-2.6.git&a=search&h=HEAD&st=commit&s=%3Csmpl%3E .
It is useful to store the SmPL scripts because they may find
problems in the newcoming code also.
In order to run a "check" the spatch tool and the SmPL is also necessary.
There was an idea to place the used SmPL scripts under the Linux
kernel source tree so it can move from the changelog but still remain
for later use. The "check" could be run similar to the tools checkpatch,
sparse or lockdep.
What do you think where the SmPL scripts can be placed?
What do you think the best way would be to introduce some check like this
in the build environment?
Regards,
Márton Németh