Hi Jack, I looked at the .py files in reasonable detail, but looked more quickly at the .c files, so please have another reviewer hit those in more detail if someone else hasn't already.
Thanks, Keith auto_parse_manifest.c: 119: This line of the comment seems unnecessary, given that you'd need to make the function call to get the parameter to pass in. ai_parse_manifest.py: 55-81: Perhaps this is unrelated to this particular webrev, but is there a reason the manifest filename can't be verified in ManifestServ.__init__? Also, this function's sole purpose seems to be providing an alternate path to instantiation of a ManifestServ object, for calling in C - can't we just instantiate it from C using appropriate C-Python API calls? (Many of the functions in this file seem that way - wrappers around simple calls of Python class methods. Perhaps a bug to refactor would be in order). auto_ddu_lib.c: General: Code that returns PyObject* needs to make sure to call one of the PyErr_* functions to flag to the interpreter that an error occurred, particularly if the function ever gets called in Python (NULL should never get back to Python-land). If a NULL return is an "expected" error, returning PyNone would be better. (I'm not the best at reading C-Python code yet, so if I misunderstood the use cases for the functions in this file, and they're not for Python consumers, disregard this - same goes for any of my comments for the other *.c files). 222: This line seems redundant - if you want to return NULL when pRet is NULL, why not just return pRet? install_utils/ManifestServ.py: General: Many functions seem to have a keep_temp_files parameter. I don't see a consumer of this parameter; is it obsolete? If not, should it be an attribute of the ManifestServ object, instead of something that must be passed around through multiple functions? Same question for 'verbose' (in terms of having it be an attribute of the ManifestServ). In some cases, there are references to ManifestServ.XML_SUFFIX; in others, ".xml" - can these be standardized? 190: Nit: "== None" -> "is None" 298: Instead of providing two separate methods for creating a ManifestServ object (__init__ and full_setup()), can you add an optional parameter to the __init__ function, and perform the additional setup if that parameter is set to True (defaulting to whichever is the more common scenario)? The function full_setup can remain, and could just be called if the parameter indicates it should be (though it would no longer need to be a staticmethod). On 02/ 9/10 06:24 PM, Jack Schwartz wrote: > Hi everyone. > > Here is the code review for AI changes to incorporate 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. > Packages may be explicitly specified in the manifest. They can also > be found via search, also initiated via the manifest. > > http://cr.opensolaris.org/~schwartz/100209.1/webrev/index.html > > This webrev is only of AI changes needed to call into the DDU > library. The DDU team will be posting a separate webrev with their > library changes. > > Testing is ongoing. Testing done: > - Validation of correct and incorrect manifest entries. > - Proper calling of DDU library, for explicit package entries > - Proper calling of DDU library to complete a search and resulting > installation. > - Verification that packages are installed when library is > correctly called, or else sending feedback to the DDU team about > found library issues. > - Verified that ManifestServ and DistroConstructor > > Please review and send comments/questions by Tues 2/23 COB. > > Thanks to the DDU team for helping me on their end, to get this done. > > Thanks, > Jack > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
