Hi Dermot.
Thanks for your review. Comments inline.
On 08/24/10 06:29, Dermot McCluskey wrote:
Jack,
The logic and coding all looks fine to me. These are mostly minor
issues:
auto_install.h
==========
all good
Great.
auto_install.c
=========
1848 * booted environment. This must be done before
semantic
1849 * validation, since this may add required
devices which
1850 * are needed to pass validation.
Have you re-instated this comment accidentally? Semantic
validation is no longer done in AI. And in any event, if it was
being done, it would be done from the call to ai_setup_manifest_image
which is now called before this section of code.
Yup. Fixed.
1854 * environment. It is possible that a package
missing here will
1855 * already included in the target install, so let
the
insert "be" after "already".
Thanks. Got it.
1864 /* FAILURE or PKG_NOT_FND */
This comment is a little cryptic. Could you expand it slightly?
It now says:
/* Handle failure or "package not found"
statuses. */
auto_ddu_lib.c
===========
110 char **type, char **location, char **name, char **descr, char
**inf_link,
It looks like I incorrectly renamed some "origin" variables
to "location" previously. Thanks for correcting this. But...
115 static int ai_du_process_manual_pkg(py_state_t *py_state_p,
116 PyObject *pPackageList, char *location, char *type, char *name,
117 char *noinstall);
...I don't think this is one of those? The variable you have
renamed from "origin" to "location" here really is an "origin"
value from the AI Schema.
OK. This one could go either way, since I'm using that variable to
initialize a package. I'll change it.
I think only the variables used to store the "pkg_location"
value from DDU should be called location/locn. I believe all
the others should remain as "origin". Do you agree?
Yes, that was the intent...
I looked through the file and found a couple of other places which I
changed back from location to origin as well.
196 PyErr_Print();
Could this be replaced with a call to ai_dump_python_exception()
as you have done elsewhere?
I left PyErr_Print here for this one instance on purpose. If we hit
this there is either a programming of system configuration error and it
is something that requires attention. It is unexpected and has nothing
to do with user input. Customers will never see this, only us if we
goof somewhere.
line 208:
- what is the reason for moving the Python thread initialization
from before the importing 'til after? Most other places that
import Py code do it the other way round.
The order is like this because it is easier to cleanup on error. Errors
are now handled above the thread code. If an error occurs, the thread
code doesn't even execute. If the order were reversed, we would have to
undo the thread code on errors.
I believe the thread code and the importing are orthogonal, so the order
shouldn't matter from a functional point of view. Please let me know if
this is incorrect.
1310 * <search_all addadd="false">
s/addadd/addall/
(I recognize this typo! I think you corrected it in an
early draft of my previous changes?!?)
D'oh! Thanks, fixed.
This just makes the point of how jaded one becomes looking at their code
for long periods of time, and how important it is to have a second set
of eyes! Thank you.
1319 * publisher name and origin are both optional, but both
must be specified
1320 * together.
Again - this looks familiar. I think Evan had asked me to change this
to something like:
* publisher and origin are both optional, but if one is specified
then the
* other must also be specified.
OK. Thanks. Changed back to what was there before.
I also added a quick note about what addall is for.
1908 * it be that that the package is not found or there was an issue
during
s/that that/that/
Yup, another one that slipped through the cracks.
Updated webrev is in same place:
http://cr.opensolaris.org/~schwartz/100820.1/webrev/
Delta webrev (from yesterday's) is at:
http://cr.opensolaris.org/~schwartz/100820.1/webrev.2.3.diff/
Thanks so much for reviewing.
Jack
- Dermot
On 08/23/10 19:53, 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
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
