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