From: Julia Lawall <[email protected]>
If the NULL test is necessary, then the dereference should be moved below
the NULL test.
The semantic patch that makes this change is as follows:
(http://www.emn.fr/x-info/coccinelle/)
// <smpl>
@@
type T;
expression E;
identifier i,fld;
statement S;
@@
- T i = E->fld;
+ T i;
... when != E
when != i
if (E == NULL) S
+ i = E->fld;
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
drivers/staging/otus/wwrap.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff -u -p a/drivers/staging/otus/wwrap.c b/drivers/staging/otus/wwrap.c
--- a/drivers/staging/otus/wwrap.c
+++ b/drivers/staging/otus/wwrap.c
@@ -1016,12 +1016,13 @@ void kevent(struct work_struct *work)
{
struct usbdrv_private *macp =
container_of(work, struct usbdrv_private, kevent);
- zdev_t *dev = macp->device;
+ zdev_t *dev;
if (macp == NULL)
{
return;
}
+ dev = macp->device;
if (test_and_set_bit(0, (void *)&smp_kevent_Lock))
{
On 07/11/2009 09:51 AM, Julia Lawall wrote:
> diff -u -p a/drivers/staging/otus/wwrap.c b/drivers/staging/otus/wwrap.c
> --- a/drivers/staging/otus/wwrap.c
> +++ b/drivers/staging/otus/wwrap.c
> @@ -1016,12 +1016,13 @@ void kevent(struct work_struct *work)
> {
> struct usbdrv_private *macp =
> container_of(work, struct usbdrv_private, kevent);
> - zdev_t *dev = macp->device;
> + zdev_t *dev;
>
> if (macp == NULL)
> {
> return;
> }
The test is rather useless here.
> + dev = macp->device;
On Sat, 11 Jul 2009, Jiri Slaby wrote:
> On 07/11/2009 09:51 AM, Julia Lawall wrote:
> > diff -u -p a/drivers/staging/otus/wwrap.c b/drivers/staging/otus/wwrap.c
> > --- a/drivers/staging/otus/wwrap.c
> > +++ b/drivers/staging/otus/wwrap.c
> > @@ -1016,12 +1016,13 @@ void kevent(struct work_struct *work)
> > {
> > struct usbdrv_private *macp =
> > container_of(work, struct usbdrv_private, kevent);
> > - zdev_t *dev = macp->device;
> > + zdev_t *dev;
> >
> > if (macp == NULL)
> > {
> > return;
> > }
>
> The test is rather useless here.
OK, I will submit a revised patch that just drops it.
thanks,
julia
> > + dev = macp->device;
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Sat, 2009-07-11 at 10:17 +0200, Jiri Slaby wrote:
> On 07/11/2009 09:51 AM, Julia Lawall wrote:
> > diff -u -p a/drivers/staging/otus/wwrap.c b/drivers/staging/otus/wwrap.c
> > --- a/drivers/staging/otus/wwrap.c
> > +++ b/drivers/staging/otus/wwrap.c
> > @@ -1016,12 +1016,13 @@ void kevent(struct work_struct *work)
> > {
> > struct usbdrv_private *macp =
> > container_of(work, struct usbdrv_private, kevent);
> > - zdev_t *dev = macp->device;
> > + zdev_t *dev;
> >
> > if (macp == NULL)
> > {
> > return;
> > }
>
> The test is rather useless here.
>
Why useless, it should be non-null before setting to dev.
But issue is why function name like kevent() is global and who is using
it.
> > + dev = macp->device;
>
>
Thanks,
--
JSR
On Sat, 2009-07-11 at 15:47 +0530, Jaswinder Singh Rajput wrote:
> On Sat, 2009-07-11 at 10:17 +0200, Jiri Slaby wrote:
> > On 07/11/2009 09:51 AM, Julia Lawall wrote:
> > > diff -u -p a/drivers/staging/otus/wwrap.c b/drivers/staging/otus/wwrap.c
> > > --- a/drivers/staging/otus/wwrap.c
> > > +++ b/drivers/staging/otus/wwrap.c
> > > @@ -1016,12 +1016,13 @@ void kevent(struct work_struct *work)
> > > {
> > > struct usbdrv_private *macp =
> > > container_of(work, struct usbdrv_private, kevent);
> > > - zdev_t *dev = macp->device;
> > > + zdev_t *dev;
> > >
> > > if (macp == NULL)
> > > {
> > > return;
> > > }
> >
> > The test is rather useless here.
> >
>
> Why useless, it should be non-null before setting to dev.
>
> But issue is why function name like kevent() is global and who is using
> it.
>
> > > + dev = macp->device;
> >
> >
>
And more funny thing is that all functions are global in this file.
Greg, can you please check the scope of these function, why all are
global.
Thanks,
--
JSR
On 07/11/2009 12:17 PM, Jaswinder Singh Rajput wrote:
> On Sat, 2009-07-11 at 10:17 +0200, Jiri Slaby wrote:
>> On 07/11/2009 09:51 AM, Julia Lawall wrote:
>>> --- a/drivers/staging/otus/wwrap.c
>>> +++ b/drivers/staging/otus/wwrap.c
>>> @@ -1016,12 +1016,13 @@ void kevent(struct work_struct *work)
>>> {
>>> struct usbdrv_private *macp =
>>> container_of(work, struct usbdrv_private, kevent);
>>> - zdev_t *dev = macp->device;
>>> + zdev_t *dev;
>>>
>>> if (macp == NULL)
>>> {
>>> return;
>>> }
>>
>> The test is rather useless here.
>>
>
> Why useless, it should be non-null before setting to dev.
Even if work was NULL, macp won't be NULL.
> But issue is why function name like kevent() is global and who is using
> it.
Since it's not declared, I guess nobody but keventd. It should be static.
On 07/11/2009 12:32 PM, Jaswinder Singh Rajput wrote:
> And more funny thing is that all functions are global in this file.
>
> Greg, can you please check the scope of these function, why all are
> global.
Why don't you do that? I think patches are welcome :).
On Sat, 11 Jul 2009, Jiri Slaby wrote:
> On 07/11/2009 12:17 PM, Jaswinder Singh Rajput wrote:
> > On Sat, 2009-07-11 at 10:17 +0200, Jiri Slaby wrote:
> >> On 07/11/2009 09:51 AM, Julia Lawall wrote:
> >>> --- a/drivers/staging/otus/wwrap.c
> >>> +++ b/drivers/staging/otus/wwrap.c
> >>> @@ -1016,12 +1016,13 @@ void kevent(struct work_struct *work)
> >>> {
> >>> struct usbdrv_private *macp =
> >>> container_of(work, struct usbdrv_private, kevent);
> >>> - zdev_t *dev = macp->device;
> >>> + zdev_t *dev;
> >>>
> >>> if (macp == NULL)
> >>> {
> >>> return;
> >>> }
> >>
> >> The test is rather useless here.
> >>
> >
> > Why useless, it should be non-null before setting to dev.
>
> Even if work was NULL, macp won't be NULL.
work could be a small negative number such that calling container_of would
return 0.
julia
On Sat, 11 Jul 2009, Julia Lawall wrote:
> On Sat, 11 Jul 2009, Jiri Slaby wrote:
>
> > On 07/11/2009 12:17 PM, Jaswinder Singh Rajput wrote:
> > > On Sat, 2009-07-11 at 10:17 +0200, Jiri Slaby wrote:
> > >> On 07/11/2009 09:51 AM, Julia Lawall wrote:
> > >>> --- a/drivers/staging/otus/wwrap.c
> > >>> +++ b/drivers/staging/otus/wwrap.c
> > >>> @@ -1016,12 +1016,13 @@ void kevent(struct work_struct *work)
> > >>> {
> > >>> struct usbdrv_private *macp =
> > >>> container_of(work, struct usbdrv_private, kevent);
> > >>> - zdev_t *dev = macp->device;
> > >>> + zdev_t *dev;
> > >>>
> > >>> if (macp == NULL)
> > >>> {
> > >>> return;
> > >>> }
> > >>
> > >> The test is rather useless here.
> > >>
> > >
> > > Why useless, it should be non-null before setting to dev.
> >
> > Even if work was NULL, macp won't be NULL.
>
> work could be a small negative number such that calling container_of would
> return 0.
On the other hand, that does not seem to be possible in this case, since
work is passed through INIT_WORK, which dereferences the work value.
julia
On Sat, Jul 11, 2009 at 10:17:24AM +0200, Jiri Slaby wrote:
> On 07/11/2009 09:51 AM, Julia Lawall wrote:
> > diff -u -p a/drivers/staging/otus/wwrap.c b/drivers/staging/otus/wwrap.c
> > --- a/drivers/staging/otus/wwrap.c
> > +++ b/drivers/staging/otus/wwrap.c
> > @@ -1016,12 +1016,13 @@ void kevent(struct work_struct *work)
> > {
> > struct usbdrv_private *macp =
> > container_of(work, struct usbdrv_private, kevent);
> > - zdev_t *dev = macp->device;
> > + zdev_t *dev;
> >
> > if (macp == NULL)
> > {
> > return;
> > }
>
> The test is rather useless here.
Yes it is.
Julia, if you just remove the NULL check, will that keep your scripts
happy from noticing such a "problem"?
thanks,
greg k-h
On Sat, 11 Jul 2009, Greg KH wrote:
> On Sat, Jul 11, 2009 at 10:17:24AM +0200, Jiri Slaby wrote:
> > On 07/11/2009 09:51 AM, Julia Lawall wrote:
> > > diff -u -p a/drivers/staging/otus/wwrap.c b/drivers/staging/otus/wwrap.c
> > > --- a/drivers/staging/otus/wwrap.c
> > > +++ b/drivers/staging/otus/wwrap.c
> > > @@ -1016,12 +1016,13 @@ void kevent(struct work_struct *work)
> > > {
> > > struct usbdrv_private *macp =
> > > container_of(work, struct usbdrv_private, kevent);
> > > - zdev_t *dev = macp->device;
> > > + zdev_t *dev;
> > >
> > > if (macp == NULL)
> > > {
> > > return;
> > > }
> >
> > The test is rather useless here.
>
> Yes it is.
>
> Julia, if you just remove the NULL check, will that keep your scripts
> happy from noticing such a "problem"?
Yes. The trigger is the NULL test after the dereference.
julia
On Sat, Jul 11, 2009 at 04:09:49AM -0700, Jiri Slaby wrote:
> On 07/11/2009 12:32 PM, Jaswinder Singh Rajput wrote:
> > And more funny thing is that all functions are global in this file.
> >
> > Greg, can you please check the scope of these function, why all are
> > global.
>
> Why don't you do that? I think patches are welcome :).
I will note Otus has a replacement ar9170 driver already upstream,
as soon as it gets aggregation support otus will be dropped. Better
focus on ar9170 instead if you are looking to spend some cycles on
driver help.
Luis