Date: Thu, 12 Feb 2015 22:06:26 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: "Pedro F. Giffuni" <pfg@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r278589 - head/usr.sbin/flowctl Message-ID: <20150212212417.U1109@besplex.bde.org> In-Reply-To: <201502111746.t1BHkZqt022491@svn.freebsd.org> References: <201502111746.t1BHkZqt022491@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 11 Feb 2015, Pedro F. Giffuni wrote:
> Log:
> flowctl: Replace alloca() with an array.
>
> Reviewed by: glebius
>
> Modified:
> head/usr.sbin/flowctl/flowctl.c
>
> Modified: head/usr.sbin/flowctl/flowctl.c
> ==============================================================================
> --- head/usr.sbin/flowctl/flowctl.c Wed Feb 11 17:41:23 2015 (r278588)
> +++ head/usr.sbin/flowctl/flowctl.c Wed Feb 11 17:46:35 2015 (r278589)
> @@ -222,12 +222,10 @@ ctl_show(int argc, char **argv)
> static void
> do_show(int version, void (*func)(struct ngnf_show_header *))
> {
> - struct ng_mesg *ng_mesg;
> + struct ng_mesg ng_mesg[SORCVBUF_SIZE];
> struct ngnf_show_header req, *resp;
> int token, nread;
>
> - ng_mesg = alloca(SORCVBUF_SIZE);
> -
> req.version = version;
> req.hash_id = req.list_id = 0;
It is surprising that this even compiles. It replaces the allocation
of 1 (variable-size, using a perhaps-standard form of the struct hack)
struct of size SORCVBUF_SIZE with that of SORCVBUF_SIZE (fixed-size)
structs, each of too-small size. This accidentally allocates enough
storage since 1 * SORCVBUF_SIZE < SORCVBUF_SIZE * sizeof(any).
It changes the variable type significantly, from a pointer to 1 struct to
the bogus array.
In 1 place, the array decays to a pointer to its first element without
too much magic. (The magic is larger than usual since the struct hack
is used. The first element is too small and is overrun. This works
because the pointer isn't really a pointer to a struct.)
In 4 places, the array decays to a pointer to its first element more
magically. It is surprising that ng_mesg->header means the same thing
as ng_mesg[0].header when ng_mesg is an array.
Using alloca() was ugly, but correctly reflected what was going on.
It is an optimization of mallocing the struct. VLAs don't work as
well as alloca() for variable-sized structs. I don't see how they
can be used at all. Since we allocate the maximum size needed, we
don't even need a VLA, and could almost use
u_char buf[SORCVBUF_SIZE]
struct ng_mesg *ng_mesg;
ng_msg = (struct ng_mesg *)buf;
but this doesn't guarantee the necessary alignment like malloc() and
alloca() do. It also takes more code than alloca(). To get the
necessary alignment, you would have to use some alignment hack,
e.g., put buf in a union with the struct (only works for fixed-size
allocation), or perhaps use an array not quite as in this commit:
struct ng_mesg buf[howmany(SORCVBUF_SIZE, sizeof(struct ng_mesg))];
struct ng_mesg *ng_mesg;
ng_msg = &buf[0];
This form works using VLAs (SORCVBUF_SIZE can be variable), but is much
uglier than using alloca().
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150212212417.U1109>