[et-mgmt-tools] [PATCH] Add support for Solaris PV

[John Levon]

On Thu, Dec 04, 2008 at 12:37:00PM +0000, Daniel P. Berrange wrote:

> The new Installer class for Solaris looks fine to me, but I don't really
> like the renaming of os_type to virt_type, since its conflicting naming
> to the libvirt usage.
> 
> 'virt type' is usually used to refer to hypervisor type, eg this bit
> of the XML
> 
>   <domain type='xen'>
>   <domain type='kvm'>
>   <domain type='qemu'>

How confusing. Wouldn't this be 'hyp_type' ?

> Unfortunaltey the virt-install CLI flag is --os-type - should really
> have been --os-distro-type. So I think I'd rather we changed the
> insternal variables for this to be os_distro_type and os_distro_variant

I can do this though. Anything but the current mess :)

> > -            if not self.extraargs is None:
> > -                self.install["extraargs"] = self.extraargs + " " + args
> > -            else:
> > -                self.install["extraargs"] = args
> > +            self.install["extraargs"] = args
> 
> This chunk looks a little questionable to me - IIRC this is reverting a fix
> we previously made, so perhaps this is just a merge error.

Hmm, no, this is intentional. Solaris needs to do all sorts of mangling
of extra args passed in. It's now the responsibility of the OSDistro to
handle .extraargs - see the change to _kernelHelper or whatever it's
called. We can't just do the pre-pending of extra args.

> > +
> > +        if not fetcher.location.startswith("/"):
> > +            args += "method=" + fetcher.location
> > +
> > +        if guest.extraargs:
> > +            args += guest.extraargs
> > +
> >          try:
> >              initrd = fetcher.acquireFile(initrdpath, progresscb)
> > -            if fetcher.location.startswith("/"):
> > -                # Local host path, so can't pass a location to guest
> > -                #for install method
> > -                return (kernel, initrd, "")
> > -            else:
> > -                return (kernel, initrd, "method=" + fetcher.location)
> > +            return kernel, initrd, args
> 
> Hmm, this existing code was dubious  even before this change. method= is
> the Fedora / RHEL anaconda syntax for specifying install location. It
> should never havebeen in Distro class in the first place - instead it
> belongs in the RedHatDistro subclass.

This seems like a change to be made separately.

> These new classes seeem to be more or less independant of the bit
> os_type/virt_type renaming (baring the obvious fact that it uses the
> new names). I've no problem add this bit right away.

If you like, I can split out the os_type renaming as a separate patch.

> >          if self.target:
> >              disknode = self.target
> > +        if self.device == VirtualDisk.DEVICE_CDROM and disknode.startswith('xvd'):
> > +            disknode = disknode + ':cdrom'
> > +
> 
> This seems wrong - you should not pass "xvda:cdrom" into the libvirt
> XML for a disk. There is an explicit attribute for specifying CDROM
> media

Ack.

regards
john