Hi Sundar, Thanks for the comments! I know Joe responded to the comments about the finalizer scripts in particular. I'll address the DC specific ones.
* sundar Yamunachari (sundar.yamunachari at sun.com) wrote: > Glenn Lagasse wrote: > >The VMC project is proud to present it's code for your review. You may > >access the webrev at: > > > >http://cr.opensolaris.org/~glagasse/slim_vmc/ > > > >I'm also attaching the DC log file for a VMC run as well as the console > >output for the same run for the curious as to what this looks like in > >practice. > > > >The timeout for providing review comments is COB October 23rd, 2009. If > >you need more time, please let me know. > > > >Thanks! > > > >------------------------------------------------------------------------ > > > >_______________________________________________ > >caiman-discuss mailing list > >caiman-discuss at opensolaris.org > >http://mail.opensolaris.org/mailman/listinfo/caiman-discuss > Glenn, > > usr/src/cmd/distro_const/DC-manifest.rng: > > Is there a reason why vmc fields are commented in the schema files > (lines 65-67, 299-305). Are these definitions are not used or not > needed? I answered this for Jean. I'll include the response I sent her here. Somehow I missed this when I was reviewing the webrev prior to publish. There actually aren't any vmimg_params for VMC. Everything is set as arguments to the VMC finalizer scripts. So the block 52,73 should look like: <attribute name="name"/> <!-- General distro-constructor parameters --> <ref name="nm_distro_constr_params"/> ! <optional> ! <!-- Parameters for building live image --> <ref name="nm_img_params"/> + </optional> The effect of which makes nm_img_params optional for all manifests since it's not needed for VMC manifests (and there was no good way I could come up with to make nm_img_params mandatory for the AI/SLIM manifests but not for VMC manifests). That said, Keith raises an interesting thought in that in order to allow manifest validation of a sort to actually occur we might want to include an empty vm_img_params. I need to think about this some more. At the moment, what I said to Jean is what I'm planning to do. > DC_defs.py: > > You got me confused here. Changing DISTRO_PARAMS -> IMG_PARAMS and > IMG_PARAMS->DISTRO_PARAMS ;-) Sorry about that :-) Since I had to move elements around in those sections, that also necessitated changing some things here. > usr/src/cmd/distro_const/utils/im_pop.py: > > General comments: The pkg set-authority is changed to set-publisher. > Do we need to convert authority to published for this project? No. That will get cleaned up at a later date. > 80: Why you have a 'pass' here? This code came from distro_const.py initially. Apparently it wasn't considered a real problem if the .image_info file wasn't created during image construction. If we're going to keep that behaviour, it should be return instead of pass. I think however that we probably should stop the build if the .image_info file can't be created. I'll talk to Karen about it. > 107: Can you add some comments to the function 'dc_ips_unset_auth' I can. Apparently it was an oversight when this was originally written. > 180, 188, 197, 208, 220: Comments the functions in those lines More existing code that didn't have initial comments. I didn't write any of this, but I can take a stab at it. > 315: publisher is used here and authority is used in all other places Good catch. I'll go ahead and change the publicly displayed authority strings to publish (since that's what it's called now). I'm not going to fully re-work everything at this time however. Thanks Sundar! -- Glenn
