> -----Messaggio originale-----
> Da: Francois Romieu [mailto:[email protected]]
> Inviato: domenica 4 settembre 2005 17.33
> A: Giampaolo Tomassoni
> Cc: [email protected];
> [email protected]
> Oggetto: Re: R: [Linux-ATM-General] [ATMSAR] Request for review - update #1
>
> ...omissis...
>
> I'd be happily surprized to see more documented ADSL PCI/USB device in the
> near future. :o(
OT Question. What about an open adsl device? The linux community had been bumped out by the adsl market for at least a couple of years, and nobody knows (or tells) why...
That could be a definitive answer. Is there anybody interested in this?
>
> ...omissis...
>
> > Finally, the fact that ATMSAR is device-unspecific makes it easier to
> > maintain, I guess.
>
> Ok. Your suggestion may have more impact if there is a patch to convert
> the sole existing in-kernel driver to use this module.
Mmmh. I can try to do this, but I would prefer to hear Sands about this.
>
> ...omissis
>
> An uniform codingstyle is useful when people need to review code.
> Something is wrong when a reviewer must uncipher a piece of code.
> You will find areas in the kernel whose trends differ but a codingstyle
> from Mars is usually a hint. So it is not _only_ a matter of taste.
Ok, ok. I'll (try to) behave...
>
> ...omissis...
>
> You may have more feedback/review then. I only gave a cursory look at the
> code.
Right, that's what I'm looking for.
>
> ...omissis...
>
> Rather the "typedef struct atmsar_dev atmsar_dev_t;" (yes, I know the "It
> saves typing" argument). Maybe something could be done at the same time
> regarding the need for the forward declarations.
Well, fine. I'll "struct _whatever *". But atmsar_dev_t no, that nonono: it mimics the atm_dev_t typedef... It's all around the idea a developer needs to use atmsar_dev_t instead of atm_dev_t...
>
> ...omissis...
>
> s/what/why/
>
> And no, documenting a call to skb_reserve is silly.
...
>
> ...omissis...
>
> The value returned by sprintf and friends contains the needed offset, i.e.
> buf += sprintf(buf, ...);.
I used an strcpy() to put the constant string in the buffer. However, I'm changing it this way:
if(skip-- == 0) {
count = strlen(strcpy(page, "dnrate:\t"));
if(dev->rx_speed != ATMSAR_SPEED_UNSPEC)
count += sprintf(
&page[count],
"%ld kbps\n",
dev->rx_speed
);
else
count += strlen(strcpy(&page[count], "unknown\n"));
return(count);
}
> [...]
> > > - "return" is not a function.
> >
> > Not even for() or while(). But doesn't they look cute this way?
>
> No.
>
> for (), while (), return rc;
...
> [...]
> > > - consider 'goto' to handle the errors instead of deep nesting
> >
> > I prefer not using goto when not required to. Nesting is far
> more readable
> > to my opinion.
>
> OTOH, it makes ugly code to have it fit in a 80 columns console.
>
> [...]
> > Anyway, which are the functions you are objecting?
>
> atmSend. Probably others.
>
> If you can make the code look like existing in-kernel code (not fs/cifs
> please) say network or ata driver code and you do not need goto, it's fine
> too.
Mmmmh. I'll check it out.
> > > - +const atmsar_aalops_t opsAALR = {
> > > + ATM_AAL0,
> > > + "raw",
> > > -> use .foo = baz instead.
> >
> > atmasr_aalops_t is not an exported structure (you'll find just an opaque
> > definition in include/linux/atmsar.h), so it is not meant to be
> statically
> > declared by device drivers. But I guess that the problem is readability,
> > right?
>
> struct foo zoy {
> .bar = barbar,
> .baz = bazbaz,
> .quuz = ...
> };
...
> [...]
> > May I ask if this is just your own contribution or if you are
> in charge of
> > something in the linux and/or linux-atm projects?
>
> /me scratches head
>
> http://ww.google.com/search?hl=en&q=romieu+linux+cabal
That was to give the right wedge to your hints. If you were just around this list, your hints had a different value than if you were a committer. Am I wrong?
Thanks,
-----------------------------------
Giampaolo Tomassoni - IT Consultant
Piazza VIII Aprile 1948, 4
I-53044 Chiusi (SI) - Italy
Ph: +39-0578-21100
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
| |
 |