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