Hi John.
Thanks for your review.
Jack,
You've been real busy. Lots of good work.
Thanks!
Comments below.
John
----------------------------------------------------------------------
usr/src/cmd/auto-install/auto_ddu_lib.c
----------------------------------------------------------------------
Increase the indentation for the value so that DDU_ERROR_MODULE and
DDU_PACKAGE_NOT_FOUND_EXC line up:
48 /* DDU error module related. */
49 #define DDU_ERROR_MODULE "DDU.ddu_errors"
50 #define DDU_PACKAGE_NOT_FOUND_EXC "PackageNoFound"
Please check for NULL returns:
407 /* Check for duplicates. */
408 vids = (char **)malloc(sizeof (char *) * orig_listlen);
409 dids = (char **)malloc(sizeof (char *) * orig_listlen);
410 classes = (char **)malloc(sizeof (char *) * orig_listlen);
Malloc's checked for the whole file. There were a few other spots
fixed too.
Can ttype, tlocn or tinf_link ever be NULL?
536 (void)
ai_get_ddu_package_object_values(*pPackageObj_p,
537 &ttype, &tlocn, NULL, NULL, &tinf_link, NULL);
538 if ((tlocn[0] == '\0') && (tinf_link[0]
== '\0') &&
539 (strcmp(ttype, "UNK") == 0)) {
No, but on error they can return uninitialized. Fixed this, to
contain empty string if ai_get_ddu_package_object_values() returns on
error.
In ai_du_get_manual_pkg_list() before the returns of FAILURE does
pPackageList_p need to be freed seeing as how it is created at:
1200 *pPackageList_p = PyList_New(0);
No. We always return a list, even on errors. Can be empty. We're
OK here. This is documented in the header.
Furthermore, should pPackageList_p be set to NULL upon error? Or is
this a
function of Python where it returns an empty list ([])? There are
several
of these situations throughout the code. So this is probably my
education
lacking in Python.
This is documented in the code. Some functions return an empty list
even on errors. This is because an error may be logged for one
package but not for another, and we want to return as many packages
as possible, even if there are some errors. In the ultimate failure,
no packages (i.e. an empty list) is returned on error.
Should not origins be freed upon error? It is allocated at:
1230 origins = ai_get_manifest_values(path_p->path_str,
&origin_len);
Yes, thanks. Fixed.
But not freed upon error before the return at:
1236 if (origin_len != num_pkgspecs) {
1237 auto_debug_print(AUTO_DBGLVL_ERR,
1238 "ai_du_get_pkg_list: <add_drivers> manifest
error:\n"
1239 "There is not a 1-1 <origin> - <software>
mapping.\n");
1240 return (AUTO_INSTALL_FAILURE);
1241 }
At line 1383 there is a log statement saying that a 'scan for
devices which
are missing drivers...' is being done. Should another log statement
be added
before this return?
1398 } else if (PyList_Size(pDeviceList) == 0) {
1399 return (AUTO_INSTALL_SUCCESS);
1400 }
Something to the effect that there are 'no missing drivers'.
OK Done.
You can save a little bit of code and memory by using pub_buf and
cmd_buf as
the same character array (i.e., buf[MAXPATHLEN]) but then you loose
readability.
1493 FILE *pub_info;
1494 char pub_buf[MAXPATHLEN];
1495 char cmd_buf[MAXPATHLEN];
Other areas in slim_source have different buffers for command and
data, so I'd like to leave it that way.
Then there's the efficiency thing... Why waste MAXPATHLEN bytes for
a small command? The tradeoff would be additional code to malloc the
right amount of memory, free it, etc. This IMO convolutes the code
and is worse than having a few extra bytes allocated until the
command finishes.
It would seem that the following should be moved below the lookup_err
checks:
1570 /* Get info for display / logging purposes, and
log it. */
1571 if (ai_get_ddu_dev_data_values(pDDUDevData,
&dev_type, &descr,
1572 NULL, NULL, NULL) != AUTO_INSTALL_SUCCESS) {
1573 auto_debug_print(AUTO_DBGLVL_ERR,
1574 "ai_du_get_searched_pkg_list: Error
retrieving "
1575 "device information for display\n");
1576 dev_type = descr = empty_string;
1577 }
Perhaps before:
1604 (void)
ai_get_ddu_package_object_values(pDDUPackageObject,
1605 NULL, NULL, NULL, NULL, NULL, &third_party);
But this initializes dev_type and descr, which is used in messages
regardless of which of the 3 results of lookup_err ensues. I think
this code is correctly placed.
Should there be a check for non-NULL before the following assignment?
1700 type = location = name = descr =
inf_link =
1701 empty_string;
No. But these variables are being initialized a second time here,
and so I have removed the initialization in the declaration section
~10 lines above this line.
The indentation needs to be increased within the switch statement:
2049 switch (ai_du_get_searched_pkg_list(py_state_p, &path,
install_root,
2050 &searched_pkg_list)) {
2051 case AUTO_INSTALL_FAILURE:
2052 rval = AUTO_INSTALL_FAILURE;
2053 auto_debug_print(AUTO_DBGLVL_ERR,
2054 "ai_du_get_and_install: "
2055 "Error searching for inoperable devices and "
2056 "missing driver packages.\n");
2057 /* Keep going. Don't abort. */
2058 break;
2059 case AUTO_INSTALL_PKG_NOT_FND:
2060 if (rval != AUTO_INSTALL_FAILURE) {
2061 rval = AUTO_INSTALL_PKG_NOT_FND;
2062 }
2063 break;
2064 default:
2065 break;
Each case should be indented further.
Not according to the cstyle document. See page 16. (Note: I have a
copy in /home/schwartz/documentation/cstyle-coding-1.8.pdf if you
want to browse.)
----------------------------------------------------------------------
usr/src/cmd/auto-install/auto_install.c
----------------------------------------------------------------------
This malloc should be checked for NULL before being used:
396 package_list =
397 malloc((num_packages + 1) * sizeof
(char *));
Not my code, but since I'm making lint changes, etc, I'll change here
and a malloc near ~1059.
----------------------------------------------------------------------
usr/src/cmd/auto-install/auto_install.h
----------------------------------------------------------------------
It would be good to increase the indentation for the values
for the following defines:
47 #define AUTO_INSTALL_SUCCESS 0
48 #define AUTO_INSTALL_EMPTY_LIST 1 /* list of packages is
empty */
49 #define AUTO_INSTALL_PKG_NOT_FND 2
50 #define AUTO_INSTALL_FAILURE -1
51 #define AUTO_TD_SUCCESS 0
52 #define AUTO_TD_FAILURE -1
OK
----------------------------------------------------------------------
usr/src/cmd/auto-install/default.xml
----------------------------------------------------------------------
Looks fine.
Yay!
Thanks again for reviewing.
Webrev updated:
http://cr.opensolaris.org/~schwartz/100820.1/webrev/index.html
Delta webrev:
http://cr.opensolaris.org/~schwartz/100820.1/webrev.1.2.diff/index.html
Jack
Thanks,
Jack
On 08/20/10 07:24 PM, Jack Schwartz wrote:
Hi everyone.
Please review a webrev for the following bugs:
6978976 AI manifest <searchall> should be more forgiving of missing
packages.
6978978 DDU install should log what pkg repo they are trying to get
packages from
6978979 AI Driver Update code needs to check for duplicate driver
requests
6971871 Handling of expected DDU Python exceptions shouldn't
display tracebacks in AI logfiles
6971872 Preface Driver Update messages with more useful
<add_drivers> instead of function names
http://strongheart.sfbay/public_webrevs/100820.1/webrev/
Please review if possible by Weds COB 8/25. I would like to get
this back to slim_source before I leave on vacation, to push on
Friday 8/27 AM. Unfortunately, I had to wait until now to send
this out, for the manifest schema changes to go back first. I've
been testing for the last few days however.
Changes:
Changes to auto_install.h are mostly reformatting to pull in line
lengths to < 80 chars. There are three other changes to that file,
all trivial: (line 49, 372, 374)
Changes to auto_install.c are for better messaging and handling for
6978976
Changes to default.xml are to uncomment the <add_drivers> clause
again.
Changes to auto_ddu_lib.c can be roughly broken down as follows:
- 163 - 238: revamp of auto_ddu_lib_init/fini since they handle
more modules now.
- 245 - 280: the guts of the fix for 6978971 regarding Python
exceptions.
- 401 - 449: the fix for 6978979
- 506 - 545: origin of the new return code for when searched
packages are missing
- 815 - 874: beef up the routine that returns info for checking for
duplicate devs
- 1487 - 1512: list repo packages are taken from (6978978)
Throughout: propagation of error to distinguish the package not
found error from the others; fixing of error messages for 6978972,
lint and other cleanup.
Thanks,
Jack
