Re: [libvirt] [PATCH 6/8] qemu: Translate the pool disk source earlier

On 04/04/2013 03:38 PM, Osier Yang wrote: > To support "shareable" for volume type disk, we have to translate > the source before trying to add the shared disk entry. To archieve s/archeive/achieve/ > the goal, this moves the helper qemuTranslateDiskSourcePool into > src/qemu/qemu_conf.c, and introduce a internal only member (voltype) s/a internal/an internal/ > for struct _virDomainDiskSourcePoolDef, to record the underlying > volume type, for use when building the drive string. s/type,/type/ > Later patch will support "shareable" volume type disk. > --- > src/conf/domain_conf.h | 1 + > src/qemu/qemu_command.c | 56 +------------------------------------------------ > src/qemu/qemu_command.h | 1 - > src/qemu/qemu_conf.c | 52 +++++++++++++++++++++++++++++++++++++++++++++ > src/qemu/qemu_conf.h | 3 +++ > src/qemu/qemu_driver.c | 16 ++++++++++---- > src/qemu/qemu_process.c | 7 +++++++ > 7 files changed, 76 insertions(+), 60 deletions(-) > > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index 988636e..13d6d89 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -611,6 +611,7 @@ typedef struct _virDomainDiskSourcePoolDef virDomainDiskSourcePoolDef; > struct _virDomainDiskSourcePoolDef { > char *pool; /* pool name */ > char *volume; /* volume name */ > + int voltype; /* enum virStorageVolType, internal only */ > }; > typedef virDomainDiskSourcePoolDef *virDomainDiskSourcePoolDefPtr; > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index cdfe801..c29796d 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -2681,56 +2681,6 @@ qemuBuildNBDString(virConnectPtr conn, virDomainDiskDefPtr disk, virBufferPtr op > return 0; > } > > -static int > -qemuTranslateDiskSourcePool(virConnectPtr conn, > - virDomainDiskDefPtr def, > - int *voltype) > -{ > - virStoragePoolPtr pool = NULL; > - virStorageVolPtr vol = NULL; > - virStorageVolInfo info; > - int ret = -1; > - > - if (def->type != VIR_DOMAIN_DISK_TYPE_VOLUME) > - return 0; > - > - if (!(pool = virStoragePoolLookupByName(conn, def->srcpool->pool))) > - return -1; > - > - if (!(vol = virStorageVolLookupByName(pool, def->srcpool->volume))) > - goto cleanup; > - > - if (virStorageVolGetInfo(vol, &info) < 0) > - goto cleanup; > - > - if (def->startupPolicy && > - info.type != VIR_STORAGE_VOL_FILE) { > - virReportError(VIR_ERR_XML_ERROR, "%s", > - _("'startupPolicy' is only valid for 'file' type volume")); > - goto cleanup; > - } > - > - switch (info.type) { > - case VIR_STORAGE_VOL_FILE: > - case VIR_STORAGE_VOL_BLOCK: > - case VIR_STORAGE_VOL_DIR: > - if (!(def->src = virStorageVolGetPath(vol))) > - goto cleanup; > - break; > - case VIR_STORAGE_VOL_NETWORK: > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > - _("Using network volume as disk source is not supported")); > - goto cleanup; > - } > - > - *voltype = info.type; > - ret = 0; > -cleanup: > - virStoragePoolFree(pool); > - virStorageVolFree(vol); > - return ret; > -} > - > char * > qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, > virDomainDiskDefPtr disk, > @@ -2743,7 +2693,6 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, > virDomainDiskGeometryTransTypeToString(disk->geometry.trans); > int idx = virDiskNameToIndex(disk->dst); > int busid = -1, unitid = -1; > - int voltype = -1; > > if (idx < 0) { > virReportError(VIR_ERR_INTERNAL_ERROR, > @@ -2751,9 +2700,6 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, > goto error; > } > > - if (qemuTranslateDiskSourcePool(conn, disk, &voltype) < 0) > - goto error; > - > switch (disk->bus) { > case VIR_DOMAIN_DISK_BUS_SCSI: > if (disk->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE) { > @@ -2889,7 +2835,7 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, > break; > } > } else if (disk->type == VIR_DOMAIN_DISK_TYPE_VOLUME) { > - switch (voltype) { > + switch (disk->srcpool->voltype) { > case VIR_STORAGE_VOL_DIR: > if (!disk->readonly) { > virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h > index 17687f4..20e3066 100644 > --- a/src/qemu/qemu_command.h > +++ b/src/qemu/qemu_command.h > @@ -237,5 +237,4 @@ qemuParseKeywords(const char *str, > char ***retvalues, > int allowEmptyValue); > > - > #endif /* __QEMU_COMMAND_H__*/ > diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c > index c2e2e10..8ae690f 100644 > --- a/src/qemu/qemu_conf.c > +++ b/src/qemu/qemu_conf.c > @@ -1240,3 +1240,55 @@ int qemuDriverAllocateID(virQEMUDriverPtr driver) > { > return virAtomicIntInc(&driver->nextvmid); > } > + > +int > +qemuTranslateDiskSourcePool(virConnectPtr conn, > + virDomainDiskDefPtr def) > +{ > + virStoragePoolPtr pool = NULL; > + virStorageVolPtr vol = NULL; > + virStorageVolInfo info; > + int ret = -1; > + > + if (def->type != VIR_DOMAIN_DISK_TYPE_VOLUME) > + return 0; > + > + if (!def->srcpool) > + return 0; > + > + if (!(pool = virStoragePoolLookupByName(conn, def->srcpool->pool))) > + return -1; > + > + if (!(vol = virStorageVolLookupByName(pool, def->srcpool->volume))) > + goto cleanup; > + > + if (virStorageVolGetInfo(vol, &info) < 0) > + goto cleanup; > + > + if (def->startupPolicy && > + info.type != VIR_STORAGE_VOL_FILE) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("'startupPolicy' is only valid for 'file' type volume")); > + goto cleanup; > + } > + > + switch (info.type) { > + case VIR_STORAGE_VOL_FILE: > + case VIR_STORAGE_VOL_BLOCK: > + case VIR_STORAGE_VOL_DIR: > + if (!(def->src = virStorageVolGetPath(vol))) > + goto cleanup; > + break; > + case VIR_STORAGE_VOL_NETWORK: > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("Using network volume as disk source is not supported")); > + goto cleanup; > + } > + > + def->srcpool->voltype = info.type; > + ret = 0; > +cleanup: > + virStoragePoolFree(pool); > + virStorageVolFree(vol); > + return ret; > +} > diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h > index c5ddaad..9f58454 100644 > --- a/src/qemu/qemu_conf.h > +++ b/src/qemu/qemu_conf.h > @@ -303,4 +303,7 @@ void qemuSharedDiskEntryFree(void *payload, const void *name) > int qemuDriverAllocateID(virQEMUDriverPtr driver); > virDomainXMLConfPtr virQEMUDriverCreateXMLConf(void); > > +int qemuTranslateDiskSourcePool(virConnectPtr conn, > + virDomainDiskDefPtr def); > + > #endif /* __QEMUD_CONF_H */ > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 552a81b..4e8c666 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -5748,6 +5748,9 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, > goto end; > } > > + if (qemuTranslateDiskSourcePool(conn, disk) < 0) > + goto end; > + > if (qemuAddSharedDisk(driver, disk, vm->def->name) < 0) > goto end; > > @@ -6016,7 +6019,8 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, > } > > static int > -qemuDomainChangeDiskMediaLive(virDomainObjPtr vm, > +qemuDomainChangeDiskMediaLive(virConnectPtr conn, > + virDomainObjPtr vm, > virDomainDeviceDefPtr dev, > virQEMUDriverPtr driver, > bool force) > @@ -6029,6 +6033,9 @@ qemuDomainChangeDiskMediaLive(virDomainObjPtr vm, > virCapsPtr caps = NULL; > int ret = -1; > > + if (qemuTranslateDiskSourcePool(conn, disk) < 0) > + goto end; > + > if (qemuDomainDetermineDiskChain(driver, disk, false) < 0) > goto end; > > @@ -6107,7 +6114,8 @@ end: > } > > static int > -qemuDomainUpdateDeviceLive(virDomainObjPtr vm, > +qemuDomainUpdateDeviceLive(virConnectPtr conn, > + virDomainObjPtr vm, > virDomainDeviceDefPtr dev, > virDomainPtr dom, > bool force) > @@ -6117,7 +6125,7 @@ qemuDomainUpdateDeviceLive(virDomainObjPtr vm, > > switch (dev->type) { > case VIR_DOMAIN_DEVICE_DISK: > - ret = qemuDomainChangeDiskMediaLive(vm, dev, driver, force); > + ret = qemuDomainChangeDiskMediaLive(conn, vm, dev, driver, force); > break; > case VIR_DOMAIN_DEVICE_GRAPHICS: > ret = qemuDomainChangeGraphics(driver, vm, dev->data.graphics); > @@ -6529,7 +6537,7 @@ qemuDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, > ret = qemuDomainDetachDeviceLive(vm, dev_copy, dom); > break; > case QEMU_DEVICE_UPDATE: > - ret = qemuDomainUpdateDeviceLive(vm, dev_copy, dom, force); > + ret = qemuDomainUpdateDeviceLive(dom->conn, vm, dev_copy, dom, force); > break; > default: > virReportError(VIR_ERR_INTERNAL_ERROR, > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index 8c4bfb7..3ac0c70 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -3008,6 +3008,8 @@ qemuProcessReconnect(void *opaque) > * qemu_driver->sharedDisks. > */ > for (i = 0; i < obj->def->ndisks; i++) { > + if (qemuTranslateDiskSourcePool(conn, obj->def->disks[i]) < 0) > + goto error; > if (qemuAddSharedDisk(driver, obj->def->disks[i], > obj->def->name) < 0) > goto error; > @@ -3556,6 +3558,11 @@ int qemuProcessStart(virConnectPtr conn, > goto cleanup; > } > > + for (i = 0; i < vm->def->ndisks; i++) { > + if (qemuTranslateDiskSourcePool(conn, vm->def->disks[i]) < 0) > + goto cleanup; > + } > + > VIR_DEBUG("Building emulator command line"); > if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig, > priv->monJSON != 0, priv->qemuCaps, > ACK; however, I see paths to qemuBuildDriveStr() that don't go through qemuBuildCommandLine(), so just double check that you aren't missing a place to get that disk->src set for one of these pool/vol's. The point being that qemuBuildDriveStr() "had" code that did something before (albeit in this set of patches). Since you're moving it - just be sure there's no path that could enter qemuBuildDriveStr() that would need it.