Newbie idiotic questions.
I have been looking at the emu10k1 driver and I had a few questions about general idioms used there.
In a line like this,
[main.c, line 175]
for (count = 0; count < sizeof(card->digmix) / sizeof(card->digmix[0]); count++) {Isn't there some sort of `ALEN' macro available, or is this considered to muddy things by using a macro?
[main.c, line 223]
if ((card->mpuout = kmalloc(sizeof(struct emu10k1_mpuout), GFP_KERNEL))
Why is the struct type referenced for the allocation size? Why not,
if ((card->mpuout = kmalloc(sizeof(card->mpuout), GFP_KERNEL))
This seems to get the size for the actual object being allocated.
[cardmi.c, line 42]
static struct {
int (*Fn) (struct emu10k1_mpuin *, u8);
} midistatefn[] = {...
Why aren't all the gobs of constant data in this driver declared as constant? Do it give a performance advantage by having the data in a different MMU section and better cache effects or something?
Thanks for any helpful pointers. I did read the FAQ. I am just wonder if I would get screamed at for changing things like this and why... so I will probably get yelled at for suggesting them anyway, but at least I won't have went through the effort. Now that I have pointed that out, the will probably irk people even more...
regards,
Bill Pringlemeir.
Quoted 6 lines · Bill Pringlemeir
[
main.c, line 175]for (count = 0; count < sizeof(card->digmix) / sizeof(card->digmix[0]); count++) {Isn't there some sort of `ALEN' macro available, or is this considered to muddy things by using a macro?
Yes, we have array size
Quoted 6 lines
[
main.c, line 223]if ((card->mpuout = kmalloc(sizeof(struct emu10k1_mpuout), GFP_KERNEL))Why is the struct type referenced for the allocation size? Why not,
if ((card->mpuout = kmalloc(sizeof(card->mpuout), GFP_KERNEL))
because then you would be allocating the size of a pointer, not the size of a structure
Quoted 3 lines
Why aren't all the gobs of constant data in this driver declared as constant? Do it give a performance advantage by having the data in a different MMU section and better cache effects or something?
Marking data const is usually a good idea.
Jeff
Signature · 3 lines
Jeff Garzik | Andre the Giant has a posse. Building 1024 | MandrakeSoft |
Quoted 4 lines · Bill Pringlemeir
I have been looking at the emu10k1 driver and I had a few questions about general idioms used there.
Quoted 8 lines
In a line like this,
[
main.c, line 175]for (count = 0; count < sizeof(card->digmix) / sizeof(card->digmix[0]); count++) {Isn't there some sort of `ALEN' macro available, or is this considered to muddy things by using a macro?
Note that code in drivers range from very nice to awful, and every level in between.
I generally use a macro called ArraySize(), defined up-front.
Quoted 10 lines
[
main.c, line 223]if ((card->mpuout = kmalloc(sizeof(struct emu10k1_mpuout), GFP_KERNEL))Why is the struct type referenced for the allocation size? Why not,
if ((card->mpuout = kmalloc(sizeof(card->mpuout), GFP_KERNEL))This seems to get the size for the actual object being allocated.
Again, you are correct. However, you may not know the history of the driver. Perhaps at one time the above statement was correct.
Quoted 7 lines
[
cardmi.c, line 42]static struct { int (*Fn) (struct emu10k1_mpuin *, u8); } midistatefn[] = {...
[...snipped....]
Quoted 3 lines
regards,
Bill Pringlemeir.
What you should do is supply a patch. Call it a "general cleanup". Send it first to the maintainer of the driver. If this doesn't work, send it to linux-kernel. Test your patch well because you may fix something that breaks something else.
Cheers,
Dick Johnson
Penguin : Linux version 2.4.1 on an i686 machine (799.53 BogoMips).
"Memory is like gasoline. You use it up when you are running. Of course you get it all back when you reboot..."; Actual explanation obtained from the Micro$oft help desk.
Em Sat, Jun 16, 2001 at 09:35:34PM -0400, Richard B. Johnson escreveu:
Quoted 5 lines
Quoted 5 lines
[
main.c, line 223]if ((card->mpuout = kmalloc(sizeof(struct emu10k1_mpuout), GFP_KERNEL))Why is the struct type referenced for the allocation size? Why not,
Quoted 7 lines
Quoted 4 lines
if ((card->mpuout = kmalloc(sizeof(card->mpuout), GFP_KERNEL))This seems to get the size for the actual object being allocated.
Again, you are correct. However, you may not know the history of the driver. Perhaps at one time the above statement was correct.
yes, and in fact this should be one of the entries in the kernel Janitor's TODO list, please take a look at http://bazar.conectiva.com.br/~acme/TODO
and consider submitting some more things to cleanup so that people wanting to start hacking the kernel can have some easy starting points. Also please consider reading http://kernel-janitor.sourceforge.net
- Arnaldo
Hi,
Quoted 3 lines · Bill Pringlemeir
I have been looking at the emu10k1 driver and I had a few questions about general idioms used there.
Warning I've not looked at that particular driver and would concider myself a Linux kernel newbie. 20 years kernel hacking but only 9 months Linux with two drivers under my belt. One of which I'm now trying to get into the standard tree.
Quoted 6 lines
[
main.c, line 175]for (count = 0; count < sizeof(card->digmix) / sizeof(card->digmix[0]); count++) {Isn't there some sort of `ALEN' macro available, or is this considered to muddy things by using a macro?
Some writers are good and some not so good. Personally I'd prefer an ArrayLen style macro but some prefer to see it long hand. My usual style would be to use the array size #def used to define the array. I'd shoot whoever wrote digmix[9 * 6 * 2]; OK so I just peeked at the source :-)
I might be tempted to tidy this up whilst I was adding something significant to the driver but not for the sake of it. If I tidied that one up I'd have to change all the other occurances in a consistent manner, seem like a lot of work for little or no gain.
Quoted 8 lines
[
main.c, line 223]if ((card->mpuout = kmalloc(sizeof(struct emu10k1_mpuout), GFP_KERNEL))Why is the struct type referenced for the allocation size? Why not,
if ((card->mpuout = kmalloc(sizeof(card->mpuout), GFP_KERNEL))This seems to get the size for the actual object being allocated.
I prefer the struct construct and this also demonstrates why it's best not to fiddle just for the sake of it. You've changed the allocation from the size of the struct into the size of a pointer.
Quoted 10 lines
[
cardmi.c, line 42]static struct { int (*Fn) (struct emu10k1_mpuin *, u8); } midistatefn[] = {...
Why aren't all the gobs of constant data in this driver declared as constant? Do it give a performance advantage by having the data in a different MMU section and better cache effects or something?
const is better but frequently forgotten :-(
Quoted 5 lines
Thanks for any helpful pointers. I did read the FAQ. I am just wonder if I would get screamed at for changing things like this and why... so I will probably get yelled at for suggesting them anyway, but at least I won't have went through the effort. Now that I have pointed that out, the will probably irk people even more...
Perhaps the biggest one is when making changes follow the style of the existing file. Style changes in midstream only add to the confusion. Don't change layout rules or methods of doing things unless you are prepared to update the entire module and justify the changes to the original module author who has every right to be highly critical.
Signature · 3 lines
Bob Dunlop FarSite Communications
[email protected] [email protected]
www.xyzzy.clara.co.uk www.farsite.co.ukQuoted 7 lines · Jeff Garzik
Quoted 3 lines · Bill Pringlemeir
Why is the struct type referenced for the allocation size? Why not,
if ((card->mpuout = kmalloc(sizeof(card->mpuout), GFP_KERNEL))because then you would be allocating the size of a pointer, not the size of a structure
Whoops Jeff, you didn't have your coffee yet:
struct foo { int a; int b; };
struct { struct foo foo; } *foobar;
int main (void)
{
printf("%i\n", sizeof(struct foo));
printf("%i\n", sizeof(foobar->foo));
printf("%i\n", sizeof(&foobar->foo));
}Prints:
8 8 4
--
Daniel
Quoted 5 lines · Daniel Phillips
Quoted 3 lines
because then you would be allocating the size of a pointer, not the size of a structure
Whoops Jeff, you didn't have your coffee yet:
Whoops yourself. The following patch brings your example into line with the driver code. mpuout is a pointer to a structure not the structure itself as the malloc assignment clearly indicates.
*** x.c-orig Sun Jun 17 13:19:33 2001
--- x.c Sun Jun 17 13:19:42 2001
***************
*** 2,8 ****
struct foo { int a; int b; };
! struct { struct foo foo; } *foobar;
int main (void)
{
--- 2,8 ----
struct foo { int a; int b; };
! struct { struct foo *foo; } *foobar;
int main (void)
{Now Prints:
8 4 4
Signature · 3 lines
Bob Dunlop FarSite Communications
[email protected] [email protected]
www.xyzzy.clara.co.uk www.farsite.co.ukQuoted 9 lines · On Sunday 17 June 2001 14:27, [email protected] wrote:
Quoted 4 lines · Daniel Phillips
Quoted 2 lines
because then you would be allocating the size of a pointer, not the size of a structure
Whoops Jeff, you didn't have your coffee yet:
Whoops yourself. The following patch brings your example into line with the driver code. mpuout is a pointer to a structure not the structure itself as the malloc assignment clearly indicates.
Yep, the only thing left to resolve is whether Jeff had coffee or not. ;-)
- if ((card->mpuout = kmalloc(sizeof(struct emu10k1_mpuout), GFP_KERNEL))
+ if ((card->mpuout = kmalloc(sizeof(*card->mpuout), GFP_KERNEL))
--
Daniel
Review
Quoted 4 lines · Daniel Phillips
Yep, the only thing left to resolve is whether Jeff had coffee or not. ;-)
(quoted diff) +1
-1- if ((card->mpuout = kmalloc(sizeof(struct emu10k1_mpuout), GFP_KERNEL)) + if ((card->mpuout = kmalloc(sizeof(*card->mpuout), GFP_KERNEL))
Yeah, this is fine. The original posted omitted the '*' which was not fine :)
Signature · 3 lines
Jeff Garzik | Andre the Giant has a posse. Building 1024 | MandrakeSoft |
Quoted 4 lines
Quoted 1 line · Daniel Phillips
Yep, the only thing left to resolve is whether Jeff had coffee or not.
;-)
Quoted 2 lines
Quoted 2 lines
- if ((
card->mpuout= kmalloc(sizeof(structemu10k1_mpuout),
GFP_KERNEL))
Quoted 4 lines
Quoted 1 line
+ if ((
card->mpuout=kmalloc(sizeof(*card->mpuout), GFP_KERNEL))Yeah, this is fine. The original posted omitted the '*' which was not fine :)
The only other thing left to ask, is which is easier to read when glancing through the code, and which is easier to read when maintaining the code. imho, ist the former for reading the code, i dont know about maintaing the code since i dont do that, however in my own projects i prefere the former when maintaing the code.
What are others oppinions on this ?
Thanks,
Dave Flynn
Quoted 6 lines
--
Jeff Garzik | Andre the Giant has a posse. Building 1024 | MandrakeSoft |
Quoted 9 lines · David Flynn
Quoted 3 lines
Quoted 1 line · Daniel Phillips
Yep, the only thing left to resolve is whether Jeff had coffee or not.
;-)
Quoted 2 lines
Quoted 2 lines
- if ((
card->mpuout= kmalloc(sizeof(structemu10k1_mpuout),
GFP_KERNEL))Quoted 2 lines
Quoted 1 line
+ if ((
card->mpuout=kmalloc(sizeof(*card->mpuout), GFP_KERNEL))
Quoted 8 lines
Quoted 2 lines
Yeah, this is fine. The original posted omitted the '*' which was not fine :)
The only other thing left to ask, is which is easier to read when glancing through the code, and which is easier to read when maintaining the code. imho, ist the former for reading the code, i dont know about maintaing the code since i dont do that, however in my own projects i prefere the former when maintaing the code.
It's the preference of the maintainer. It's a tossup: using the type in the kmalloc makes the type being allocated obvious. But using sizeof(*var) is a tiny bit more resistant to change.
Neither one sufficiently affects long term maintenance AFAICS, so it's personal preference, not any sort of kernel standard one way or the other...
Jeff
Signature · 3 lines
Jeff Garzik | Andre the Giant has a posse. Building 1024 | MandrakeSoft |
Quoted 18 lines · Jeff Garzik
Quoted 8 lines · David Flynn
Quoted 2 lines
Quoted 1 line · Daniel Phillips
Yep, the only thing left to resolve is whether Jeff had coffee or not.
;-)
Quoted 2 lines
Quoted 2 lines
- if ((
card->mpuout= kmalloc(sizeof(structemu10k1_mpuout),
GFP_KERNEL))Quoted 2 lines
Quoted 1 line
+ if ((
card->mpuout=kmalloc(sizeof(*card->mpuout), GFP_KERNEL))Quoted 8 lines
Quoted 2 lines
Yeah, this is fine. The original posted omitted the '*' which was not fine :)
The only other thing left to ask, is which is easier to read when glancing through the code, and which is easier to read when maintaining the code. imho, ist the former for reading the code, i dont know about maintaing the code since i dont do that, however in my own projects i prefere the former when maintaing the code.
Quoted 7 lines
It's the preference of the maintainer. It's a tossup: using the type in the kmalloc makes the type being allocated obvious. But using
sizeof(*var)is a tiny bit more resistant to change.Neither one sufficiently affects long term maintenance AFAICS, so it's personal preference, not any sort of kernel standard one way or the other...
The first one can be made a bit safer against changes by creating a knew' macro that behaves like new' in C++:
| #define knew(type, flags) (type *)kmalloc(sizeof(type), (flags))
If the types in the assignment don't match, gcc will tell you.
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
Quoted 4 lines · Geert Uytterhoeven
Quoted 3 lines · Jeff Garzik
Quoted 2 lines · David Flynn
Quoted 2 lines
Quoted 1 line · Daniel Phillips
- if ((
card->mpuout= kmalloc(sizeof(structemu10k1_mpuout),
GFP_KERNEL))
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
+ if ((
card->mpuout= kmalloc(sizeof(*card->mpuout),
GFP_KERNEL))
Quoted 18 lines
Quoted 10 lines
Quoted 9 lines
Quoted 3 lines
Yeah, this is fine. The original posted omitted the '*' which was not fine :)
The only other thing left to ask, is which is easier to read when glancing through the code, and which is easier to read when maintaining the code. imho, ist the former for reading the code, i dont know about maintaing the code since i dont do that, however in my own projects i prefere the former when maintaing the code.
Quoted 7 lines
It's the preference of the maintainer. It's a tossup: using the type in the kmalloc makes the type being allocated obvious. But using
sizeof(*var)is a tiny bit more resistant to change.Neither one sufficiently affects long term maintenance AFAICS, so it's personal preference, not any sort of kernel standard one way or the other...
Quoted 6 lines
The first one can be made a bit safer against changes by creating a `knew'
macro that behaves like `new' in C++:
| #defineknew(type, flags)(type *)kmalloc(sizeof(type), (flags))If the types in the assignment don't match, gcc will tell you.
Well, since we are still beating this one to death, I'd written a "knew" macro as well, and put it aside. It does the assignment for you too:
#define knew(p) ((p) = (typeof(p)) kmalloc(sizeof(*(p)), GFP_KERNEL))
Example:
struct foo { int a; int b; };
struct { struct foo *foo; } *foobar;
if (knew(foobar))
if (knew(foobar->foo))
printk("foo: %p\n", foobar->foo);Terse and clear at the same time, and type safe. I still don't like it much.
--
Daniel
Quoted 11 lines · Daniel Phillips
Quoted 4 lines
macro that behaves like `new' in C++:
| #defineknew(type, flags)(type *)kmalloc(sizeof(type), (flags))If the types in the assignment don't match, gcc will tell you.
Well, since we are still beating this one to death, I'd written a "knew" macro as well, and put it aside. It does the assignment for you too:
#define knew(p) ((p) = (typeof(p)) kmalloc(sizeof(*(p)), GFP_KERNEL))Terse and clear at the same time, and type safe. I still don't like it much.
And ungreppable, not to mention gratitious use of GNU extension.
Quoted 10 lines · Alexander Viro
Quoted 7 lines · Daniel Phillips
Well, since we are still beating this one to death, I'd written a "knew" macro as well, and put it aside. It does the assignment for you too:
#define knew(p) ((p) = (typeof(p)) kmalloc(sizeof(*(p)), GFP_KERNEL))Terse and clear at the same time, and type safe. I still don't like it much.
And ungreppable, not to mention gratitious use of GNU extension.
typeof? It's rather popular in the kernel already. Besides, who is going to compile this with anything other than gcc?
I don't see your point about greppability.
--
Daniel
Quoted 1 line · Daniel Phillips
typeof? It's rather popular in the kernel already. Besides, who is going to
Really? 5 instances in PPC arch-specific code, 1 (absolutely gratitious) in drivers/mtd, 2 - in m68k (also useless), 4 - in drivers/video, 2 - in AFFS and 1 - in netfilter.
I wouldn't call it "rather popular".
Quoted 4 lines
compile this with anything other than gcc?
I don't see your point about greppability.
You are making the types it is applied to harder to deal with wrt. global search.
But the real issue here is that preprocessor is not a way to get
polymorphism. And that would be the only context where typeof might have any use. Trying to turn C into the things it isn't is always a bad idea - had been proven many times. starting at least with Bourne shel (check the v7 sh source if you don't know what I'm refering to).
Hi,
Quoted 7 lines · Alexander Viro
Quoted 1 line
typeof? It's rather popular in the kernel already. Besides, who is going to
Really? 5 instances in PPC arch-specific code, 1 (absolutely gratitious) in
drivers/mtd, 2 - in m68k (also useless), 4 - indrivers/video, 2 - in AFFS and 1 - in netfilter.I wouldn't call it "rather popular".
You should also grep for '__typeof__'. :-)
bye, Roman
Quoted 3 lines · Roman Zippel
Quoted 1 line
I wouldn't call it "rather popular".
You should also grep for '
__typeof__'. :-)
Yeeeccchhh. OK, there is more of that. However, the main user of that beast is, AFAICS, get_user()/put_user() and their ilk in include/asm-* The rest looks very bogus...
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Jeff" == Jeff Garzik <[email protected]> writes:
[snip]
Jeff> It's the preference of the maintainer. It's a tossup: using Jeff> the type in the kmalloc makes the type being allocated obvious. Jeff> But using sizeof(*var) is a tiny bit more resistant to change.
Ok, thanks. I was looking at fixing an actual bug' in this driver and I was wonder what else I could/should do while there. I didn't necessarily want to change sizeof(struct Type)' to sizeof(*value)'. I considered that change to be somewhat dubious, even though I like sizeof(*value)'. Of course, I unwittingly demonstrated what dangers lie in making cosmetic changes [I was on my way to a party at the time and my girlfriend was calling, excuses, excuses...].
So, It looks like I might fix the actual race condition, post that diff, fix any other small oddities, post that diff. If no one complains, etc I can ask the maintainer. Of course I will test it myself as well.
regards,
Bill Pringlemeir.