Date: Fri, 14 Mar 2014 08:07:07 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: John Baldwin <jhb@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r263113 - head/sys/x86/x86 Message-ID: <20140314073741.T978@besplex.bde.org> In-Reply-To: <201403131811.s2DIBgUb019577@svn.freebsd.org> References: <201403131811.s2DIBgUb019577@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 13 Mar 2014, John Baldwin wrote:
> Log:
> Correct type for malloc().
>
> Submitted by: "Conrad Meyer" <conrad.meyer@isilon.com>
Not so nice a cleanup as a previous one by the same author.
> Modified: head/sys/x86/x86/mca.c
> ==============================================================================
> --- head/sys/x86/x86/mca.c Thu Mar 13 16:51:40 2014 (r263112)
> +++ head/sys/x86/x86/mca.c Thu Mar 13 18:11:42 2014 (r263113)
> @@ -700,8 +700,8 @@ cmci_setup(void)
> {
> int i;
>
> - cmc_state = malloc((mp_maxid + 1) * sizeof(struct cmc_state **),
> - M_MCA, M_WAITOK);
> + cmc_state = malloc((mp_maxid + 1) * sizeof(struct cmc_state *), M_MCA,
> + M_WAITOK);
This still spells the element type verbosely and non-robustly as
<element type> instead of as <element object>. Better spelling:
cmc_state = malloc((mp_maxid + 1) * sizeof(*cmc_state), M_MCA,
M_WAITOK);
The variable names are confusing. cmc_state is a doubly-indirect pointer,
but is spelled without a single p. This was confusing enough to give the
original bug (but since all struct pointers have the same size even in
strictly portable C, this was only a style bug). It is unclear what sort
of a pointer cmc_state is in the changed version, but using the correct
spelling automatically gets the number of stars right (since the size being
malloc()ed has a product term with a variable, the allocation must be for a
dynamic array and the element type must need a single star to modify the
result type; this star actually removes 1 of the 2 indirections, giving
a pointer to the basic struct type).
Local pointers to struct cmc_state are mostly spelled cc.
Struct member names in struct cmc_state are missing prefixes.
> for (i = 0; i <= mp_maxid; i++)
> cmc_state[i] = malloc(sizeof(struct cmc_state) * mca_banks,
> M_MCA, M_WAITOK | M_ZERO);
SImilarly, plus the order of terms in the multiplication is gratuitously
different. I prefer the first order:
cmc_state[i] = malloc(mca_banks * sizeof(*cmc_state[i]),
M_MCA, M_WAITOK | M_ZERO);
There are 2 other malloc()s in the file. These use the best spelling
for the sizeof() but not for variable names.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140314073741.T978>