[libvirt] [PATCHv3 1/4] VMX: Create virVMXFormatDisk() from HD and CD-ROM
Doug Goldstein
cardoe at cardoe.com
Mon Sep 2 04:22:26 UTC 2013
On Thu, Aug 29, 2013 at 5:19 AM, Michal Privoznik <mprivozn at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130901/eda4617b/attachment-0001.htm>
More information about the libvir-list
mailing list