On Thu, Aug 29, 2013 at 5:19 AM, Michal Privoznik <mpriv...@redhat.com>wrote:
> 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 > Thanks. Fixed and pushed. -- Doug Goldstein
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list