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
_______________________________________________
et-mgmt-tools mailing list
et-mgmt-tools@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/et-mgmt-tools
