From owner-freebsd-hackers@FreeBSD.ORG Wed Feb 20 15:49:14 2013 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id A95E21D3; Wed, 20 Feb 2013 15:49:14 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (bigknife-pt.tunnel.tserv9.chi1.ipv6.he.net [IPv6:2001:470:1f10:75::2]) by mx1.freebsd.org (Postfix) with ESMTP id 4553DD7D; Wed, 20 Feb 2013 15:49:14 +0000 (UTC) Received: from pakbsde14.localnet (unknown [38.105.238.108]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 9B02FB9B9; Wed, 20 Feb 2013 10:49:13 -0500 (EST) From: John Baldwin To: "Steven Hartland" Subject: Re: Looking for reviewers for patch that adds foreign disk support mfiutil Date: Wed, 20 Feb 2013 08:40:32 -0500 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p25; KDE/4.5.5; amd64; ; ) References: <49693195BAD841469129EE4B7523CABE@multiplay.co.uk> <201302191346.33415.jhb@freebsd.org> <70A4A7995FBD430EA37B56C0C8B32B34@multiplay.co.uk> In-Reply-To: <70A4A7995FBD430EA37B56C0C8B32B34@multiplay.co.uk> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201302200840.32641.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Wed, 20 Feb 2013 10:49:13 -0500 (EST) Cc: freebsd-hackers@freebsd.org X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 Feb 2013 15:49:14 -0000 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