Dave Miner wrote: > Moinak Ghosh wrote: >> Hi, >> >> [...] >> >> I have re-written the module in Python. The original C interfaces >> to the Transfer module remain unchanged. However the these >> interfaces are now wrappers that load up an embedded Python >> interpreter. An argument list is built up based on the nvlist >> entries passed in and the python module is invoked. The C wrapper >> extends the embedded interpreter by providing a couple of callbacks >> that the python code can invoke. The memory overhead of loading an >> embedded python interpreter is approximately 3MB. There is no >> performance degradation as Transfer module spends most of it's >> time in cpio. >> >> The webrev is at: >> http://cr.opensolaris.org/~moinakg/tm_python/ >> >> This code replaces the functionality provided by the existing >> Transfer module in C. I have successfully installed a DP2 image >> using this module and got a working installed setup. >> >
Thanks for the review comments. I will be posting a much updated version shortly. > transfer_mod.py > > 55 (and others): seems stylistically wrong for the indent level of the > continuation line to increment to the same as the contained block; I > would have expected a four-space indent similar to what we do in other > languages. Is there a reason you did it this way? No reason other than I was in a hurry. I have changed these. > > 63: s/spio/cpio/ > > 113 (and others): now you went very deep with the continuation indent; > let's be consistent somehow. > > 112-122: it seems like we will want to make the slim post-processing > in the DC either provide these patterns or the complete file lists. > Has that been discussed with those on the slim team who are supposed > to do the post-processing? Yes. As per Jean these features are not scoped in DC for the May release so I am keeping these as-is for now. > > 129: should be under the if Done. > > 176: please file that rfe in solaris/kernel/keyboard > > 333: a comment on this block seems advisable Done. > > 355: why flush these at different levels? The file list is flushed inside the loop for ease of debugging. There is no need to flush the zerolength file list inside the list. I have added a couple of comments. > > 400,445: i'm a little concerned that we completely dismiss any cpio > errors, but seem to expect to see them. what are the errors we're > typically seeing, and can we eliminate them? Users who look at logs > are liable to generate extra support traffic that we don't need. Okay. Cpio errors are generally warnings about trying to copy symlinks twice or already existing files etc. I changed the code to only log these warnings if the debug flag is set. > > 425: can this actually happen? seems like we'll end up with mis-owned > directories if we do, so maybe this should be an error? This should not happen. I took out these lines. > > 455: the skeleton.cpio isn't on the slim CD, so this can be removed Removed. > > 470-: There's a very fuzzy line between what's here and what happens > in install-finish and in other parts of the orchestrator. I'd like to > see us have a discussion about where these things actually belong, > because right now it's a big mish-mash of things (especially things > like removing menu.lst here, then install-finish creates a different > one). A few things look like they belong here, such as some of the > unlnk_list and the cleanup of /var/tmp, but others may not. I will bring this up in the next Slim meeting. > > > 482-484: please leave the ssh keys alone, they're now generated on > each boot and should just be transferred and used > > 487: I don't believe we're creating /.mounted any more, that was in > anil's fix for bug 700. Done. > > transfermod.c > > numerous cstyle errors here... > > 1: Missing our CDDL and copyright Fixed these. > > 41: I presume we're going to switch this over to using liblogsvc > > 237,255,261: needs to be a log message Yes updated the code to use logsvc. > > Makefile.master, Makefile.lib, Makefile.targ, Makefile > > update copyright and remove SCCS idents Done. Regards, Moinak. > > > Dave
