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

2009/11/12 Justin T. Gibbs <gibbs_at_scsiguy.com>:
> 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");
>

Could you check if this revision of the patch is adeguate?:
http://www.freebsd.org/~attilio/Sandvine/STABLE_8/ahd/ahd-current3.diff

Thanks,
Attilio


-- 
Peace can only be achieved by understanding - A. Einstein