Date: Fri, 12 Nov 1999 09:10:02 -0800 (PST) From: Bill Fumerola <billf@chc-chimes.com> To: freebsd-bugs@FreeBSD.org Subject: Re: kern/14843: Added Frame Relay support Message-ID: <199911121710.JAA92308@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/14843; it has been noted by GNATS.
From: Bill Fumerola <billf@chc-chimes.com>
To: vak@cronyx.ru
Cc: FreeBSD-gnats-submit@freebsd.org
Subject: Re: kern/14843: Added Frame Relay support
Date: Fri, 12 Nov 1999 11:02:50 -0500 (EST)
I am not offering a code review below, just a style review. If you
hate people who nitpick your style, you might not want to read on.
However, if you want to contribute code to FreeBSD, we typically follow
KNF as detailed in style(9).
On with the show..
On Fri, 12 Nov 1999 vak@cronyx.ru wrote:
> @@ -663,7 +735,7 @@
> * become invalid. So we
> * - don't let packets with src ip addr 0 thru
> * - we flag TCP packets with src ip 0 as an error
> - */
> + */
>
> if(ip->ip_src.s_addr == INADDR_ANY) /* -hm */
> {
> @@ -674,7 +746,7 @@
> else
> return(0);
> }
> -
> +
Whitespace changes, avoid.
> @@ -1117,7 +1202,7 @@
> #if defined(__FreeBSD__) && __FreeBSD__ >= 3
> getmicrouptime(&tv);
> #endif
> -
> +
> MGETHDR (m, M_DONTWAIT, MT_DATA);
> if (! m)
> return;
Ditto.
> @@ -1738,7 +1823,7 @@
> case STATE_STOPPING:
> sppp_cp_send(sp, cp->proto, TERM_REQ, ++sp->pp_seq,
> 0, 0);
> - TIMEOUT(cp->TO, (void *)sp, sp->lcp.timeout,
> + TIMEOUT(cp->TO, (void *)sp, sp->lcp.timeout,
> sp->ch[cp->protoidx]);
Ditto.
> @@ -1779,7 +1864,7 @@
> case STATE_REQ_SENT:
> case STATE_ACK_RCVD:
> case STATE_ACK_SENT:
> - TIMEOUT(cp->TO, (void *)sp, sp->lcp.timeout,
> + TIMEOUT(cp->TO, (void *)sp, sp->lcp.timeout,
Ditto.
> @@ -2322,7 +2407,7 @@
> /* notify low-level driver of state change */
> if (sp->pp_chg)
> sp->pp_chg(sp, (int)sp->pp_phase);
> -
> +
Ditto. :>
> @@ -2641,7 +2726,7 @@
> (hisaddr == 1 && desiredaddr != 0)) {
> /*
> * Peer's address is same as our value,
> - * or we have set it to 0.0.0.1 to
> + * or we have set it to 0.0.0.1 to
> * indicate that we do not really care,
> * this is agreeable. Gonna conf-ack
> * it.
> @@ -3026,7 +3111,7 @@
> }
> break;
> }
> -
> +
> if (debug) {
> log(LOG_DEBUG,
> SPP_FMT "chap input <%s id=0x%x len=%d name=",
> @@ -3136,7 +3221,7 @@
> sppp_print_string(sp->hisauth.name,
> sppp_strnlen(sp->hisauth.name, AUTHNAMELEN));
> addlog("\n");
> - }
> + }
> if (debug) {
And all of the above.
> @@ -3958,7 +4048,7 @@
> si->sin_addr.s_addr = htonl(src);
>
> /* add new route */
> - error = rtinit(ifa, (int)RTM_ADD, RTF_HOST);
> + error = rtinit(ifa, (int)RTM_ADD, RTF_HOST);
Here too.
> @@ -3966,7 +4056,7 @@
> }
> #endif
> }
> -}
> +}
And here.
>
> static int
> sppp_params(struct sppp *sp, u_long cmd, void *data)
> @@ -4098,7 +4188,7 @@
> /* if no NCP is starting, all this was in vain, close down */
> sppp_lcp_check_and_close(sp);
> }
> -
> +
>
> static const char *
> sppp_cp_type_name(u_char type)
Here too
> @@ -4271,4 +4361,500 @@
> sppp_null(struct sppp *unused)
> {
> /* do just nothing */
> +}
> +
> +/*
> + * Frame Relay link level subroutines.
> + * ANSI T1.617-compatible link management signaling implemented.
> + * Only one DLCI per channel for now.
> + * Copyright (C) 1994-1999 Cronyx Engineering Ltd.
> + * Author: Serge Vakulenko, <vak@cronyx.ru>
> + */
> +static void sppp_fr_input (struct sppp *sp, struct mbuf *m)
static void
sppp_fr_input (struct sppp *sp, struct mbuf *m)
{
...
This allows for "grep -e '^functionname' foo.c".
> +{
> + STDDCL;
> + u_char *h = mtod (m, u_char*);
Please initialilize and declare on differnt lines.
> +
> +/*
> + * Add the frame relay header to the packet.
> + * For IP the header length is 4 bytes,
> + * for all other protocols - 10 bytes (RFC 1490).
> + */
> +static struct mbuf *sppp_fr_header (struct sppp *sp, struct mbuf *m,
> + int family)
Same as above.
> + if (! m)
> + return 0;
I didn't mention this before because that was changing an old file, but
in this file you'll want
if (m == '\0') or at least if (!m)
> + h = mtod (m, u_char*);
> +
> + /* Fill the header. */
> + h[0] = sp->fr_dlci >> 2 & 0xfc;
> + h[1] = sp->fr_dlci << 4 | 1;
> + h[2] = PPP_UI;
> +
> + switch (family) {
> + default:
> + if (debug)
> + printf ("%s%d: cannot handle address family %d\n",
> + ifp->if_name, ifp->if_unit, family);
> + m_freem (m);
> + return 0;
Default belongs as the bottom.
> +/*
> + * Send periodical frame relay link verification messages via DLCI 0.
> + * Called every 10 seconds (default value of T391 timer is 10 sec).
> + * Every 6-th message is a full status request
> + * (default value of N391 counter is 6).
> + */
> +static void sppp_fr_keepalive (struct sppp *sp)
Same as above.
> + MGETHDR (m, M_DONTWAIT, MT_DATA);
> + if (! m)
> + return;
Same as above.
> +/*
> + * Process the frame relay Inverse ARP request.
> + */
> +static void sppp_fr_arp (struct sppp *sp, struct arp_req *req,
> + u_short his_hardware_address)
Same as above.`
> + switch (ntohs (req->op)) {
> + default:
> + if (debug)
> + printf ("%s%d: Invalid ARP op = 0x%x\n",
> + ifp->if_name, ifp->if_unit, ntohs (req->op));
> + return;
Default belongs at the bottom.
> + case ARPOP_INVREQUEST:
> + sppp_get_ip_addrs (sp, &my_ip_address, 0, 0);
> + if (! my_ip_address)
> + return; /* nothing to reply */
if (my_ip_address != '\0')
> + *(short*) (h+8) = htons (ETHERTYPE_ARP);
> +
> + reply->htype = htons (ARPHRD_FRELAY);
> + reply->ptype = htons (ETHERTYPE_IP);
> + reply->halen = 2;
> + reply->palen = 4;
> + reply->op = htons (ARPOP_INVREPLY);
> + reply->hsource = htons (my_hardware_address);
> + reply->psource1 = htonl (my_ip_address);
> + reply->psource2 = htonl (my_ip_address) >> 16;
> + reply->htarget = htons (his_hardware_address);
> + reply->ptarget1 = htonl (his_ip_address);
> + reply->ptarget2 = htonl (his_ip_address) >> 16;
All of those spaces between functionname(and_the, variables)
needs to be dropped.
Spaces only come after the keywords if, while, for, return and switch
Everything else uses functionname(variables)
Some of the same style snafus are made throughout the file, so I tried
not to point out every single one of them when they were repeated.
Please read style(9) for the source for most of these comments.
--
- bill fumerola - billf@chc-chimes.com - BF1560 - computer horizons corp -
- ph:(800) 252-2421 - bfumerol@computerhorizons.com - billf@FreeBSD.org -
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-bugs" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199911121710.JAA92308>