* Karen Tung (Karen.Tung at Sun.COM) wrote: > >>- All the 4 existing manifests - The list of finalizer scripts used > >>to be in the position of the file below the warning about not > >>modifying certain fields in the manifest. They are now moved > >>to a spot before that warning. That might confuse people. > >>I think it is better to move them back to be below the warning. > > > >Well, the finalizer section is now part of the output_image tag which is > >part of the distro_constr_params tag. Previously, it was part of the > >img_params tag. > > > ah, I see.. In that case, I think it would be better to have the > finalizer scripts > section be the last thing in the distro_constr_params section. Then, put a > warning there about people shouldn't need to modify it.
Sure, I can do this. > >>Specific Comments: > >>-------------------- > >> > >>usr/src/cmd/distro_const/DC-manifest.rng > >> > >>- line 57-67: why is the nm_img_params optional? Shouldn't it be either > >>nm_img_params or nm_vming_params? > >> > >>- Would be more clear to name the 2 choices as "im_live_img_params" and > >>"nm_vm_img_params"? > > > >This actually was an oversight. See my response to Jean and Sundar. > >Though I'm currently wondering if it might not be a better idea (than > >what I outlined to them) to deliver an empty nm_vm_img_params so that we > >can get some manifest validation (ie a manifest must have iehter > >nm_img_params or nm_vmimg_params). > I think it is better to deliver an empty nm_vm_img_params for the > time being. I think it is better that we enforce either live image > parameters or VMC parameters are specified. Agreed. Based on other feedback that echoed this, that's what I'll do. > >>- lines 294-305: why is this commented out? > > > >This is related to my comment for lines 57-67. I had planned to remove > >it completely. > > > >>usr/src/cmd/distro_const/DC-manifest.defval.xml > >> > >>- Did you verified that the appropriate default value > >>is assigned after you added all the skip_if_no_exist things? > >>For example, if I have a manifest that have the img_params section, > >>but I didn't specify some of the values in the manifest, > >>did the "right" value get assigned? > > > >Actually I did not. I'll double check that. Do you suspect that it > >doesn't work? > I am not very familiar with that piece of code, Jack would know for > sure. However, like others who have noticed that we added > a lot of skip_if_no_exist to this file, I wonder how it will work. > Additionally, if you enforce either img_params or vmc_params must > be specified, would we be able to not have all these skip_if_no_exist > things then? Good question, I'll re-test after I re-include the vm_img_params. > >>usr/src/cmd/distro_const/utils/im_pop.py > >> > >>- This just moves the "original" im_pop step into a finalizer script. > >>The bug (6366) requests for breaking up the im_pop step. Does this mean > >>your changes will not address the bug? > > > >That is correct. It was going to add significant time to the VMC > >schedule to break this up as outlined in 6366 which I deemed out of > >scope for VMC since that's not what VMC needed. > > > OK. I will leave 6366 open for that work then. I was going to assign > you to the bug. :-) Feel free to assign me to it. I won't get to it for a bit, but I doubt anyone else will either and I'd be happy to tackle it when VMC is putback. Thanks Karen. Glenn
