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