On Thu, Nov 29, 2007 at 08:14:50PM -0800, Robert Nelson wrote: > Daniel P. Berrange wrote: > >>I spent some time reworking the virtinst code to support OpenSolaris and > >>make it much easier to support additional OSes. I've attached the patch > >>file to get some feedback on the work so far. > >> > > > >Thanks for this. Took me a little while to get to grips with it, but I > >like the resulting structure. I'm going todo a few tests and if it > >works I'll commit it to the repo. > > > >BTW, for any large patches in the future, its handy for review if you > >do separate patches for re-factoring existing code, vs adding new > >capabilities. It would have made it easier to review if the splitting > >up of the DistroManager.py file were a simple no-functional-change > >refactoring. Also helps people browsing the SCM history in the future > >to distinguish the changes. No need to change this existing patch > >though, I'll just add as is... > > Please hold off on committing this patch. I have an updated and better > version coming shortly. I'll post it as two patches to make the SCM > history more readable. I'll also post it to a new thread. So it turned out to be a little more complicated than I expected. At first I liked the idea of having OSDistro create & use the ImageFetcher class directly, however, there are circumstances where we need to use the OSDistro class without the ImageFetcher, so this dependancy causes problems. Second, the FullVirtGuest/ParaVirtGuest classes now call into the installer class to get the disk target - this is a problem because this code is only relevant for the DistroInstaller class - the ImageInstaller works in a different way. Really, theFullVirt/ParaVirt guest class should not try todo any assignement of disk targets at all. The 'prepare' method fo the DistroInstaller class should contain the code to assign disk targets directly. This simplifies the get_disk_xml methods in the Full/ParaVirtGuest classes, and avoids the hard dependancy. So, in the end I just committed two small parts. I pulled the ImageFetcher classes out into a separate python module, and pulled the OSDistro classes out into a separate python module. So just 2 no-functional-change refactorings for now. If you post your latest patches I'll give them another try & see if there's more bits we can safely commit as we go. > >>Unfortunately a bunch for the code from virtinst is duplicated in > >>virt-manager in the Add Device code. This means either moving it back > >>to virtinst with the appropriate additional APIs or duplicating work in > >>virt-manager. > >> > > > >We should make virt-manager call into the virtinst APIs really. virtinst > >is intended as the library for virt-manager to get all OS install/setup > >logic from. > > > > > > If you don't mind, I'll put together a prototype of this for review. Sure. The complicated thing about the 'Add device' code is that we no longer have any record of the OS Distro type at that point - this info is only available at install time. So its not clear how we'd be able to get an instance of the OSDistro class neccessary to figure out the disk target names here. Also this is one use case I mentioned where you need OSDistro to be independant of the ImageFetcher class. BTW, I'm not convinced the Solaris distro class is correct. I know the paravirt guests have to name their disk targets 0, 1, 2 ... etc isntead of xvda, xvdb, etc. For fully-virt Solaris installs though it must still use hda, hdb, hdc, etc as per Linux. This is because the hda, hdb, etc names translate directly to QEMU command line args - they have no resemblance to what the guest OS calls its disks - indeed modern Linux will call them sda, sdb, sdc, etc regardless of what QEMU calls tem. Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=| _______________________________________________ et-mgmt-tools mailing list et-mgmt-tools@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/et-mgmt-tools
