From owner-freebsd-hackers@FreeBSD.ORG Tue Feb 19 19:02:51 2013 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id A0EA0E32 for ; Tue, 19 Feb 2013 19:02:51 +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 802F6F6D for ; Tue, 19 Feb 2013 19:02:51 +0000 (UTC) Received: from pakbsde14.localnet (unknown [38.105.238.108]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id E6BDCB9BA; Tue, 19 Feb 2013 14:02:50 -0500 (EST) From: John Baldwin To: freebsd-hackers@freebsd.org Subject: Re: Looking for reviewers for patch that adds foreign disk support mfiutil Date: Tue, 19 Feb 2013 13:46:33 -0500 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p25; KDE/4.5.5; amd64; ; ) References: <49693195BAD841469129EE4B7523CABE@multiplay.co.uk> In-Reply-To: <49693195BAD841469129EE4B7523CABE@multiplay.co.uk> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201302191346.33415.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Tue, 19 Feb 2013 14:02:51 -0500 (EST) Cc: Steven Hartland 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: Tue, 19 Feb 2013 19:02:51 -0000 On Sunday, February 17, 2013 1:06:40 pm Steven Hartland wrote: > Hi all I'm looking for someone to review the attached patch > to mfiutil which adds foreign disk support to mfiutil as > per: > http://www.freebsd.org/cgi/query-pr.cgi?pr=172091 > > Any and all feedback welcome :) Some suggestions: - Please stick with FreeBSD style, e.g. please use: if (foo == NULL) rather than: if (NULL == foo) I understand the reasons for the latter style (turn accidental assignments into compile errors) but I don't buy them because 1) modern compilers can already catch such things, but most importantly 2) it doesn't read correctly. Above all else code should be readable, and one doesn't say "if NULL the pointer is" (unless one is Yoda), but "if the pointer is NULL". - Don't make dump_config() use a default prefix, just fix the existing call to dump_config() to pass in a prefix. - 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. - 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'? - 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) - Leaving out the 'MFI_DCMD_' prefix from the opcode description was intentional. If you are ever fortunate enough to examine the manuals from LSI, they refer to the firmware commands as 'LD_CONFIG', etc. (Maybe it's 'MR_LD_CONFIG'?) The MFI_DCMD_ prefix is specific to the FreeBSD driver. - 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) - Reindent the call to mfi_ldprobe() if CFG_ADD or CFG_FOREIGN_IMPORT succeeds. -- John Baldwin