Date: Wed, 20 Feb 2013 08:40:32 -0500 From: John Baldwin <jhb@freebsd.org> To: "Steven Hartland" <smh@freebsd.org> Cc: freebsd-hackers@freebsd.org Subject: Re: Looking for reviewers for patch that adds foreign disk support mfiutil Message-ID: <201302200840.32641.jhb@freebsd.org> In-Reply-To: <70A4A7995FBD430EA37B56C0C8B32B34@multiplay.co.uk> References: <49693195BAD841469129EE4B7523CABE@multiplay.co.uk> <201302191346.33415.jhb@freebsd.org> <70A4A7995FBD430EA37B56C0C8B32B34@multiplay.co.uk>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201302200840.32641.jhb>