Re: [libvirt] Re: [Libvir] [PATCH] Add bus attribute to disk target definition
On Tue, May 06, 2008 at 10:02:21PM +0100, Daniel P. Berrange wrote:
> sorry for the delay. Here's the newest version of the patch
which
> should address all the issues that have been raised so far.
Yes & no. It didn't address the redundant re-ordering of -drive
parameters when using boot=on,
The reasoning here was (I mentioned this in a previous mail, too) that
when qemu/kvm some day grows the ability to have more than one boot
device specified with boot=on (using extboot or whatever), we're going
to have to change things *anyway*. Ordering the devices according to
boot priority seems like a reasonable guess as to what would be required
to do, so I figured I'd leave it as is.
nor re-add the -boot param to make PXE work again.
Yes and no. In the latest patch I provided I only set use_extboot if
there's only one boot device defined, and it's a virtio device, so PXE
booting would use the old-style "-boot n" syntax. I literallly woke up
this morning and instantly smacked my forehead due to another problem
this introduced, so I'm happy you changed it. :)
One further complication is that QEMU 0.9.1 has -drive support but
not
the extboot support, so boot=on can't be used there. It rather
annoyingly complains
qemu: unknowm parameter 'boot' in
'file=/home/berrange/boot.iso,media=cdrom,boot=on'
Ah, figures.
> + if (!bus)
> + disk->bus = QEMUD_DISK_BUS_IDE;
This was giving floppy disks a bus of 'ide' which isn't technically
correct - they're really their own bus type - I reckon we should call
it 'fdc'.
Ah. Yes, I must admit that floppy disks were completely off my radar.
This double loop is redundant - there's no need to pull bootable
drives to the front of the list - this is why there's an explicit flag
rather than relying on ordering.
I did this for two reasons:
a) I wanted to avoid the bootDisk, bootFloppy, bootCD variables approach
you used. It just didn't appeal to me. *shrug*
b) As I mentioned further up, this was also done in an attempt to match
what would be needed when it becomes possible to specify ",boot=on" for
more than one device, but we can revisit this when that day comes.
Your patch looks fine to me.
Oh, and thanks for doing all the test cases as well. I didn't want to
get started on those until we had agreed on the logic that should be
applied.
--
Soren Hansen |
Virtualisation specialist | Ubuntu Server Team
Canonical Ltd. | http://www.ubuntu.com/
Attachments:
- signature.asc (application/pgp-signature — 189 bytes)