Hi Keith, Thanks for the comments! Much appreciated. I'll comment on the DC specific issues you raise and let Joe comment on the finalizer script specific issues.
* Keith Mitchell (Keith.Mitchell at Sun.COM) wrote: > Hi Glenn & Joe, > > There's been a lot of comments already - I tried to make sure my > comments weren't addressed, but if I missed something, please point > me to your other response. Thanks. > > DC-manifest.rng: > - With the changes for VMC, it seems <img_params> is an optional > subset. As you mentioned in another response, it's not *truly* > optional. It seems like originally you were going for something like > the following (based on the commented sections and your previous > responses) > > <start> > <element name="distribution"> > <attribute name="name"/> > <ref name="nm_distro_constr_params"/> > <choice> > <ref name="nm_img_params" /> > <ref name="nm_vmc_params" /> > </choice> > </element> > </start> > > I'm curious why you reversed that reasoning; it seems like such a > restriction would allow us to enforce the need for img_params to > exist when building a live image, but also not cause the parser to > fail on VMC manifests. While there aren't any VMC specific > parameters right now, there could be in the future, and I don't > think there's anything wrong with an empty <vmc_params/> element in > VMC manifests for now. When I initially reversed my reasoning, it was because during implementation I didn't actually end up creating anything in nm_vmc_params. And I didn't think delivering an empty nm_vmc_params (nor including an empty sub-section in the manifest) was necessarily a good idea. Though I don't think my current implementation (which makes nm_img_params truly optional even when it isn't) is a good idea either. So, I'm leaning towards including an empty nm_vmc_params and then manifests will require either nm_img_params or nm_vmc_params in order to validate. Seems somewhat ugly to me (though being able to validate manifests completely again is a win), so I'm open to better ideas if anyone has any. > DC-manifest.defval.xml: > The need to specify "skip_if_no_exist" on almost every field > suggests to me that there 'something' needs to change - likely in > our defval processing code, to increase flexibility. Off the top of > my head, it seems the defval manifests are distinctly NON-tree like, > which is what is causing the flexibility: "missing_parent" actions > either create the whole tree or not, regardless of how much of the > tree is already there. If we had a manifest, that, say, could look > more like this: > > <default node="img_params" missing="skip"> > <default node="pkg_repo_default_authority" missing="skip"> > <default node="main" missing="create"> > <default node="url" missing="create" /> > <default node="authname" missing="create" /> > </default> > </default> > ... > > Then, we could evaluate each node, but only if we have a place to > put it already (because the parents are there). Then, we wouldn't > really need skip_if_no_exist littered throughout. This is just a > quick example, not necessarily the 'best' solution. If it's not in > scope for VMC release, then can you file an RFE to make an > enhancement along these lines? I agree with everything you said. I'm not certain however that such a change is in scope for VMC. Let me ponder this a bit more (and have some side conversations) and see what I come up with. At the very least, I'll file the requisite RFE(s). > im_pop.py: > General note: As I understand, this code was simply moved and isn't > new. So if any of my comments are truly out of scope changes, can > you file bugs? > > General: > try: > ... > except: > ... > will catch more exceptions than intended, including > KeyboardInterrupt (a.k.a. control-C) and SystemExit. In general, per > PEP 8, except clauses should be as specific as possible when > catching exceptions. If a generic exceptions is *truly* needed, it > should be "except Exception:" (which will catch everything BUT > KeyboardInterrupt and SystemExit). I'm going to consider this out of scope. Though I will file a bug. > 34-37: Just want to reinforce the desire that these be made into > import statements. Accepted. Will do. > 83-104: Python/C code nit: raise Exceptions (or subclasses thereof), > don't return status codes. Particularly given that below, the status > code is checked for non-zero and an exception is raised, the change > here should be simple. In other cases, I don't know how disruptive a > change this will be, depending on how many consumers of the function > there are and how equipped they are to handle a change from C-style > status codes to Python style try/except clauses. > --Same comment for other functions. Some are just strictly calling > tm_perform_transfer, so truly, the easiest way to affect such a > change would be to change *that* which I think is out of scope here. I'll consult with Jean on this. My initial thought is to leave it as is and file a bug. > 227: There's no "if __name__ == '__main__':" clause to lead off the > "Main" section. I'll add. > 352: I think this should be "IOError"? Yep. Will change. > ---- Shell scripts ---- I'll let Joe respond to these issues. Modulo a specific one I saw in create_vm. > create_vm: > 274, 284: Nitpick, but why not capture the output from the first > call to "VBoxManage -v"? > > 295-300: We blindly destroy an existing VM? Will first time users > blow away their VMs when they give their VMC build name the same > name as a current VM? Should we be using the --basefolder flag to > create the VM in the DC build area instead of $HOME/.VirtualBox? > (Especially since we're running as root to fire off DC...). If we > did this, we might be able to handle my previous comment by > unregistering an existing VM if it's not located in the DC build > area (and of course, re-registering it when finished). > > 341: Only 32 bit Solaris? Wow. Good catch. Yes, currently as implemented you could only create 32-bit OpenSolaris vm's. We'll need to change this. We'll add another argument to the script (along the lines of ram size and disk size) to allow the user to specify what vm type they want (32-bit or 64-bit). > 341-343: In the case of the hardcoded parameters, can we set > variables at the top of the script, and reference them here? This > would make it easier for users to modify if they truly wanted (and > easier for us to add flexibility in the future) > > export_esx: > 218-232: This is duplicate code from elsewhere. Any chance we can > isolate this somehow? > > 285: Am I confused, or is this backwards? You create the directory, > then immediately rm -rf it? > > export_ovf: > 218-232: Same comment as export_esx > 285: Same confusion! > > General note: Someone else mentioned that instead of VBoxManage, you > should set a variable to the precise path and use that. I'd > recommend setting that variable to have the '-q' flag embedded, > since it's used in each case. Thanks Keith! -- Glenn
