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/ signature.asc Description: Digital signature -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Re: [Libvir] [PATCH] Add bus attribute to disk target definition
On Wed, May 07, 2008 at 09:38:26AM +0200, Soren Hansen wrote: 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. :) The thing is that you can specify multiple boot devices in the XML to set a priority, eg boot dev=network/ boot dev=hd/ Gets maps to '--boot nc' so in this scenario you'd have a virtio device being bootable, and also have PXE enabled. 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. FWIW, I always start with the test cases first defining the XML and QEMU args, and then write the impl until the test cases pass. Dan. -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] Re: [Libvir] [PATCH] Add bus attribute to disk target definition
On Tue, May 06, 2008 at 08:11:11PM +0200, Soren Hansen 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, nor re-add the -boot param to make PXE work again. 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'
=== modified file 'src/qemu_conf.c'
--- old/src/qemu_conf.c 2008-04-30 12:30:55 +
+++ new/src/qemu_conf.c 2008-05-06 17:58:44 +
@@ -491,6 +491,8 @@
*flags |= QEMUD_CMD_FLAG_KQEMU;
if (strstr(help, -no-reboot))
*flags |= QEMUD_CMD_FLAG_NO_REBOOT;
+if (strstr(help, \n-drive))
+*flags |= QEMUD_CMD_FLAG_DRIVE_OPT;
Turns out that this needed to also check for 'boot=on' availability.
@@ -673,13 +676,28 @@
goto error;
}
+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'.
+else if (STREQ((const char *)bus, ide))
+disk-bus = QEMUD_DISK_BUS_IDE;
+else if (STREQ((const char *)bus, scsi))
+disk-bus = QEMUD_DISK_BUS_SCSI;
+else if (STREQ((const char *)bus, virtio))
+disk-bus = QEMUD_DISK_BUS_VIRTIO;
+else {
+qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
_(Invalid bus type: %s), bus);
+goto error;
+}
+
xmlFree(device);
xmlFree(target);
xmlFree(source);
+xmlFree(bus);
return 0;
error:
+xmlFree(bus);
xmlFree(type);
xmlFree(target);
xmlFree(source);
@@ -1373,6 +1391,68 @@
return -1;
}
+static int qemudDiskCompare(const void *aptr, const void *bptr) {
+struct qemud_vm_disk_def *a = (struct qemud_vm_disk_def *) aptr;
+struct qemud_vm_disk_def *b = (struct qemud_vm_disk_def *) bptr;
+if (a-device == b-device)
+return virDiskNameToIndex(a-dst) - virDiskNameToIndex(b-dst);
+else
+return a-device - b-device;
Typo - this should be comparing the 'bus' field rather than 'device' to
get IDE, SCSI and VirtIO disks ordered correctly wrt to each other.
+static const char *qemudBusIdToName(int busId) {
+const char *busnames[] = { ide,
+scsi,
+virtio };
+
+ if (busId = 0 busId 3)
+ return busnames[busId];
+ else
+ return 0;
+}
This can be changed to make use of the GNULIB 'verify_true' function
to get a compile time check fo the array cardinality vs the enum
length.
+static char *qemudDriveOpt(struct qemud_vm_disk_def *disk, int boot)
+{
+char opt[PATH_MAX];
+
+switch (disk-device) {
+case QEMUD_DISK_CDROM:
+snprintf(opt, PATH_MAX, file=%s,if=ide,media=cdrom%s,
+ disk-src, boot ? ,boot=on : );
+break;
+case QEMUD_DISK_FLOPPY:
+snprintf(opt, PATH_MAX, file=%s,if=floppy%s,
+ disk-src, boot ? ,boot=on : );
+break;
+case QEMUD_DISK_DISK:
+snprintf(opt, PATH_MAX, file=%s,if=%s%s,
+ disk-src, qemudBusIdToName(disk-bus), boot ?
,boot=on : );
This is missing the 'index=NNN' parameter so IDE disks are not getting
assigned to the correct bus/unit. eg if i define 'hda' and 'hdd' in the
XML the guest gets given 'hda' and 'hdb'.
@@ -1775,13 +1854,20 @@
goto error;
}
def-ndisks++;
-disk-next = NULL;
if (i == 0) {
+disk-next = NULL;
def-disks = disk;
} else {
-prev-next = disk;
+struct qemud_vm_disk_def *ptr = def-disks;
+while (ptr) {
+if (!ptr-next || qemudDiskCompare(ptr-next, disk) 0)
{
The parameters to the compare function are inverted so the ordering
is being reversed.
@@ -2110,6 +2196,7 @@
struct qemud_vm_chr_def *parallel = vm-def-parallels;
struct utsname ut;
int disableKQEMU = 0;
+int use_extboot = 0;
/* Make sure the binary we are about to try exec'ing exists.
* Technically we could catch the exec() failure, but that's
@@ -2230,30 +2317,47 @@
goto no_memory;
}
-for (i = 0 ; i vm-def-os.nBootDevs ; i++) {
-switch (vm-def-os.bootDevs[i]) {
-case QEMUD_BOOT_CDROM:
-boot[i] = 'd';
-break;
-case QEMUD_BOOT_FLOPPY:
-boot[i] = 'a';
-break;
-case QEMUD_BOOT_DISK:
-boot[i] =
