Hi Bill and Tony.
Here are my comments on the tools webrev.
all_devices.c: see attachment. Look for "JAS" to find my comments.
all_devices.h: OK
ids.c: see attachment. Look for "JAS" to find my comments.
ids.h:
80, 89: In header files, it is OK to put the function return type on the
same line as the function name.
bat_detect/Makefile:
Remove the DBX line and reference to $(DBX)
bat_detect.c:
Please create a header for this file, and document how to call this
program. Document what the possible arguments do. List what the
different return statuses mean.
Please add comments to this file, especially before calls to functions
which most are unfamiliar with, such as lines 94, 98, 102... For
example, what is hal_ctx for?
Please add headers to all functions.
cd_detect/Makefile:
Remove the DBX line and reference to $(DBX)
cd_detect.c:
Please create a header for this file, and document how to call this
program. Document what the possible arguments do. List what the
different return statuses mean.
Document how this program works, that it uses uscsi.
46-48: What do these variables do? Add a comment.
59: RETRIES is 10, WAIT_TIME is 3, so that's up to 30 seconds for one
call to scsi_cmd, not counting the command time itself. timeout in each
scsi_cmd is 30 (lines 126, 160, 191, others), so that adds another 30
seconds per call. With one minute per call per retry, a user could wait
up to 10 minutes before a particular scsi operation with retries fails.
This is too long.
I suggest reducing the number of retries. Does the 30 second scsi
command timeout account for spin-up times? If so, why is an additional
30 second wait needed before retrying? If it is possible to display an
error message between retries, do that too, so that the user knows what
is happening.
74-77, 82-83, 139-141: Explain in a comment what these values translate
into: what scsi status do they mean...
Alternatively, #define names for these values and use the names
instead. The names will describe what the values represent.
105, other places referencing uscsi_timeout: #define a timeout value at
the top, and use it throughout. Then if you have to change it, you need
change it in only one place.
108-114: why not just return (scsi_cmd(fd, &cmd)) ? All the caller
checks for is whether 0 or non-zero is returned.
Just checking: on 110, error is returned if scsi_cmd returns < 0. But
the checking of lines 137-142 are done only if scsi_cmd returns < 0. Is
this correct?
165, 166, 196,224,251,252,277: what do these values mean?
Some functions are similar enough that the could be combined into one.
Example, read_toc() and read_disc_info() are almost identical except for
the values the scsi cdb is initialized with. This would make your code
much smaller and thus easier to maintain.
172-178: same comment as for 108-114.
201-207: same comment as for 108-114.
228-234: same comment as for 108-114.
257-263: same comment as for 108-114.
286-290: same comment as for 108-114.
359 and so on: Messages in this file probably need to be localized using
gettext().
450: Please add a comment that you are looking at slice 2.
453, 455: Please #define what 0 and 1 are here and use the names instead
of hardcoded numbers.
530: Please use RETRIES instead of a hardwired number.
530: Having 5 retries, with each retry of 30 seconds timeout, adds up to
2.5 minutes -- per device. This is too long. I suggest reducing the
number of retries to two. The 30 second retry length is long, but may be
needed to allow for CDs to spin up, etc. However, if the length can be
shortened, please do it. If it is possible to display an error message
between retries, do that too, so that the user knows what is happening.
cd_detect.h:
37-39: Instead of u8, u16, u32, please use uint8_t, uint16_t, uint32_t
of int_types.h. (Include <sys/types.h> to get these definitions.)
I would delete this file and just include the few needed definitions
into cd_detect.c, since cd_detect.c is the only file including this file.
dmi/Makefile:
I think you can get rid of ARCH macro here, as it is already in
src/include Makefile.common.
dmi/i386/Makefile:
Get rid of DBX lines.
bios_info.c:
Same general comments about adding a header for the module, and adding
headers for all functions.
This module has no comments except on line 27. Please add comments.
74-75: suggest doing what was done above (for example, line 64).
(void)printf (" BIOS Revision:")
if ((info->bios_major_rev......) {
(void) printf ( "%u.%u", '''');
}
(void)printf ("\n");
Same for 82-83.
bios_info.h:
Please document what fields are in bios_info_t. Please describe
bios_supports1, bios_supports2, bios_support_ext1, at least why there
are three separate arrays and what the significance is of having three
arrays.
cpuid.s:
Please add a comment (a header at the top would be fine) describing how
these routines work.
cpuid_info.c:
Same request for a module header and function headers.
Use sys/types.h and uint32_t instead of u32.
53, 59, 65, 116: Please #define these values or else state what they
cause cpuid_info to return in a comment.
80-102 can be optimized:
char newstr[size];
char *curr = cpu_name;
int i = 0;
while (*curr != '\0') {
while (isblank(*curr)) {
curr++;
}
newstr[i++] = *(curr++);
}
strncpy(cpu_name, newstr, i);
cpu_name[i] = '\0';
Without comments, I don't have enough info to know what I'm looking at,
to be able to review the rest of this file. Sorry.
cpuid_info.h:
Headers and comments needed.
Don't use u8, u16 and u32, but use uint8_t, uint16_t and uint32_t as
defined by sys/types.h (which is already included).
55, 61, 76: It is clearer to typedef a struct and then declare a pointer
to it, rather than declare a pointer in the typedef itself. For example,
at 55,
typedef struct cpuid_data {
...
} *cpuid_data_t;
and it is used as
cpuid_data_t pdata
I suggest removing the * from the typedef:
typedef struct ...
} cpuid_data_t;
and using it in a declaration as
cpuid_data_t *pdata
This makes it more apparent that it is a pointer being passed into the
function.
The other half of the C code review, starting with dev_info.c, is
pending. This will get you started though.
Thanks,
Jack
Bill Yan wrote:
> Please review and send comments/questions by Wed. 2/24 COB. Thanks.
>
> 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
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: all_devices.c
URL:
<http://mail.opensolaris.org/pipermail/caiman-discuss/attachments/20100221/32d7c7dd/attachment-0002.c>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: ids.c
URL:
<http://mail.opensolaris.org/pipermail/caiman-discuss/attachments/20100221/32d7c7dd/attachment-0003.c>
