Date: Tue, 19 Feb 2013 23:49:52 -0000 From: "Steven Hartland" <smh@freebsd.org> To: "John Baldwin" <jhb@freebsd.org>, <freebsd-hackers@freebsd.org> Subject: Re: Looking for reviewers for patch that adds foreign disk support mfiutil Message-ID: <70A4A7995FBD430EA37B56C0C8B32B34@multiplay.co.uk> References: <49693195BAD841469129EE4B7523CABE@multiplay.co.uk> <201302191346.33415.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
----- Original Message ----- From: "John Baldwin" Thanks for the feedback John appreciated, a couple of questions inline below if you would be so kind. > 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". Acknowledged, I'm working through some old patches so need to bring them in line, will check for others like this. > - Don't make dump_config() use a default prefix, just fix the existing call > to dump_config() to pass in a prefix. Done. > - 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 ;-) > - 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 order to keep it more user friendly I'm thinking of using mfi_op_desc, but this is currently only available under MFI_DEBUG from mfi_debug.c. Would it be better to move this else where e.g. mfireg.h so human readable command information is always available? > - 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 :) > - 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. Thanks for the info, changed. > - 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; ... > - Reindent the call to mfi_ldprobe() if CFG_ADD or CFG_FOREIGN_IMPORT > succeeds. Nice spot, sorted. Regards Steve
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?70A4A7995FBD430EA37B56C0C8B32B34>