Hi Karen.
Thanks for reviewing. My comments are inline.
Karen Tung wrote:
> Hi Jack,
>
> I am not familiar with C to python interfaces, so, I just
> quickly browse though auto_ddu_lib.c and auto_parse_manifest.c. I
> don't see
> anything obviously wrong in those.
>
> I reviewed all the other files in your webrev in detail. My comments
> to those
> files are below. If I don't list a file, that means I have no comment
> about it.
>
> ai_manifest.rng
> * line 171: why are these things "zeroOrMore"? Since "add_drivers"
> is already optional, I think that if they do specify the tag, then,
> they should
> at least specify 1 or more thing.
I have improved on the contents even more than you suggest: Before
there was zeroOrMore of a choice between bundles and searchalls.
However, the spec calls for:
either:
one or more bundles
or
zero or more bundles and one searchall
I have updated the schema accordingly. What I had in the schema before
would have worked alright, as more stringent checking is done in the
driver update code before the schema is even consulted. However, for
documentation and correctness purposes, the schema is updated.
> ai_parse_manifest.py:
> * The ai_verify_manifest_filename() function is just used in
> ai_create_manifestserv() function,
> and it really only does one thing that can be done inside the
> ai_create_manifestserv()
> function itself, why have a separate function for it?
I have removed that function. Please see my comments to Keith.
>
> * lines 103-105: the logic to create this temp manifest filename can be
> part of the schedule_validate() function. Since it is just a temp
> filename,
> I don't really see a point in having to pass it in, and having to
> duplicate
> the logic here as well as ManifestServ.py, lines 388-389. If you want
> people
> to have the flexibility to pass in a name, you can make that an
> optional argument.
I have changed the argument code in ManifestServ utility module. Now,
__init__ will generate defaults for all names. schema_validate,
semantic_validate and set_defaults will use these defaults if args
aren't passed into those methods.
> * line 110: since the last 3 arguments are optional, and you are just
> specifying the default values here, why not just not specify them?
Good point. Fixed.
> * The ai_start_manifest_server() function is another 1 liner that can
> be done inline in the
> ai_setup_manifestserv() function, why have it in a separate function?
OK.
>
> ManifestServ.py:
> * Since the manifest_name is passed in in the __init__() function of
> the ManifestServ object,
> why not just keep a copy of manifest_name in the object. That way, you
> don't need to maintain
> a copy of it in auto_parse.c. Additionally,
> you can just use the stored name to compute the schema used for
> validation, and the defval manifest, and you
> don't need to compute "valfile_base" separately outside, and pass it in.
OK... I have changed the argument code in ManifestServ.py. Now,
__init__ takes all of the args it used to take (plus one more:
full_init). This means that DC doesn't need to change, the ManifestServ
utility module doesn't need to change, and auto_parse.c doesn't need to
pass in the same argument twice.
> * line 423: Do you mean to put "raise err" here?
"raise" with no arguments will raise the previous exception.
>
>
> auto_parse.c:
> *line 41: why not initialize manifest_filename to NULL here?
static variables are automatically initialized to NULL or 0.
> * ai_setup_manifest_image() function: valfile_base is allocated but
> never freed in
> all the success or failure return points in this function
No longer applicable due to other changes.
> * line 160: should manifest_filename be checked to make sure it is not
> null before
> you do "strdup" here. What if ai_create_anifest_image failed for some
> reason and
> the value didn't get set?
In the general case I would agree. However, since this function is
specialized to AI and the caller of ai_create_manifest_image checks for
errors before calling ai_setup_manifest_image, I don't think it is
necessary. There is already a comment that manifest_filename is
guaranteed not to be NULL.
>
> ai_sparc_image.xml:
> ai_x86_image.xml:
> * In prior discussions, you mentioned that in order for things to work
> TODAY, before
> bug 13109 is fixed, you need to add a 50mb padding to the root
> archive. I don't see
> this padding added to the manifests. Is it the plan to not integrate
> these changes
> until bug 13109 if fixed?
It is more important that a 512Mb system can install at all, than
whether IPS will work for low memory systems. SVR4 packages will still
work with 512Mb, and right now that is most important. There are not
many small memory systems which require some IPS-packaged driver which
exists in the repo but is not part of the AI image, so I see this as a
corner case. Later on, as SVR4 fades away and IPS catches on, this case
will become more important.
Thanks again for reviewing.
I'll retest and post a new webrev with the changes soon...
Thanks,
Jack
>
> Thanks,
>
> --Karen
>
> 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
>
