Hi Bill.
After making changes per Rich and my findings during testing, I
re-reviewed most of this code as well.
I will re-review load_driver.sh and other files undergoing changes per
our bug-findings once you are done making changes to them. At that time
I will also do a more compete review of probe.sh (because I ran out of
time today).
The form of the code is pretty good. Most of the code is commented well
enough to be understood. I have called out what is not.
Here are my comments:
3rd_drv_ins.sh:
---------------
Looks pretty good.
An improvement (not for now, but for the future) would be to change the
form of
the file. Right now load_driver.sh exists in both the if and else
clauses. I
suggest changing to something like this:
if (compressed) {
uncompress the file
}
load_driver.sh
- - -
NIC_keywords: not reviewed
-------------
comp_lookup.sh:
---------------
79: Please add an error message whenever a script exists on an error.
84, 85: Please add a short comment about what you are doing here. I
only know
what you are doing because I have insight :).
106: $repo_candi is referenced here, but $repo_candi is ""
(as this is the else clause of "if -z $repo_candi")
132: Please add a comment that you are looking for a match on the driver
in a
package, since you couldn't find a match in a package for a compatible name.
159: what are hardwired values 2 and 3? Actually, not sure what this else
clause is doing... A comment would be helpful.
166 - 175: All of these matched_drv_pkg_type=UNK can be eliminated if
you set
matched_drv_pkg_type=UNK at the beginning. Then you can change
matched_drv_pkg_type again when you determine what it is.
ddu_magic:
----------
Not reviewed except to note that /etc/magic has this entry:
0 string PK\003\004 ZIP archive
so I'm wondering if ddu_magic is necessary. I'm not familiar with this,
but if
it would be better to use /etc/magic if possible, as that would be one less
file for you to maintain in the future.
det_info.sh:
------------
50: For next time:
Put settings which are common to many files, such as err_log, in a separate
file. Then include that file in the other files. In shell you do this like
this:
. <file>
All of the statements in <file> execute and have the same effects as if they
were part of the file which called it.
62-63: What do these hard coded values represent?
file_check.sh:
--------------
54: State the filename in this error message.
Else the user won't know which file the message originated in.
--> Same for 3rd_drv_ins.sh
--> Same for load_driver.sh
--> Same for pkg_relate.sh
64, 69, 80 : STate filename in this error message.
146: What does IFS and old_IFS do? Can they be removed? When I search the
scripts directory I see it getting set, but don't see it getting used:
strongheart:/home/schwartz/gates/ddu_131_child/src/scripts> grep IFS *
det_info.sh: IFS='
file_check.sh: IFS='
find_media.sh: IFS='
find_media.sh: IFS='
probe.sh:IFS='
probe.sh: old_IFS=$IFS
probe.sh: IFS=" "
probe.sh: IFS=$old_IFS
172, 178: Add a comment that load_driver.sh will show the bad driver.
find_media.sh
-------------
61: removal -> removable
61: the comment says probe for mounted removable media, but on line 110 it
looks like the script is mounting something. Is the comment accurate?
Looks like a comment is needed around line 106 as well.
load_driver.sh:
---------------
I'll defer review of this until we're all done with pending changes.
noask: Not reviewed
------
71: IFS?
78: Add a comment about what kinds of errors are expected here.
p5i_install.py:
---------------
I cannot really review this. There are NO comments in this file and
I've not
used JSON before. Sorry.
I suggest you add comments to explain what this file is doing...
pkg_check.sh
------------
72: Just curious: how come egrep is used here, whereas grep is used for
lines
93 and 115?
93: Since ${magic_file} is ddu_magic which has one entry for PKZIP, how
can it
recognize other compressed file types? Please explain.
139: typo ouput -> output
72: How come a special magic_file is needed? Please add a comment to
explain.
200: Where is ${base_dir}/magic, dc_dir, and dc_dir_1 cleaned up?
Is this done in another file?
probe.sh:
---------
It would be nice if you could use pci.h for this... Oh well...
59: trough -> through
255-256: I saw these defs in another file. Please see the note I made about
having common definitions in a single file. (det_info.sh, line 50)
284: I'm not familiar with the :>file syntax. What does this do?
general: if someone ^C's in the middle of this file, will the temporary
files et
cleaned up?
Thanks,
Jack
Bill Yan wrote:
> Hi Joseph,
>
> I updated the code again based on the comments from your previous
> emails. The URL is at:
>
> http://cr.opensolaris.org/~billyan/DDU_1.3.1/webrev_20100323/
>
> I really appreciate that if you could help take a look. Thanks a lot.
>
> Regards,
> Bill
<snip>
