Hi Bill and DDU Team.
In general, most of the files look really good now! Thanks for adding
comments to most files, and for creating the libddudev.c library of
common functions. This makes the code much more maintainable than before.
Changes to a few files near the bottom of the list were not done.
In this pass I have overlooked the less-important things. I have
included comment requests if they are easy, and code requests which I
made before and were not done. (Fortunately these changes are easy and
straightforward.) Please fix these or else state why you don't want to
change them.
Please do this and get changed files back to me ASAP to review so we can
complete this review.
Below are my specific comments:
all_devices/Makefile:
- Thanks for making a library ddudev.
- Please remove the $(DBX) reference on line 27:
CFLAGS +=
all_devices/all_devices.c:
115: typo Ouput -> Output
all_devices/all_devices.h: OK
ids.c:
588: "must great 0" -> "must be > 0"
ids.h: OK
bat_detect/Makefile:
27: CFLAGS +=
bat_detect.c:
143: indentation.
cd_detect/Makefile:
27: CFLAGS +=
cd_detect.c:
134: meida => media
639: Please add a comment here.
dmi/Makefile: OK
dmi/i386/Makefile:
27: CFLAGS +=
i386/bios_info.c: OK
cpuid.s:
This file really needs comments. (I know, after Nevada Dock putback...)
cpuid_info.c:
103 - 107: can be optimized to:
while (isblank(cpu_name[i])) {
i++;
}
cpuid_info.h: OK
dmi/i386/dmi.c:
77: This comment is confusing. I think you mean:
"If not NULL, start looking from this node."
dmi/i386/dmi.h: OK
dmi/i386/dmi_info.c: OK
dmi/i386/dmi_info.h: OK
dmi/i386/mb_info.c: OK
dmi/i386/mb_info.h: OK
dmi/i386/mem_info.c: OK
dmi/i386/mem_info.h: OK
dmi/i386/processor_info.c: OK
dmi/i386/processor_info.h: OK
dmi/i386/sys_info.c: OK
dmi/i386/sys_info.h: OK
dmi/sparc/Makefile:
27: CFLAGS +=
dmi/sparc/dmi_info.c:
197, 221,234: PRINTF("Invalid int/uint/float size\n");
257: PRINTF("Unknown type\n");
845-874 is the same as 736-765: Please make a common function.
1063: Please add a comment on why get_phy_mem_size() is called or
sysconf() is called. Without knowing the reason, I would just use
sysconf() as it is easier.
dmi/sparc/dmi_info.h: OK
find_driver/Makefile:
27: CFLAGS +=
find_driver.c:
58-59: split a line like this:
printf ("this is "
"a long line");
so that you can indent the second line properly.
257, 310-317:
257: Driver_status is not necessary.
310: Not needed
311, 314, 317: replace these lines similar to:
if (tmparray[2] == 'T') {
or use a switch statement:
switch (tmparray[2]) :
case 'T':
hd_detect/Makefile:
27: CFLAGS +=
disk_info.c:
62: Based on the old version of the file, I think rqbuf for scsi
commands can be 32 chars in size.
62: Please #define RQBUF_SIZE 255, then use this #define on 62, 83, 109
Does scsi_cmd zero out rq_buf before it fills it in?
79-81, 94-96: Can make one command:
ucmd.uscsi_flags = USCSI_ISOLATE | USCSI_SILENT | USCSI_READ;
hd_detect.c:
274: REMOVE_MEIDA -> REMOVE_MEDIA
(needs to be changed also in hd_detect.h)
343, 402: Hardcoded value 15
Use sizeof(CD_MINOR_TYPE) or just regular strcmp
366-367: inconsistent indentation of arguments relative to other
functions in this module.
I suggest taking the code at 505-516 and putting it above the switch.
Something like :
switch (c){
case 'l':
case 'r':
case 'c':
root_node = di_init(....)
...
break;
default:
break;
}
switch (c) {
The old switch code less the code moved to above.
}
hd_detect.h (not part of this webrev, but part of the last one):
47: MEIDA -> MEDIA
mpath.c:
64: double (())
168-169: indentation. Note, you can split lines on a "."
mpath.h:
My comment about include files in include files still stands, but OK
libddudev.h:
I don't think these are used anywhere...
#define PROP_DEV_TYPE "device_type"
#define PROP_CPU_BRAND "brand-string"
#define PROP_CPU_CLOCK "clock-frequency"
#define PROP_CPU_MHZ "cpu-mhz"
#define PROP_INIT_PORT "initiator-port"
The following is not used, except in hd_detect.c. hd_detect.c includes
hd_detect.h where it is also defined.
#define BLOCK_TYPE
#define CD_MINOR_TYPE
#define REMOVE_MEIDA
Not used anywhere:
USB_VENDOR_NAME
... but maybe it is included out of completeness as USB_PRODUCT_NAME is
used?
lib/Makefile:
27 CFLAGS = -m32 -Kpic
(can this be +=?)
lib/libddudev.c:
scsi_cmd(): A call can take potentially 30 seconds + the calls to the
scsi ioctl. If scsi_cmd() is called in a loop, this time can really add
up. Can we time out and return sooner?
Thanks,
Jack
On 03/03/10 04:30, Bill Yan wrote:
> Thanks a lot for the code review. We've updated the code based on the
> feedback and really appreciate if you could review it again. The
> following is the webrev URL:
>
> http://cr.opensolaris.org/~billyan/DDU_1.3.1/webrev_20100303/
>
>
> Thanks,
> Bill
> Bill Yan ???:
>
>> 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
>>
>>
>
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
