On Mon, Nov 8, 2010 at 6:58 AM, John Baldwin <jhb@freebsd.org> wrote:
quoted text
> On Saturday, November 06, 2010 4:13:23 am Garrett Cooper wrote:
>>     Similar to r214396, this patch deals with properly capturing error
>> and passing it up to the caller in mptutil just in case the errno
>> value gets stomped on by warn*(3); this patch deals with an improper
>> use of warn(3), and also some malloc(3) errors, as well as shrink down
>> some static buffers to fit the data being output.
>>     If someone could review and help me commit this patch it would be
>> much appreciated; all I could do is run negative tests on my local box
>> and minor positive tests on my vmware fusion instance because it
>> doesn't fully emulate a fully working mpt(4) device (the vmware
>> instance consistently crashed with a warning about the mpt
>> controller's unimplemented features after I poked at it enough).
>>     I'll submit another patch to fix up style(9) in this app if requested.
>> Thanks!
>
> The explicit 'return (ENOMEM)' calls are fine as-is.  I do not think they need
> changing.

    I'll changed back all of the other *alloc calls (I was just
thinking about all of the other awesome cases that malloc can fail,
but if we break that assumption a lot more of our programs would break
too unfortunately :(...).
    I read up on cam_getccb and I agree based on the description today:

     "cam_getccb() allocates a CCB using malloc(3) and sets fields in the CCB
     header using values from the cam_device structure."

    But if it called something else in cam_getccb other than malloc it
might confuse the end-user if it failed *shrugs*... But whatever, that
would have to be a wholesale change if things changed in this area so
I'll default to being lazy now :) (I'm almost positive Scott did that
because he understands the overall system a lot better than me :D..).

quoted text
> Having static char arrays of '15' rather than '16' is probably pointless.  The
> stack is already at least 4-byte aligned on all the architectures we support,
> so a 15-byte char array will actually be 16 bytes.  It was chose to be a good
> enough value, not an exact fit.  An exact fit is not important here.

    Yeah, that was stupid micro-optimization on my part.

quoted text
> Moving the 'buf' in mpt_raid_level() is a style bug.  It should stay where it
> is.  Same with 'buf' in mpt_volstate() and mpt_pdstate().

    Same as above.

quoted text
> IOC_STATUS_SUCCESS() returns a boolean, it is appropriate to test it with !
> rather than == 0.  It is also easier for a person to read the code that way.

    Good point. I didn't catch that part in style(9) before now.
Thanks for the review!
-Garrett