Re: [patch 0/1][mdadm] Fix needed to enable RAID volumes on SAS devices (version 2).

[Neil Brown <neilb@suse.de>]

On Mon, 30 Nov 2009 16:12:13 +0100
Artur Wojcik <artur.wojcik@intel.com> wrote:

> Neil/Andre,
> I incorporated the suggestions Andre gave me and this resulted in the
> second version of patch. However I decided to not use pathconf()
> function but to define PATH_MAX instead. It is just the size of a buffer
> and I would like this to be resolved during compilation time rather then
> dynamically during execution time. Please review it and let me know if
> something needs to be changed.
> 
> As for the being paranoid... so I really am, if we talk about software
> security and vulnerability. I decided to introduce a new __str_fmt()
> function and str_fmt() helper macro. Both are documented in source code
> (let me know if you need separate patch for this).
> 

I'm afraid that I really don't like this.
Creating little functions that do almost-but-not-quite-exactly what some
standard library function does always bothers me.  It makes the code harder
to read.
If you just used it in 2 or 3 places where you actually needed it then it
might be OK, but you have used it everywhere, including several places where
it isn't needed at all as the buffer is known to be big enough.

And I don't like the introduction of a new header file just to store one or 2
definitions.  Just put new definitions in mdadm.h  Keep It Simple.

I am perfectly happy with making 'buf' larger as appropriate, even making it
PATH_MAX in some cases.
I would be happy with more use of asprinf.
I would be happy adding checks before certain critical sprintf calls that
the result will not exceed the buffer.

But I'm afraid that I don't like str_fmt at all.

Can we just fix the bits that are actually broken please?

NeilBrown