Date: Wed, 11 Nov 2009 20:25:19 -0700 From: "Justin T. Gibbs" <gibbs@scsiguy.com> To: Attilio Rao <attilio@freebsd.org> Cc: gibbs@freebsd.org, freebsd-current@freebsd.org, Scott Long <scottl@freebsd.org>, Ed Maste <emaste@sandvine.com> Subject: Re: [PATCH] Adding sysctl for errors statistics to ahd(4) Message-ID: <4AFB801F.5020806@scsiguy.com> In-Reply-To: <3bbf2fe10911111746g48c8fb82r74744dffedf49758@mail.gmail.com> References: <3bbf2fe10911060643g60079e31y7679b32dac670fd8@mail.gmail.com> <4AFAEE89.3020009@scsiguy.com> <3bbf2fe10911111746g48c8fb82r74744dffedf49758@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 11/11/2009 6:46 PM, Attilio Rao wrote: > 2009/11/11 Justin T. Gibbs<gibbs@scsiguy.com>: >> On 11/6/2009 7:43 AM, Attilio Rao wrote: >>> This patch introduces some mechanisms for collecting informations on >>> errors frequency and debugging (and relative sysctls for printing them >>> out) for the ahd(4) driver: >>> http://www.freebsd.org/~attilio/Sandvine/STABLE_8/ahd/ahd-current2.diff >>> >>> The usage of array for sysctls is a bit too paranoid but it allows for >>> further extendibility of the code and doesn't loose of cleaness. >>> This code has been contributed back by Sandvine Incorporated with some >>> cleanups. >>> Please review. >>> >>> Thanks, >>> Attilio >> >> In general, I think the patch is fine. It violates the existing style >> of the driver in some places (e.g. the aic7xxx drivers wrap function >> arguments to the opening '(' not to a 4 space indent), which should >> probably be addressed so the code remains consistent. > > Sorry, I cannot find where these existing style breakage happens, > could you be a bit more precise? > (I just found an un-sorted header introduction in aic79xx.h that I will fix). For example this code: + ahd->sysctl_tree[AHD_SYSCTL_ROOT] = SYSCTL_ADD_NODE( + &ahd->sysctl_ctx[AHD_SYSCTL_ROOT], SYSCTL_STATIC_CHILDREN(_hw), + OID_AUTO, device_get_nameunit(ahd->dev_softc), CTLFLAG_RD, + 0, ahd_sysctl_node_descriptions[AHD_SYSCTL_ROOT]); + SYSCTL_ADD_PROC(&ahd->sysctl_ctx[AHD_SYSCTL_ROOT], + SYSCTL_CHILDREN(ahd->sysctl_tree[AHD_SYSCTL_ROOT]), OID_AUTO, + "clear", CTLTYPE_UINT | CTLFLAG_RW, ahd, 0, ahd_clear_allcounters, + "IU", "Clear all counters"); should look like this, not style(9), to match the existing style of the driver: ahd->sysctl_tree[AHD_SYSCTL_ROOT] = SYSCTL_ADD_NODE(&ahd->sysctl_ctx[AHD_SYSCTL_ROOT], SYSCTL_STATIC_CHILDREN(_hw), OID_AUTO, device_get_nameunit(ahd->dev_softc), CTLFLAG_RD, 0, ahd_sysctl_node_descriptions[AHD_SYSCTL_ROOT]); SYSCTL_ADD_PROC(&ahd->sysctl_ctx[AHD_SYSCTL_ROOT], SYSCTL_CHILDREN(ahd->sysctl_tree[AHD_SYSCTL_ROOT]), OID_AUTO, "clear", CTLTYPE_UINT|CTLFLAG_RW, ahd, 0, ahd_clear_allcounters, "IU", "Clear all counters"); -- Justin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4AFB801F.5020806>