question about drivers/mtd/ubi/cdev.c

question about drivers/mtd/ubi/cdev.c

[Julia Lawall]

I was wondering about the following code in the function rename_volumes in 
the file drivers/mtd/ubi/cdev.c:

list_for_each_entry(re, &rename_list, list) {
	...
        if (no_remove_needed)
                continue;
	...
        re = kzalloc(sizeof(struct ubi_rename_entry), GFP_KERNEL);
	if (!re) { ... }
        re->remove = 1;
        re->desc = desc;
        list_add(&re->list, &rename_list);
	...
}

Is there a danger of repeating computation since re is redefined and moved 
back to the beginning of the list?

julia

Re: question about drivers/mtd/ubi/cdev.c

[Artem Bityutskiy]

On Tue, 2010-08-03 at 22:12 +0200, Julia Lawall wrote:
> I was wondering about the following code in the function rename_volumes in 
> the file drivers/mtd/ubi/cdev.c:
> 
> list_for_each_entry(re, &rename_list, list) {
> 	...
>         if (no_remove_needed)
>                 continue;
> 	...
>         re = kzalloc(sizeof(struct ubi_rename_entry), GFP_KERNEL);
> 	if (!re) { ... }
>         re->remove = 1;
>         re->desc = desc;
>         list_add(&re->list, &rename_list);
> 	...
> }
> 
> Is there a danger of repeating computation since re is redefined and moved 
> back to the beginning of the list?

Hmm, the code is bogus, I do not know how it works, but it does, because
I tested it :-)

We should not use 're' as a temporary variable inside the loop. I guess
we can use 're1' instead. I'll change this later, when have time, unless
you submit a fix earlier :-)

Thanks for pointing this!

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: question about drivers/mtd/ubi/cdev.c

[Julia Lawall]

On Wed, 4 Aug 2010, Artem Bityutskiy wrote:

> On Tue, 2010-08-03 at 22:12 +0200, Julia Lawall wrote:
> > I was wondering about the following code in the function rename_volumes in 
> > the file drivers/mtd/ubi/cdev.c:
> > 
> > list_for_each_entry(re, &rename_list, list) {
> > 	...
> >         if (no_remove_needed)
> >                 continue;
> > 	...
> >         re = kzalloc(sizeof(struct ubi_rename_entry), GFP_KERNEL);
> > 	if (!re) { ... }
> >         re->remove = 1;
> >         re->desc = desc;
> >         list_add(&re->list, &rename_list);
> > 	...
> > }
> > 
> > Is there a danger of repeating computation since re is redefined and moved 
> > back to the beginning of the list?
> 
> Hmm, the code is bogus, I do not know how it works, but it does, because
> I tested it :-)
>
> We should not use 're' as a temporary variable inside the loop. I guess
> we can use 're1' instead. I'll change this later, when have time, unless
> you submit a fix earlier :-)

Would the proper approach be to use list_for_each_entry_safe?

julia

Re: question about drivers/mtd/ubi/cdev.c

[Artem Bityutskiy]

On Wed, 2010-08-04 at 07:48 +0200, Julia Lawall wrote:
> > Hmm, the code is bogus, I do not know how it works, but it does, because
> > I tested it :-)
> >
> > We should not use 're' as a temporary variable inside the loop. I guess
> > we can use 're1' instead. I'll change this later, when have time, unless
> > you submit a fix earlier :-)
> 
> Would the proper approach be to use list_for_each_entry_safe?

Yes, plus not using the iterator as a temporary variable is also a good
idea.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)