Looking for reviewers for patch that adds foreign disk support mfiutil

[John Baldwin]

On Tuesday, February 19, 2013 6:49:52 pm Steven Hartland wrote:
> ----- Original Message ----- 
> From: "John Baldwin"
> 
> Thanks for the feedback John appreciated, a couple of questions inline
> below if you would be so kind.

Certainly.

> > - Is dump_config() really the right choice for 'foreign config'?  It doesn't
> >  attempt to output things very pretty, and I think mfiutil's non-debug
> >  commands should aim to be human readable.
> 
> Will check this, just didn't want to re-invent the wheel ;-)

Heh, can you reuse the show_config code instead perhaps?  It might be useful if
you could provide an example of the current 'foreign config' output?

> > - This (human readable) is also why it doesn't include the opcode in the error
> >  message by default.  Sysadmins don't really care which opcode fails.  Maybe
> >  put that under '#ifdef DEBUG'?
> 
> Previously there was no information about what command failed, which made
> the failure message kinda useless, so while debugging I added the opcode
> to help me trace things.

In general my goal had been to make the caller provide that level of detail if
it is useful.  While developing a command it can indeed be useful which is why
I suggested moving it under #ifdef DEBUG.  This provides the extra detail while
working on a command while keeping the UI for users clean.  If it is under DEBUG
you can just print the raw opcode in hex as you are doing now.

> > - mfireg.h should be kept in sync with the driver's version of that header, so
> >  don't reorder the enum's unless you are changing it to match what is in the
> >  device driver's mfireg.h.  In fact, mfiutil should probably be using the
> >  mfireg.h from sys/dev/mfi directly now that it is in the tree.  (mfiutil
> >  was originally developed outside of the tree as a standalone app)
> 
> There is only one mfireg.h and that is already in sys/dev/mfi :)

Oh, bah.  I misread the diff.  Reordering the commands looks good to me in that
case.

> > - Please don't do assignments in declarations and leave a blank line between
> >  declarations and the bode of code.  Thus:
> > 
> >     mfi_op_desc(...)
> >     {
> >         int i, num_ops;
> > 
> >         num_ops = nitems(mfi_op_codes);
> >         ...
> > 
> >  (nitems() is nice to use when it is available as well)
> 
> Changed, this the case for constant initialisers too? e.g. is the
> following incorrect or acceptable?
> myfunc(...)
> {
>     int i = 0, j = 1;
> ...

style(9) forbids those as well (and I generally avoid them myself), but you
will find code in the tree that does use initializers for simple expressions.

-- 
John Baldwin