Hi everyone.
Here is the last installment of my code review comments for DDU C code:
sparc/dmi_info.c:
Module header and function headers needed.
u8/u16/u32 instead of uint8_t, uint16_t, uint32_t
This file uses
#define PRINTF (void)printf
and I like that.
However, i386/dmi_info.c and other modules don't. Please be consistent,
and either use it everywhere or don't use it at all.
39, 40: nit: make these static.
66,70,76,83: if this function fails, you won't know why. Returning
failure values if possible (may not be possible here due to how function
is called) or at least printing a message for each failure would be helpful.
110: Would someone who saw "err" know what happened? A more descriptive
message would be useful.
print_bios_info and print_system_info share a lot of common code.
Suggest a common function with an argument that takes either prom_info
or system_info.
249: I think this needs to change to Oracle or maybe Sun/Oracle. Check
with Frank.
232-234: Just curious: Is this needed so that SPARC output has the same
number of lines as x86?
281: What does args contain? Please add a comment.
283: Don't test an int as if it was a boolean.
288: Not needed. i will be 0.
321: How does this function work? Please add a comment to explain it.
I don't really understand the logic so I cannot comment well on it.
361-362: I suggest initializing these values where they are declared
(325, 333) and getting rid of these two lines.
399: I don't understand the logic here. Please add a comment to explain it.
473, 479: Do you mean "number of CPUs" rather than "CPU number"? npkg
can be initialized by sysconf(_SC_NPROCESSORS_CONF).
523: what's so special about i == 1 ? Please document this or use a
#define instead of a 1.
556, 561, 565, 571, 578, 603: If message can go to stderr here, someone
can figure out which of the five failures occurred.
650-677: This code is identical to 553-580. Please make a common
function out of this and call it from get_mem_bank_size() and
prt_mem_bank_size(), or else implement one function by calling the other
one.
745, 786: opt_pro is left out of the "all" case. Is this intentional?
832-838: Please add a comment why these lines are here.
853,885-886: Is get_phy_mem_size() more accurate than sysconf to get
memory size? If not, why not just call sysconf? It is simpler and thus
easier to maintain and understand.
sparc/dmi_info.h:
use uint8_t, uint16_t, uint32_t, uint64_t from sys/types.h instead of
u8/u16/u32/u64.
Don't declare static items in header files. (See second installment of
code review email for details.)
find_driver/Makefile:
27-28: get rid of DBX references.
find_driver.c:
Module header and function headers needed.
For the future, you can use the second arg passed to check_dev to pass a
struct containing all the global variables. This would make your code
easier to follow.
58-62: Make these #defines to save memory, since these are constants
66-67: Avoid using global variables when not necessary. Declare these
in main(), and call find_driver and find_link with pointers to them as
arguments.
72,74: can combine:
char *str = arg;
76: don't treat a string like a boolean. do:
while ((str = strchr(str, " ")) != NULL) {
89-90: split a line like this:
printf ("this is "
"a long line");
so that you can indent the second line properly.
95: check_dev_option() is identical to all_devices.c/process_arg(). Why
not make a library of common functions and call them from multiple programs?
173: get_pci_path() is identical to name, form and function of
get_pci_path in all_devices.c. Put this in a common library.
same for lookup_node_ints()
same for lookup_node_strings()
same for match_by_pci_path()
same for match_by_dev_path()
compare_dev_option() matches dev_match() in all_devices.c
Put all of these in a common library and call the library from this
module and all_devices.c.
360: Don't start a line with an operator.
390-394: return ((found > 0) ? 0 : 1);
442: Use a #define for 100, and then...
497, 500, 503: use strncmp instead of strcmp to make sure you don't go
out of bounds.
hd_detect/Makefile:
Module header and function headers needed.
Remove references to DBX
hd_detect/disk_info.c:
scsi_cmd here is the same as scsi_cmd of cd_detect.c. Put into library.
57: Actually there is one difference between cd_detect.c and here:
uscsi_rqlen is 255 here, only 32 in cd_detect.c. Please set it here to
32. (Looks like it may be settable to 13 since nothing higher than
rqbuf[13] is referenced, but I'm not sure.)
118-120: Can make one command:
ucmd.uscsi_flags = USCSI_ISOLATE | USCSI_SILENT | USCSI_READ;
hd_detect/disk_info.h:
Thanks for using uint64_t.
Looks good!
hd_detect/hd_detect.c:
Module header and function headers needed.
46 - 73 Please declare as static, all globals and functions which are
used only by this module.
90: prt_info() is identical to prt_info() in all_devices.c
121: Same for process_arg().
164: Same for lookup_node_ints().
193: Same for lookup_node_strings().
220: Same for get_inq_ven_info().
245: Same for get_inq_pro_info().
270: Same for get_usb_pro_info().
295: Same for get_devid_info().
(286: function name in comment is incorrect in both files)
310: Same for get_pci_path().
620: Same for match_by_pci_path().
641: Same for match_by_dev_path().
700: Same for dev_match().
Put all of these functions in a library called from both modules.
The maintainers of this code will thank you!!!
377-378: Can save yourself a nesting level and say:
if (i == DEVID_INFO) {
...
} else if (i < INFO_END) {
...
} else {
ret = 1;
}
Parts of prt_hd_info() are shared with all_devices.c/prt_dev_info().
Consider putting the common parts into a single routine and calling it
from both functions.
488: REMOVE_MEIDA -> REMOVE_MEDIA
(needs to be changed also in hd_detect.h)
532, 590: Hardcoded value 15
Use sizeof(CD_MINOR_TYPE) or just regular strcmp
546: What does this function do? It looks like it calls
lookup_mpath_dev() via di_walk_minor(). lookup_mpath_dev() calls
check_mpath_link() with a minor path. check_mpath_link() checks a link
but doesn't store a value. Where is the output? I must have missed
it... Please add comments explaining how this works...
566-567: inconsistent indentation of arguments relative to other
functions in this module.
790: To be consistent with other functions, please return a status to
main, and have main exit on error.
There is no usage() function. Having one would help document what this
module does.
hd_detect.h:
47: REMOVE_MEIDA -> REMOVE_MEDIA
49-51: not used. Can remove
mpath.c:
Module header and function headers needed.
I have no clue what this module does, so cannot review functionality.
Needs comments and longer, more descriptive variable names.
54: double (())
55,57,61,64,65,70,97,98,111,112,134,137: You can cut down on lines of
code by combining
status = MP_...();
if (status == MP_STATUS_SUCCESS) {
into:
if (MP_...() == MP_STATUS_SUCCESS) {
140-141: indentation. Note, you can split lines on a "."
173: Please document which globals this function affects and how.
(Do this for all functions which affect globals.)
mpath.h:
33-34: It's preferred not to have include files including other files.
media_event/Makefile:
Module header and function headers needed.
Remove references to DBX
media_event.c:
Module header and function headers needed.
58: "Error opening mnttab"
66, 75, 76: Clarify code by
#define 0 IMMEDIATE
or
#define -1 WAIT
and call poll using these values, for example:
poll(&fds, 1, IMMEDIATE);
66: If /etc/mnttab happens to disappear, this program will go into an
infinite loop!
Finally, one thing I noticed about all_devices.c while reviewing the
files above:
all_devices.c: 2060: DINFOFORCE is a private, undocumented bit. Your
code could break if it is changed.
Thanks,
Jack
On 02/10/10 03:05, Bill Yan wrote:
> Here is the code review for Device Driver Utility(DDU) changes for
> Driver Update.
>
> Driver Update will allow the installer to install packages of needed
> drivers in its own boot environment before performing the installation,
> and then install those same packages on the target.
>
> DDU provides 3 methods for user to install the missing driver package:
>
> 1. GUI mode
> On live-cd mode, user install the missing driver package by DDU with GUI
> 2. Text mode
> User install the missing driver package by DDU test mode during the text
> installation.
> 3. AI mode
> DDU provides the common library for AI to install missing driver package
> during the AI installation.
>
>
>
> http://cr.opensolaris.org/~billyan/DDU_1.3.1/webrev/ddu_1.3.1_UI_webrev/
>
> DDU GUI, start with ddu.py
>
> http://cr.opensolaris.org/~billyan/DDU_1.3.1/webrev/ddu_1.3.1_text_webrev/
>
> DDU text mode, start with ddu-text.py
>
> http://cr.opensolaris.org/~billyan/DDU_1.3.1/webrev/ddu_1.3.1_AI_webrev/
>
> Common library interface used by DDU GUI, text-mode and AI
>
> http://cr.opensolaris.org/~billyan/DDU_1.3.1/webrev/ddu_1.3.1_scripts_webrev/
>
> Common library functions used by common library interface.
>
>
> http://cr.opensolaris.org/~billyan/DDU_1.3.1/webrev/ddu_1.3.1_tools_webrev/
>
> Common library functions used by common library interface.
>
>
> Please be noted the Makefiles and packaging will be rehashed when the DDU goes
> back to ON in April.
>
> Thanks a lot,
> Bill
>
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>
