Hi Keith.
Keith Mitchell wrote:
> Hi Jack,
>
> Responses inline.
>
> On 02/26/10 03:42 PM, Jack Schwartz wrote:
>>
>> Hi Keith.
>>
>> Thanks for your review.
>>
>> On 02/12/10 16:09, Keith Mitchell wrote:
>>> 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.
>> Deleted.
>>>
>>> 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__?
>> Thanks for pointing this out. Believe it or not, the manifest is
>> checked in three places:
>> - the combined manifest is checked in auto_split_manifest
>> - the AI manifest is checked here and then when the file is opened
>> for actual use.
>>
>> Removed ai_verify_manifest_filename().
>>
>>> 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).
>> There are a couple of reasons why this is not a good idea IMO:
>> - The ManifestServ object is the common mechanism for all of our
>> (current) tools to read manifests. This includes AI and DC.
>> - It would be better to re-implement AI in python and get rid of the
>> C code rather than reimplement ManifestServ in C. Code would be much
>> smaller, simpler and more maintainable.
>>
>> As a blatant example, consider that auto_ddu_lib.c was a
>> reimplementation of the same functionality in python, and it took
>> ~1500 lines of C to do the work of 168 lines of Python. Even
>> accounting for a non-1-for-1 comparison, that's still a huge difference.
>
> Oops, let me clarify. I didn't mean implementing ManifestServ in C,
> but rather, calling the appropriate C-api methods to access Python
> code from C.
>
> The function ai_create_manifestserv, in auto_parse_manifest.c, makes a
> call into ai_parse_manifest.py:ai_create_manifestserv, which only
> instantiates a ManifestServ.
>
> Instead, ai_create_manifestserv (the C version) could call the C
> equivalent of the Python expression "myobj = ManifestServ(<args>)",
> which is:
> PyInstance_New(*class, *args, *kwargs)
> http://docs.python.org/c-api/class.html#PyInstance_New
>
> Similarly, functions such as
> ai_parse_manifest.py:ai_start_manifest_server aren't really needed;
> the amount of C code to call a Python module function vs. the amount
> of C code to call a Python class method is the same.
While your point is well taken, I don't want to veer too far away from
the original scope of my project. My intent isn't to optimize AI so
much as to get Driver Update to work with it. I have filed bug
14926 - auto-installer C-Python interface code can be optimized
to address this in the future.
>
>>>
>>> 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).
>> The functions in this file call into Python, not the other way
>> around. They call PyErr_Occurred to check the Python interpreter for
>> errors after the Python code returns.
>>
>> The only entrances into this module are ai_du_get_and_install() and
>> ai_du_install(). They are called only from C and return only status.
>
> Looks like I had it backwards - thanks for the clarification!
>
>>>
>>> 222: This line seems redundant - if you want to return NULL when
>>> pRet is NULL, why not just return pRet?
>> Removed :)
>> Likewise for 281, 337, 460
>>>
>>> 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?
>> It is consumed by schema_validate(), set_defaults() and
>> semantic_validate().
>>> If not, should it be an attribute of the ManifestServ object,
>>> instead of something that must be passed around through multiple
>>> functions?
>> The reason it is passed around is so the user of ManifestServ can
>> choose the stages at which to save the files.
I have changed the code now so that a user of ManifestServ can choose
different files if desired, but can also take defaults set up by
__init__. This, I believe, addresses your original concerns while
maintaining flexibility.
>
> Thanks for clarifying.
>
>>>
>>> Same question for 'verbose' (in terms of having it be an attribute
>>> of the ManifestServ).
>> Same answer.
Changed code addresses this issue as well.
>>>
>>> In some cases, there are references to ManifestServ.XML_SUFFIX; in
>>> others, ".xml" - can these be standardized?
>> Thanks; fixed.
>>>
>>> 190: Nit: "== None" -> "is None"
>> Fixed.
>>>
>>> 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).
>> Yes, I like this idea. That would remove the need for changing DC
>> and install-tools/ManifestServ.py To achieve this, I'll make the
>> default to do the full setup as it was doing before.
>>
>> Look for an updated review after I go through the rest of the comments.
>>
>> Thanks again for reviewing.
>
> You're welcome.
I'll post a new webrev after I retest. I've made lots of changes...
Thanks,
Jack
>
> - Keith
>
>>
>> Jack
>>>
>>>
>>>
>>> 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
>>
