On Tue, Apr 29, 2008 at 09:56:13AM +0200, Soren Hansen wrote:
> Hi!
>
> I'd like to propose that the following patch gets applied against
> libvirt. It adds the option of putting a bus attribute on a disk target.
> To acommodate this, it also changes the way drives are defined for kvm
> from the old "-hda /path/to/file -boot c" style to the new "-drive
> file=/path/to/file,if=ide,boot=on". This makes it possible to specify
> virtio, scsi, and ide disks.
> @@ -646,7 +648,9 @@
> strcmp((const char *)target, "hda") &&
> strcmp((const char *)target, "hdb") &&
> strcmp((const char *)target, "hdc") &&
> - strcmp((const char *)target, "hdd")) {
> + strcmp((const char *)target, "hdd") &&
> + strcmp((const char *)target, "hdd") &&
These two lines test the same thing !
> + strncmp((const char *)target, "vd", 2)) {
Its probably a better idea to just compress the explicit hda -> hdd
comparisons down into a single 'hd' prefix comparison, as you did
with 'vd'.
> + if (!bus)
> + disk->bus = QEMUD_DISK_BUS_IDE;
> + else if (!strcmp((const char *)bus, "ide"))
> + disk->bus = QEMUD_DISK_BUS_IDE;
> + else if (!strcmp((const char *)bus, "scsi"))
> + disk->bus = QEMUD_DISK_BUS_SCSI;
> + else if (!strcmp((const char *)bus, "virtio"))
> + disk->bus = QEMUD_DISK_BUS_VIRTIO;
Can you use the STREQ macro here instead of strcmp.
> + else {
> + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, "Invalid bus type: %s", bus);
> + goto error;
> + }
This line needs to be marked for translation, with _(...). You can
run 'make syntax-check' for check for such issues.
> +static char *qemudAddBootDrive(virConnectPtr conn,
> + struct qemud_vm_def *def,
> + char *handledDisks,
> + int type) {
> + int j = 0;
> + struct qemud_vm_disk_def *disk = def->disks;
> +
> + while (disk) {
> + if (!handledDisks[j] && disk->device == type) {
> + handledDisks[j] = 1;
> + return qemudDriveOpt(disk, 1);
> + }
> + j++;
> + disk = disk->next;
> + }
> + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> + "Requested boot device type %d, but no such device defined.", type);
> + return 0;
> +}
>
> @@ -2207,30 +2295,32 @@
> 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] = 'c';
> - break;
> - case QEMUD_BOOT_NET:
> - boot[i] = 'n';
> - break;
> - default:
> - boot[i] = 'c';
> - break;
> + if (vm->def->virtType != QEMUD_VIRT_KVM) {
This is wrong - both QEMU and KVM can support the -drive parameter,
and not all KVM support it. You need to add a check in the method
qemudExtractVersionInfo() to probe for availability of the -drive
option, as we do with other opts - see QEMUD_CMD_FLAG_NO_REBOOT
for example.
Even if the -drive parameter is supported, it should still pass the
-boot a/c/d/n parameter in.
> + if (vm->def->virtType == QEMUD_VIRT_KVM) {
> + char *handledDisks = NULL;
> + int j;
> +
> + handledDisks = calloc(sizeof(*handledDisks), vm->def->ndisks);
> +
> + if (!handledDisks)
> + goto no_memory;
> +
> + /* When using -drive notation, we need to provide the devices in boot
> + * preference order. */
> + for (i = 0 ; i < vm->def->os.nBootDevs ; i++) {
> + if (!((*argv)[++n] = strdup("-drive")))
> + goto no_memory;
> +
> + switch (vm->def->os.bootDevs[i]) {
> + case QEMUD_BOOT_CDROM:
> + if (!((*argv)[++n] = qemudAddBootDrive(conn, vm->def, handledDisks, QEMUD_DISK_CDROM)))
> + goto error;
> + break;
> + case QEMUD_BOOT_FLOPPY:
> + if (!((*argv)[++n] = qemudAddBootDrive(conn, vm->def, handledDisks, QEMUD_DISK_FLOPPY)))
> + goto error;
> + break;
> + case QEMUD_BOOT_DISK:
> + if (!((*argv)[++n] = qemudAddBootDrive(conn, vm->def, handledDisks, QEMUD_DISK_DISK)))
> + goto error;
> + break;
> + }
> + }
There is nothing in the -drive parameter handling, AFAICT, that requires
the boot drive to be listed first on the command line. So this first loop
is not needed, and this second loop is sufficient, simply turn on the
boot= flag on the first match drive type when iterating over the list.
> +
> + /* Pick up the rest of the devices */
> + j=0;
> + while (disk) {
> + if (!handledDisks[j]) {
> + handledDisks[j] = 1;
> + if (!((*argv)[++n] = strdup("-drive")))
> + goto no_memory;
> + if (!((*argv)[++n] = qemudDriveOpt(disk, 0)))
> + goto no_memory;
> + }
> + disk = disk->next;
> + j++;
> + }
> === modified file 'src/util.c'
> --- src/util.c 2008-04-25 14:53:05 +0000
> +++ src/util.c 2008-04-29 06:59:49 +0000
> @@ -771,3 +771,43 @@
>
> return -1;
> }
> +
> +/* Translates a device name of the form (regex) "[fhv]d[a-z]+" into
> + * the corresponding index (e.g. sda => 1, hdz => 26, vdaa => 27)
> + * @param name The name of the device
> + * @return name's index, or 0 on failure
> + */
> +int virDiskNameToIndex(const char *name) {
> + const char *ptr = NULL;
> + int idx = 0;
> +
> + if (strlen(name) < 3)
> + return 0;
> +
> + switch (*name) {
> + case 'f':
> + case 'h':
> + case 'v':
You need a case 's' there to allow for SCSI...
Regards,
Daniel
--
|: 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list