Re: [libvirt] [PATCHv3 1/4] VMX: Create virVMXFormatDisk() from HD and CD-ROM

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