Re: [PATCH] Adding sysctl for errors statistics to ahd(4)

On 11/11/2009 6:46 PM, Attilio Rao wrote:
> 2009/11/11 Justin T. Gibbs<gibbs_at_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