On 28.08.2013 23:53, Doug Goldstein wrote:
virVMXFormatHardDisk() and virVMXFormatCDROM() duplicated a lot of code from each other and made a lot of nested if checks to build each part of the VMX file. This hopefully simplifies the code path while combining the two functions with no net difference. --- src/libvirt_vmx.syms | 3 +- src/vmx/vmx.c | 192 +++++++++++++++++---------------------------------- src/vmx/vmx.h | 5 +- 3 files changed, 64 insertions(+), 136 deletions(-)
diff --git a/src/libvirt_vmx.syms b/src/libvirt_vmx.syms index 206ad44..e673923 100644 --- a/src/libvirt_vmx.syms +++ b/src/libvirt_vmx.syms @@ -6,11 +6,10 @@ virVMXConvertToUTF8; virVMXDomainXMLConfInit; virVMXEscapeHex; -virVMXFormatCDROM; virVMXFormatConfig; +virVMXFormatDisk; virVMXFormatEthernet; virVMXFormatFloppy; -virVMXFormatHardDisk; virVMXFormatParallel; virVMXFormatSerial; virVMXFormatVNC; diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 35afe26..f5cb9fe 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -517,7 +517,6 @@ VIR_ENUM_IMPL(virVMXControllerModelSCSI, VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST, "UNUSED lsisas1078");
- /* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * Helpers */ @@ -3213,14 +3212,8 @@ virVMXFormatConfig(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virDomainDe for (i = 0; i < def->ndisks; ++i) { switch (def->disks[i]->device) { case VIR_DOMAIN_DISK_DEVICE_DISK: - if (virVMXFormatHardDisk(ctx, def->disks[i], &buffer) < 0) { - goto cleanup; - } - - break; - case VIR_DOMAIN_DISK_DEVICE_CDROM: - if (virVMXFormatCDROM(ctx, def->disks[i], &buffer) < 0) { + if (virVMXFormatDisk(ctx, def->disks[i], &buffer) < 0) { goto cleanup; }
@@ -3369,67 +3362,89 @@ virVMXFormatVNC(virDomainGraphicsDefPtr def, virBufferPtr buffer) return 0; }
- - int -virVMXFormatHardDisk(virVMXContext *ctx, virDomainDiskDefPtr def, +virVMXFormatDisk(virVMXContext *ctx, virDomainDiskDefPtr def, virBufferPtr buffer) { int controllerOrBus, unit; - const char *busName = NULL; - const char *entryPrefix = NULL; - const char *deviceTypePrefix = NULL; + const char *vmxDeviceType = NULL; char *fileName = NULL;
- if (def->device != VIR_DOMAIN_DISK_DEVICE_DISK) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument")); + /* Convert a handful of types to their string values */ + const char *busType = virDomainDiskBusTypeToString(def->bus); + const char *deviceType = virDomainDeviceTypeToString(def->device); + const char *diskType = virDomainDeviceTypeToString(def->type); + + /* If we are dealing with a disk its a .vmdk, otherwise it must be + * an ISO. + */ + const char *fileExt = (def->device == VIR_DOMAIN_DISK_DEVICE_DISK) ? + ".vmdk" : ".iso"; + + /* Check that we got a valid device type */ + if (def->device != VIR_DOMAIN_DISK_DEVICE_DISK && + def->device != VIR_DOMAIN_DISK_DEVICE_CDROM) {
Indentation looks off.
+ virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid device type supplied: %s"), deviceType); return -1; }
- if (def->bus == VIR_DOMAIN_DISK_BUS_SCSI) { - busName = "SCSI"; - entryPrefix = "scsi"; - deviceTypePrefix = "scsi"; + /* We only support type='file' and type='block' */ + if (def->type != VIR_DOMAIN_DISK_TYPE_FILE && + def->type != VIR_DOMAIN_DISK_TYPE_BLOCK) {
And again.
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("%s %s '%s' has unsupported type '%s', expecting " + "'%s' or '%s'"), busType, deviceType, def->dst, diskType,
And again.
+ virDomainDiskTypeToString(VIR_DOMAIN_DISK_TYPE_FILE), + virDomainDiskTypeToString(VIR_DOMAIN_DISK_TYPE_BLOCK)); + return -1; + }
+ if (def->bus == VIR_DOMAIN_DISK_BUS_SCSI) { if (virVMXSCSIDiskNameToControllerAndUnit(def->dst, &controllerOrBus, &unit) < 0) { return -1; } } else if (def->bus == VIR_DOMAIN_DISK_BUS_IDE) { - busName = "IDE"; - entryPrefix = "ide"; - deviceTypePrefix = "ata"; - if (virVMXIDEDiskNameToBusAndUnit(def->dst, &controllerOrBus, - &unit) < 0) { + &unit) < 0) { return -1; } } else { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unsupported bus type '%s' for harddisk"), - virDomainDiskBusTypeToString(def->bus)); + _("Unsupported bus type '%s' for %s"), + busType, deviceType); return -1; }
- if (def->type != VIR_DOMAIN_DISK_TYPE_FILE) { + if (def->device == VIR_DOMAIN_DISK_DEVICE_DISK && + def->type == VIR_DOMAIN_DISK_TYPE_FILE) {
So does it here.
+ vmxDeviceType = (def->bus == VIR_DOMAIN_DISK_BUS_SCSI) ? + "scsi-hardDisk" : "ata-hardDisk"; + } else if (def->device == VIR_DOMAIN_DISK_DEVICE_CDROM) { + if (def->type == VIR_DOMAIN_DISK_TYPE_FILE) + vmxDeviceType = "cdrom-image"; + else + vmxDeviceType = "atapi-cdrom"; + } else { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("%s harddisk '%s' has unsupported type '%s', expecting '%s'"), - busName, def->dst, virDomainDiskTypeToString(def->type), - virDomainDiskTypeToString(VIR_DOMAIN_DISK_TYPE_FILE)); + _("%s %s '%s' has an unsupported type '%s'"), + busType, deviceType, def->dst, diskType); return -1; }
virBufferAsprintf(buffer, "%s%d:%d.present = \"true\"\n", - entryPrefix, controllerOrBus, unit); - virBufferAsprintf(buffer, "%s%d:%d.deviceType = \"%s-hardDisk\"\n", - entryPrefix, controllerOrBus, unit, deviceTypePrefix); + busType, controllerOrBus, unit); + virBufferAsprintf(buffer, "%s%d:%d.deviceType = \"%s\"\n", + busType, controllerOrBus, unit, vmxDeviceType);
- if (def->src != NULL) { - if (! virFileHasSuffix(def->src, ".vmdk")) { + if (def->type == VIR_DOMAIN_DISK_TYPE_FILE) { + if (def->src != NULL && ! virFileHasSuffix(def->src, fileExt)) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Image file for %s harddisk '%s' has unsupported suffix, " - "expecting '.vmdk'"), busName, def->dst); - return -1; + _("Image file for %s %s '%s' has " + "unsupported suffix, expecting '%s'"),
And again.
+ busType, deviceType, def->dst, fileExt); + return -1;
And again.
}
fileName = ctx->formatFileName(def->src, ctx->opaque);
diff --git a/src/vmx/vmx.h b/src/vmx/vmx.h index ec63317..cdf6b76 100644 --- a/src/vmx/vmx.h +++ b/src/vmx/vmx.h @@ -115,12 +115,9 @@ char *virVMXFormatConfig(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt,
int virVMXFormatVNC(virDomainGraphicsDefPtr def, virBufferPtr buffer);
-int virVMXFormatHardDisk(virVMXContext *ctx, virDomainDiskDefPtr def, +int virVMXFormatDisk(virVMXContext *ctx, virDomainDiskDefPtr def, virBufferPtr buffer);
And again.
-int virVMXFormatCDROM(virVMXContext *ctx, virDomainDiskDefPtr def, - virBufferPtr buffer); - int virVMXFormatFloppy(virVMXContext *ctx, virDomainDiskDefPtr def, virBufferPtr buffer, bool floppy_present[2]);
ACK with those fixed. Michal