From: "Giampaolo Tomassoni" <g.tomassoni@libero.it>
To: "Francois Romieu" <romieu@fr.zoreil.com>
Cc: <linux-kernel@vger.kernel.org>, <linux-atm-general@lists.sourceforge.net>
Subject: R: R: [Linux-ATM-General] [ATMSAR] Request for review - update #1
Date: Sun, 4 Sep 2005 20:44:01 +0200
Message-ID: <NBBBIHMOBLOHKCGIMJMDIEIIEKAA.g.tomassoni@libero.it>
MIME-Version: 1.0
Content-Type: text/plain;
	charset="us-ascii"
Content-Transfer-Encoding: 8BIT
In-Reply-To: <20050904153241.GA7779@electric-eye.fr.zoreil.com>
Importance: Normal
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4673
Lines: 169

> -----Messaggio originale-----
> Da: Francois Romieu [mailto:romieu@fr.zoreil.com]
> Inviato: domenica 4 settembre 2005 17.33
> A: Giampaolo Tomassoni
> Cc: linux-kernel@vger.kernel.org;
> linux-atm-general@lists.sourceforge.net
> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/