[libvirt] [PATCHv2 0/7] more of round 2 of virstoragefile refactoring

compared to v1, this is rebased on top of Cole's changes, treats disk partition types slightly differently, and adds a couple patches. 1/7: was 16/n, unreviewed so far 2/7: was 13/n with weak ack 3-4/7: was 14-15/n; commit messages improved to justify it further 5-7: all new patches I'm still working on how best to fold virStorageFileMetadata into virStorageSource; it's a bit tricky since the metadata type is rather awkward (storing type information about the backing file in the parent, rather than in the backing file) Eric Blake (7): conf: manage disk source by struct instead of pieces conf: tweak volume target struct details conf: move volume structs to util/ conf: use common struct in storage volumes conf: track sizes directly in source struct conf: drop redundant parameters during probe conf: modify tracking of encrypted images src/conf/domain_conf.c | 139 ++++++++++++------------------ src/conf/domain_conf.h | 24 ++---- src/conf/snapshot_conf.c | 17 +--- src/conf/storage_conf.c | 40 ++++----- src/conf/storage_conf.h | 51 ++--------- src/esx/esx_storage_backend_iscsi.c | 5 +- src/esx/esx_storage_backend_vmfs.c | 20 ++--- src/parallels/parallels_driver.c | 2 +- src/parallels/parallels_storage.c | 22 ++--- src/phyp/phyp_driver.c | 4 +- src/storage/storage_backend.c | 150 +++++++++++++++------------------ src/storage/storage_backend.h | 11 +-- src/storage/storage_backend_disk.c | 26 +++--- src/storage/storage_backend_fs.c | 32 +++---- src/storage/storage_backend_gluster.c | 12 ++- src/storage/storage_backend_logical.c | 18 ++-- src/storage/storage_backend_mpath.c | 8 +- src/storage/storage_backend_rbd.c | 11 +-- src/storage/storage_backend_scsi.c | 6 +- src/storage/storage_backend_sheepdog.c | 10 +-- src/storage/storage_driver.c | 40 ++++----- src/test/test_driver.c | 16 ++-- src/util/virstoragefile.c | 11 ++- src/util/virstoragefile.h | 30 ++++++- src/vbox/vbox_tmpl.c | 11 +-- tests/storagebackendsheepdogtest.c | 5 +- tests/virstoragetest.c | 4 +- 27 files changed, 322 insertions(+), 403 deletions(-) -- 1.9.0

Now that we have a dedicated type for representing a disk source, we might as well parse and format directly into that type instead of piecemeal into pointers to members of the type. * src/conf/domain_conf.h (virDomainDiskSourceDefFormatInternal) (virDomainDiskSourceDefParse): Rename... (virDomainDiskSourceFormat, virDomainDiskSourceParse): ...and compress signatures. * src/conf/domain_conf.c (virDomainDiskSourceParse) (virDomainDiskSourceFormat): Rewrite to use common struct. (virDomainDiskSourceDefFormat): Delete. (virDomainDiskDefParseXML, virDomainDiskDefFormat): Update callers. * src/conf/snapshot_conf.c (virDomainSnapshotDiskDefParseXML) (virDomainSnapshotDiskDefFormat): Likewise. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.c | 139 ++++++++++++++++++----------------------------- src/conf/domain_conf.h | 24 ++------ src/conf/snapshot_conf.c | 17 +----- 3 files changed, 62 insertions(+), 118 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0af5be7..6e71885 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4954,13 +4954,8 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node, int -virDomainDiskSourceDefParse(xmlNodePtr node, - int type, - char **source, - int *proto, - size_t *nhosts, - virStorageNetHostDefPtr *hosts, - virStorageSourcePoolDefPtr *srcpool) +virDomainDiskSourceParse(xmlNodePtr node, + virStorageSourcePtr src) { char *protocol = NULL; char *transport = NULL; @@ -4970,15 +4965,15 @@ virDomainDiskSourceDefParse(xmlNodePtr node, memset(&host, 0, sizeof(host)); - switch (type) { + switch (src->type) { case VIR_STORAGE_TYPE_FILE: - *source = virXMLPropString(node, "file"); + src->path = virXMLPropString(node, "file"); break; case VIR_STORAGE_TYPE_BLOCK: - *source = virXMLPropString(node, "dev"); + src->path = virXMLPropString(node, "dev"); break; case VIR_STORAGE_TYPE_DIR: - *source = virXMLPropString(node, "dir"); + src->path = virXMLPropString(node, "dir"); break; case VIR_STORAGE_TYPE_NETWORK: if (!(protocol = virXMLPropString(node, "protocol"))) { @@ -4987,14 +4982,14 @@ virDomainDiskSourceDefParse(xmlNodePtr node, goto cleanup; } - if ((*proto = virStorageNetProtocolTypeFromString(protocol)) < 0){ + if ((src->protocol = virStorageNetProtocolTypeFromString(protocol)) < 0){ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown protocol type '%s'"), protocol); goto cleanup; } - if (!(*source = virXMLPropString(node, "name")) && - *proto != VIR_STORAGE_NET_PROTOCOL_NBD) { + if (!(src->path = virXMLPropString(node, "name")) && + src->protocol != VIR_STORAGE_NET_PROTOCOL_NBD) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing name for disk source")); goto cleanup; @@ -5048,28 +5043,28 @@ virDomainDiskSourceDefParse(xmlNodePtr node, host.port = virXMLPropString(child, "port"); } - if (VIR_APPEND_ELEMENT(*hosts, *nhosts, host) < 0) + if (VIR_APPEND_ELEMENT(src->hosts, src->nhosts, host) < 0) goto cleanup; } child = child->next; } break; case VIR_STORAGE_TYPE_VOLUME: - if (virDomainDiskSourcePoolDefParse(node, srcpool) < 0) + if (virDomainDiskSourcePoolDefParse(node, &src->srcpool) < 0) goto cleanup; break; default: virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected disk type %s"), - virStorageTypeToString(type)); + virStorageTypeToString(src->type)); goto cleanup; } /* People sometimes pass a bogus '' source path when they mean to omit the * source element completely (e.g. CDROM without media). This is just a * little compatibility check to help those broken apps */ - if (*source && STREQ(*source, "")) - VIR_FREE(*source); + if (src->path && !*src->path) + VIR_FREE(src->path); ret = 0; @@ -5107,7 +5102,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, char *sgio = NULL; char *driverName = NULL; char *driverType = NULL; - char *source = NULL; + const char *source = NULL; char *target = NULL; char *trans = NULL; char *bus = NULL; @@ -5176,13 +5171,9 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, xmlStrEqual(cur->name, BAD_CAST "source")) { sourceNode = cur; - if (virDomainDiskSourceDefParse(cur, def->src.type, - &source, - &def->src.protocol, - &def->src.nhosts, - &def->src.hosts, - &def->src.srcpool) < 0) + if (virDomainDiskSourceParse(cur, &def->src) < 0) goto error; + source = def->src.path; if (def->src.type == VIR_STORAGE_TYPE_NETWORK) { if (def->src.protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI) @@ -5799,8 +5790,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, def->startupPolicy = val; } - def->src.path = source; - source = NULL; def->dst = target; target = NULL; def->src.auth.username = authUsername; @@ -5852,7 +5841,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_FREE(rawio); VIR_FREE(sgio); VIR_FREE(target); - VIR_FREE(source); VIR_FREE(tray); VIR_FREE(removable); VIR_FREE(trans); @@ -14694,17 +14682,10 @@ virDomainDiskSourceDefFormatSeclabel(virBufferPtr buf, } int -virDomainDiskSourceDefFormatInternal(virBufferPtr buf, - int type, - const char *src, - int policy, - int protocol, - size_t nhosts, - virStorageNetHostDefPtr hosts, - size_t nseclabels, - virSecurityDeviceLabelDefPtr *seclabels, - virStorageSourcePoolDefPtr srcpool, - unsigned int flags) +virDomainDiskSourceFormat(virBufferPtr buf, + virStorageSourcePtr src, + int policy, + unsigned int flags) { size_t n; const char *startupPolicy = NULL; @@ -14712,51 +14693,56 @@ virDomainDiskSourceDefFormatInternal(virBufferPtr buf, if (policy) startupPolicy = virDomainStartupPolicyTypeToString(policy); - if (src || nhosts > 0 || srcpool || startupPolicy) { - switch (type) { + if (src->path || src->nhosts > 0 || src->srcpool || startupPolicy) { + switch ((enum virStorageType)src->type) { case VIR_STORAGE_TYPE_FILE: virBufferAddLit(buf, "<source"); - virBufferEscapeString(buf, " file='%s'", src); + virBufferEscapeString(buf, " file='%s'", src->path); virBufferEscapeString(buf, " startupPolicy='%s'", startupPolicy); - virDomainDiskSourceDefFormatSeclabel(buf, nseclabels, seclabels, flags); + virDomainDiskSourceDefFormatSeclabel(buf, src->nseclabels, + src->seclabels, flags); break; case VIR_STORAGE_TYPE_BLOCK: virBufferAddLit(buf, "<source"); - virBufferEscapeString(buf, " dev='%s'", src); + virBufferEscapeString(buf, " dev='%s'", src->path); virBufferEscapeString(buf, " startupPolicy='%s'", startupPolicy); - virDomainDiskSourceDefFormatSeclabel(buf, nseclabels, seclabels, flags); + virDomainDiskSourceDefFormatSeclabel(buf, src->nseclabels, + src->seclabels, flags); break; case VIR_STORAGE_TYPE_DIR: virBufferAddLit(buf, "<source"); - virBufferEscapeString(buf, " dir='%s'", src); + virBufferEscapeString(buf, " dir='%s'", src->path); virBufferEscapeString(buf, " startupPolicy='%s'", startupPolicy); virBufferAddLit(buf, "/>\n"); break; case VIR_STORAGE_TYPE_NETWORK: virBufferAsprintf(buf, "<source protocol='%s'", - virStorageNetProtocolTypeToString(protocol)); - virBufferEscapeString(buf, " name='%s'", src); + virStorageNetProtocolTypeToString(src->protocol)); + virBufferEscapeString(buf, " name='%s'", src->path); - if (nhosts == 0) { + if (src->nhosts == 0) { virBufferAddLit(buf, "/>\n"); } else { virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); - for (n = 0; n < nhosts; n++) { + for (n = 0; n < src->nhosts; n++) { virBufferAddLit(buf, "<host"); - virBufferEscapeString(buf, " name='%s'", hosts[n].name); - virBufferEscapeString(buf, " port='%s'", hosts[n].port); + virBufferEscapeString(buf, " name='%s'", + src->hosts[n].name); + virBufferEscapeString(buf, " port='%s'", + src->hosts[n].port); - if (hosts[n].transport) + if (src->hosts[n].transport) virBufferAsprintf(buf, " transport='%s'", - virStorageNetHostTransportTypeToString(hosts[n].transport)); + virStorageNetHostTransportTypeToString(src->hosts[n].transport)); - virBufferEscapeString(buf, " socket='%s'", hosts[n].socket); + virBufferEscapeString(buf, " socket='%s'", + src->hosts[n].socket); virBufferAddLit(buf, "/>\n"); } @@ -14768,22 +14754,23 @@ virDomainDiskSourceDefFormatInternal(virBufferPtr buf, case VIR_STORAGE_TYPE_VOLUME: virBufferAddLit(buf, "<source"); - if (srcpool) { - virBufferAsprintf(buf, " pool='%s' volume='%s'", - srcpool->pool, srcpool->volume); - if (srcpool->mode) + if (src->srcpool) { + virBufferEscapeString(buf, " pool='%s'", src->srcpool->pool); + virBufferEscapeString(buf, " volume='%s'", + src->srcpool->volume); + if (src->srcpool->mode) virBufferAsprintf(buf, " mode='%s'", - virStorageSourcePoolModeTypeToString(srcpool->mode)); + virStorageSourcePoolModeTypeToString(src->srcpool->mode)); } virBufferEscapeString(buf, " startupPolicy='%s'", startupPolicy); - virDomainDiskSourceDefFormatSeclabel(buf, nseclabels, seclabels, flags); + virDomainDiskSourceDefFormatSeclabel(buf, src->nseclabels, + src->seclabels, flags); break; - default: + case VIR_STORAGE_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected disk type %s"), - virStorageTypeToString(type)); + _("unexpected disk type %d"), src->type); return -1; } } @@ -14793,25 +14780,6 @@ virDomainDiskSourceDefFormatInternal(virBufferPtr buf, static int -virDomainDiskSourceDefFormat(virBufferPtr buf, - virDomainDiskDefPtr def, - unsigned int flags) -{ - return virDomainDiskSourceDefFormatInternal(buf, - def->src.type, - def->src.path, - def->startupPolicy, - def->src.protocol, - def->src.nhosts, - def->src.hosts, - def->src.nseclabels, - def->src.seclabels, - def->src.srcpool, - flags); -} - - -static int virDomainDiskDefFormat(virBufferPtr buf, virDomainDiskDefPtr def, unsigned int flags) @@ -14932,7 +14900,8 @@ virDomainDiskDefFormat(virBufferPtr buf, virBufferAddLit(buf, "</auth>\n"); } - if (virDomainDiskSourceDefFormat(buf, def, flags) < 0) + if (virDomainDiskSourceFormat(buf, &def->src, def->startupPolicy, + flags) < 0) return -1; virDomainDiskGeometryDefFormat(buf, def); virDomainDiskBlockIoDefFormat(buf, def); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 02ac5de..2f874c5 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2271,17 +2271,10 @@ int virDomainDefFormatInternal(virDomainDefPtr def, unsigned int flags, virBufferPtr buf); -int virDomainDiskSourceDefFormatInternal(virBufferPtr buf, - int type, - const char *src, - int policy, - int protocol, - size_t nhosts, - virStorageNetHostDefPtr hosts, - size_t nseclabels, - virSecurityDeviceLabelDefPtr *seclabels, - virStorageSourcePoolDefPtr srcpool, - unsigned int flags); +int virDomainDiskSourceFormat(virBufferPtr buf, + virStorageSourcePtr src, + int policy, + unsigned int flags); int virDomainNetDefFormat(virBufferPtr buf, virDomainNetDefPtr def, @@ -2328,13 +2321,8 @@ virDomainDiskDefPtr virDomainDiskRemove(virDomainDefPtr def, size_t i); virDomainDiskDefPtr virDomainDiskRemoveByName(virDomainDefPtr def, const char *name); -int virDomainDiskSourceDefParse(xmlNodePtr node, - int type, - char **source, - int *proto, - size_t *nhosts, - virStorageNetHostDefPtr *hosts, - virStorageSourcePoolDefPtr *srcpool); +int virDomainDiskSourceParse(xmlNodePtr node, + virStorageSourcePtr src); bool virDomainHasDiskMirror(virDomainObjPtr vm); diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 3bd4732..374a104 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -150,13 +150,7 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, if (!def->src.path && xmlStrEqual(cur->name, BAD_CAST "source")) { - if (virDomainDiskSourceDefParse(cur, - def->src.type, - &def->src.path, - &def->src.protocol, - &def->src.nhosts, - &def->src.hosts, - NULL) < 0) + if (virDomainDiskSourceParse(cur, &def->src) < 0) goto cleanup; } else if (!def->src.format && @@ -635,14 +629,7 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf, if (disk->src.format > 0) virBufferEscapeString(buf, "<driver type='%s'/>\n", virStorageFileFormatTypeToString(disk->src.format)); - virDomainDiskSourceDefFormatInternal(buf, - type, - disk->src.path, - 0, - disk->src.protocol, - disk->src.nhosts, - disk->src.hosts, - 0, NULL, NULL, 0); + virDomainDiskSourceFormat(buf, &disk->src, 0, 0); virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</disk>\n"); -- 1.9.0

On 04/02/14 05:04, Eric Blake wrote:
Now that we have a dedicated type for representing a disk source, we might as well parse and format directly into that type instead of piecemeal into pointers to members of the type.
* src/conf/domain_conf.h (virDomainDiskSourceDefFormatInternal) (virDomainDiskSourceDefParse): Rename... (virDomainDiskSourceFormat, virDomainDiskSourceParse): ...and compress signatures. * src/conf/domain_conf.c (virDomainDiskSourceParse) (virDomainDiskSourceFormat): Rewrite to use common struct. (virDomainDiskSourceDefFormat): Delete. (virDomainDiskDefParseXML, virDomainDiskDefFormat): Update callers. * src/conf/snapshot_conf.c (virDomainSnapshotDiskDefParseXML) (virDomainSnapshotDiskDefFormat): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.c | 139 ++++++++++++++++++----------------------------- src/conf/domain_conf.h | 24 ++------ src/conf/snapshot_conf.c | 17 +----- 3 files changed, 62 insertions(+), 118 deletions(-)
ACK, this fixes my suggestion in one of the previous patches Peter

Some preparatory work before consolidating storage volume structs with the rest of virstoragefile. Making these changes allows a volume target to be much closer to (a subset of) the virStorageSource struct. Making perms be a pointer allows it to be optional if we have a storage pool that doesn't expose permissions in a way we can access. It also allows future patches to optionally expose permissions details learned about a disk image via domain <disk> listings, rather than just limiting it to storage volume listings. Disk partition types was only used by internal code to control what type of partition to create when carving up an MS-DOS partition table storage pool (and is not used for GPT partition tables or other storage pools). It was not exposed in volume XML, and as it is more closely related to extent information of the overall block device than it is to the <target> information describing the host file. Besides, if we ever decide to expose it in XML down the road, we can move it back as needed. * src/conf/storage_conf.h (_virStorageVolTarget): Change perms to pointer, enhance comments. Move partition type... (_virStorageVolSource): ...here. * src/conf/storage_conf.c (virStorageVolDefFree) (virStorageVolDefParseXML, virStorageVolTargetDefFormat): Update clients. * src/storage/storage_backend_fs.c (createFileDir): Likewise. * src/storage/storage_backend.c (virStorageBackendCreateBlockFrom) (virStorageBackendCreateRaw, virStorageBackendCreateExecCommand) (virStorageBackendUpdateVolTargetInfoFD): Likewise. * src/storage/storage_backend_logical.c (virStorageBackendLogicalCreateVol): Likewise. * src/storage/storage_backend_disk.c (virStorageBackendDiskMakeDataVol) (virStorageBackendDiskPartTypeToCreate): Likewise. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/storage_conf.c | 26 ++++++++++++------ src/conf/storage_conf.h | 9 ++++--- src/storage/storage_backend.c | 50 ++++++++++++++++++++--------------- src/storage/storage_backend_disk.c | 18 ++++++------- src/storage/storage_backend_fs.c | 6 ++--- src/storage/storage_backend_logical.c | 6 ++--- 6 files changed, 67 insertions(+), 48 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 65504b4..e4986e6 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -332,11 +332,17 @@ virStorageVolDefFree(virStorageVolDefPtr def) VIR_FREE(def->target.compat); virBitmapFree(def->target.features); VIR_FREE(def->target.path); - VIR_FREE(def->target.perms.label); + if (def->target.perms) { + VIR_FREE(def->target.perms->label); + VIR_FREE(def->target.perms); + } VIR_FREE(def->target.timestamps); virStorageEncryptionFree(def->target.encryption); VIR_FREE(def->backingStore.path); - VIR_FREE(def->backingStore.perms.label); + if (def->backingStore.perms) { + VIR_FREE(def->backingStore.perms->label); + VIR_FREE(def->backingStore.perms); + } VIR_FREE(def->backingStore.timestamps); virStorageEncryptionFree(def->backingStore.encryption); VIR_FREE(def); @@ -1355,7 +1361,9 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, VIR_FREE(format); } - if (virStorageDefParsePerms(ctxt, &ret->target.perms, + if (VIR_ALLOC(ret->target.perms) < 0) + goto error; + if (virStorageDefParsePerms(ctxt, ret->target.perms, "./target/permissions", DEFAULT_VOL_PERM_MODE) < 0) goto error; @@ -1424,7 +1432,9 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, VIR_FREE(nodes); } - if (virStorageDefParsePerms(ctxt, &ret->backingStore.perms, + if (VIR_ALLOC(ret->backingStore.perms) < 0) + goto error; + if (virStorageDefParsePerms(ctxt, ret->backingStore.perms, "./backingStore/permissions", DEFAULT_VOL_PERM_MODE) < 0) goto error; @@ -1541,15 +1551,15 @@ virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, virBufferAdjustIndent(buf, 2); virBufferAsprintf(buf, "<mode>0%o</mode>\n", - def->perms.mode); + def->perms->mode); virBufferAsprintf(buf, "<owner>%u</owner>\n", - (unsigned int) def->perms.uid); + (unsigned int) def->perms->uid); virBufferAsprintf(buf, "<group>%u</group>\n", - (unsigned int) def->perms.gid); + (unsigned int) def->perms->gid); virBufferEscapeString(buf, "<label>%s</label>\n", - def->perms.label); + def->perms->label); virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</permissions>\n"); diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index b811046..abff7ec 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -71,6 +71,9 @@ typedef virStorageVolSource *virStorageVolSourcePtr; struct _virStorageVolSource { int nextent; virStorageVolSourceExtentPtr extents; + + int partType; /* enum virStorageVolTypeDisk, only used by disk + * backend for partition type creation */ }; @@ -81,10 +84,10 @@ typedef struct _virStorageVolTarget virStorageVolTarget; typedef virStorageVolTarget *virStorageVolTargetPtr; struct _virStorageVolTarget { char *path; - int format; - virStoragePerms perms; + int format; /* enum virStorageFileFormat */ + virStoragePermsPtr perms; virStorageTimestampsPtr timestamps; - int type; /* only used by disk backend for partition type */ + /* The next three are currently only used in vol->target, * not in vol->backingStore. */ virStorageEncryptionPtr encryption; diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index eedd11b..c21504d 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -301,8 +301,10 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn ATTRIBUTE_UNUSED, vol->target.path); goto cleanup; } - uid = (vol->target.perms.uid != st.st_uid) ? vol->target.perms.uid : (uid_t) -1; - gid = (vol->target.perms.gid != st.st_gid) ? vol->target.perms.gid : (gid_t) -1; + uid = (vol->target.perms->uid != st.st_uid) ? vol->target.perms->uid + : (uid_t) -1; + gid = (vol->target.perms->gid != st.st_gid) ? vol->target.perms->gid + : (gid_t) -1; if (((uid != (uid_t) -1) || (gid != (gid_t) -1)) && (fchown(fd, uid, gid) < 0)) { virReportSystemError(errno, @@ -311,10 +313,10 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn ATTRIBUTE_UNUSED, (unsigned int) gid); goto cleanup; } - if (fchmod(fd, vol->target.perms.mode) < 0) { + if (fchmod(fd, vol->target.perms->mode) < 0) { virReportSystemError(errno, _("cannot set mode of '%s' to %04o"), - vol->target.path, vol->target.perms.mode); + vol->target.path, vol->target.perms->mode); goto cleanup; } if (VIR_CLOSE(fd) < 0) { @@ -439,9 +441,9 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, if ((fd = virFileOpenAs(vol->target.path, O_RDWR | O_CREAT | O_EXCL, - vol->target.perms.mode, - vol->target.perms.uid, - vol->target.perms.gid, + vol->target.perms->mode, + vol->target.perms->uid, + vol->target.perms->gid, operation_flags)) < 0) { virReportSystemError(-fd, _("Failed to create file '%s'"), @@ -578,13 +580,13 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, if ((pool->def->type == VIR_STORAGE_POOL_NETFS) && (((geteuid() == 0) - && (vol->target.perms.uid != (uid_t) -1) - && (vol->target.perms.uid != 0)) - || ((vol->target.perms.gid != (gid_t) -1) - && (vol->target.perms.gid != getegid())))) { + && (vol->target.perms->uid != (uid_t) -1) + && (vol->target.perms->uid != 0)) + || ((vol->target.perms->gid != (gid_t) -1) + && (vol->target.perms->gid != getegid())))) { - virCommandSetUID(cmd, vol->target.perms.uid); - virCommandSetGID(cmd, vol->target.perms.gid); + virCommandSetUID(cmd, vol->target.perms->uid); + virCommandSetGID(cmd, vol->target.perms->gid); if (virCommandRun(cmd, NULL) == 0) { /* command was successfully run, check if the file was created */ @@ -608,8 +610,10 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, } } - uid = (vol->target.perms.uid != st.st_uid) ? vol->target.perms.uid : (uid_t) -1; - gid = (vol->target.perms.gid != st.st_gid) ? vol->target.perms.gid : (gid_t) -1; + uid = (vol->target.perms->uid != st.st_uid) ? vol->target.perms->uid + : (uid_t) -1; + gid = (vol->target.perms->gid != st.st_gid) ? vol->target.perms->gid + : (gid_t) -1; if (((uid != (uid_t) -1) || (gid != (gid_t) -1)) && (chown(vol->target.path, uid, gid) < 0)) { virReportSystemError(errno, @@ -618,10 +622,10 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, (unsigned int) gid); return -1; } - if (chmod(vol->target.path, vol->target.perms.mode) < 0) { + if (chmod(vol->target.path, vol->target.perms->mode) < 0) { virReportSystemError(errno, _("cannot set mode of '%s' to %04o"), - vol->target.path, vol->target.perms.mode); + vol->target.path, vol->target.perms->mode); return -1; } return 0; @@ -1495,9 +1499,11 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, } } - target->perms.mode = sb->st_mode & S_IRWXUGO; - target->perms.uid = sb->st_uid; - target->perms.gid = sb->st_gid; + if (!target->perms && VIR_ALLOC(target->perms) < 0) + return -1; + target->perms->mode = sb->st_mode & S_IRWXUGO; + target->perms->uid = sb->st_uid; + target->perms->gid = sb->st_gid; if (!target->timestamps && VIR_ALLOC(target->timestamps) < 0) return -1; @@ -1506,7 +1512,7 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, target->timestamps->ctime = get_stat_ctime(sb); target->timestamps->mtime = get_stat_mtime(sb); - VIR_FREE(target->perms.label); + VIR_FREE(target->perms->label); #if WITH_SELINUX /* XXX: make this a security driver call */ @@ -1519,7 +1525,7 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, return -1; } } else { - if (VIR_STRDUP(target->perms.label, filecon) < 0) { + if (VIR_STRDUP(target->perms->label, filecon) < 0) { freecon(filecon); return -1; } diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index fb7a2a4..01f1b17 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -119,13 +119,13 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, /* set partition type */ if (STREQ(groups[1], "normal")) - vol->target.type = VIR_STORAGE_VOL_DISK_TYPE_PRIMARY; + vol->source.partType = VIR_STORAGE_VOL_DISK_TYPE_PRIMARY; else if (STREQ(groups[1], "logical")) - vol->target.type = VIR_STORAGE_VOL_DISK_TYPE_LOGICAL; + vol->source.partType = VIR_STORAGE_VOL_DISK_TYPE_LOGICAL; else if (STREQ(groups[1], "extended")) - vol->target.type = VIR_STORAGE_VOL_DISK_TYPE_EXTENDED; + vol->source.partType = VIR_STORAGE_VOL_DISK_TYPE_EXTENDED; else - vol->target.type = VIR_STORAGE_VOL_DISK_TYPE_NONE; + vol->source.partType = VIR_STORAGE_VOL_DISK_TYPE_NONE; vol->type = VIR_STORAGE_VOL_BLOCK; @@ -445,10 +445,10 @@ virStorageBackendDiskPartTypeToCreate(virStoragePoolObjPtr pool) size_t i; int count = 0; for (i = 0; i < pool->volumes.count; i++) { - if (pool->volumes.objs[i]->target.type == VIR_STORAGE_VOL_DISK_TYPE_PRIMARY || - pool->volumes.objs[i]->target.type == VIR_STORAGE_VOL_DISK_TYPE_EXTENDED) { - count++; - } + int partType = pool->volumes.objs[i]->source.partType; + if (partType == VIR_STORAGE_VOL_DISK_TYPE_PRIMARY || + partType == VIR_STORAGE_VOL_DISK_TYPE_EXTENDED) + count++; } if (count >= 4) { return VIR_STORAGE_VOL_DISK_TYPE_LOGICAL; @@ -614,7 +614,7 @@ virStorageBackendDiskPartBoundaries(virStoragePoolObjPtr pool, *end -= (*start % cylinderSize); } - /* counting in byte, we want the last byte of the current sector */ + /* counting in bytes, we want the last byte of the current sector */ *end -= 1; VIR_DEBUG("final aligned start %llu, end %llu", *start, *end); return 0; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index be0659a..b361804 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1051,9 +1051,9 @@ static int createFileDir(virConnectPtr conn ATTRIBUTE_UNUSED, return -1; } - if ((err = virDirCreate(vol->target.path, vol->target.perms.mode, - vol->target.perms.uid, - vol->target.perms.gid, + if ((err = virDirCreate(vol->target.path, vol->target.perms->mode, + vol->target.perms->uid, + vol->target.perms->gid, VIR_DIR_CREATE_FORCE_PERMS | (pool->def->type == VIR_STORAGE_POOL_NETFS ? VIR_DIR_CREATE_AS_UID : 0))) < 0) { diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 7893626..aea624e 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -1,7 +1,7 @@ /* * storage_backend_logical.c: storage backend for logical volume handling * - * Copyright (C) 2007-2009, 2011, 2013 Red Hat, Inc. + * Copyright (C) 2007-2014 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -767,14 +767,14 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, /* We can only chown/grp if root */ if (geteuid() == 0) { - if (fchown(fd, vol->target.perms.uid, vol->target.perms.gid) < 0) { + if (fchown(fd, vol->target.perms->uid, vol->target.perms->gid) < 0) { virReportSystemError(errno, _("cannot set file owner '%s'"), vol->target.path); goto error; } } - if (fchmod(fd, vol->target.perms.mode) < 0) { + if (fchmod(fd, vol->target.perms->mode) < 0) { virReportSystemError(errno, _("cannot set file mode '%s'"), vol->target.path); -- 1.9.0

On 04/02/14 05:04, Eric Blake wrote:
Some preparatory work before consolidating storage volume structs with the rest of virstoragefile. Making these changes allows a volume target to be much closer to (a subset of) the virStorageSource struct.
Making perms be a pointer allows it to be optional if we have a storage pool that doesn't expose permissions in a way we can access. It also allows future patches to optionally expose permissions details learned about a disk image via domain <disk> listings, rather than just limiting it to storage volume listings.
Disk partition types was only used by internal code to control what type of partition to create when carving up an MS-DOS partition table storage pool (and is not used for GPT partition tables or other storage pools). It was not exposed in volume XML, and as it is more closely related to extent information of the overall block device than it is to the <target> information describing the host file. Besides, if we ever decide to expose it in XML down the road, we can move it back as needed.
* src/conf/storage_conf.h (_virStorageVolTarget): Change perms to pointer, enhance comments. Move partition type... (_virStorageVolSource): ...here. * src/conf/storage_conf.c (virStorageVolDefFree) (virStorageVolDefParseXML, virStorageVolTargetDefFormat): Update clients. * src/storage/storage_backend_fs.c (createFileDir): Likewise. * src/storage/storage_backend.c (virStorageBackendCreateBlockFrom) (virStorageBackendCreateRaw, virStorageBackendCreateExecCommand) (virStorageBackendUpdateVolTargetInfoFD): Likewise. * src/storage/storage_backend_logical.c (virStorageBackendLogicalCreateVol): Likewise. * src/storage/storage_backend_disk.c (virStorageBackendDiskMakeDataVol) (virStorageBackendDiskPartTypeToCreate): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/storage_conf.c | 26 ++++++++++++------ src/conf/storage_conf.h | 9 ++++--- src/storage/storage_backend.c | 50 ++++++++++++++++++++--------------- src/storage/storage_backend_disk.c | 18 ++++++------- src/storage/storage_backend_fs.c | 6 ++--- src/storage/storage_backend_logical.c | 6 ++--- 6 files changed, 67 insertions(+), 48 deletions(-)
...
diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index fb7a2a4..01f1b17 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -614,7 +614,7 @@ virStorageBackendDiskPartBoundaries(virStoragePoolObjPtr pool, *end -= (*start % cylinderSize); }
- /* counting in byte, we want the last byte of the current sector */ + /* counting in bytes, we want the last byte of the current sector */
Unrelated typo fix?
*end -= 1; VIR_DEBUG("final aligned start %llu, end %llu", *start, *end); return 0;
ACK Peter

On 04/02/2014 03:16 AM, Peter Krempa wrote:
On 04/02/14 05:04, Eric Blake wrote:
Some preparatory work before consolidating storage volume structs with the rest of virstoragefile. Making these changes allows a volume target to be much closer to (a subset of) the virStorageSource struct.
+++ b/src/storage/storage_backend_disk.c @@ -614,7 +614,7 @@ virStorageBackendDiskPartBoundaries(virStoragePoolObjPtr pool, *end -= (*start % cylinderSize); }
- /* counting in byte, we want the last byte of the current sector */ + /* counting in bytes, we want the last byte of the current sector */
Unrelated typo fix?
Yeah. I thought I factored all those out into an earlier trivial patch (commit 17f8263) - but missed this one. But it's not completely unrelated, as it is the code dealing with partition manipulation, and I found it while making sure that moving the disk partition type was an internal-only variable that I could freely move between structs.
*end -= 1; VIR_DEBUG("final aligned start %llu, end %llu", *start, *end); return 0;
ACK
I've pushed 1-7, and am back to hacking on my RFC 8 and 9 to turn it into something that actually matches my proposals. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Another step towards unification of structures. While we might not expose everything in XML via domain disk as we do for storage volume pointer, both places want to deal with (at least part of) the backing chain; therefore, moving towards a single struct usable from both contexts will make the backing chain code more reusable. * src/conf/storage_conf.h (_virStoragePerms) (virStorageTimestamps): Move... * src/util/virstoragefile.h: ...here. (_virStorageSource): Add more fields. * src/util/virstoragefile.c (virStorageSourceClear): Clean additional fields. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/storage_conf.h | 23 +---------------------- src/util/virstoragefile.c | 7 +++++++ src/util/virstoragefile.h | 25 +++++++++++++++++++++++++ 3 files changed, 33 insertions(+), 22 deletions(-) diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index abff7ec..507f08b 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -26,33 +26,12 @@ # include "internal.h" # include "virstorageencryption.h" +# include "virstoragefile.h" # include "virbitmap.h" # include "virthread.h" # include <libxml/tree.h> -typedef struct _virStoragePerms virStoragePerms; -typedef virStoragePerms *virStoragePermsPtr; -struct _virStoragePerms { - mode_t mode; - uid_t uid; - gid_t gid; - char *label; -}; - -typedef struct _virStorageTimestamps virStorageTimestamps; -typedef virStorageTimestamps *virStorageTimestampsPtr; -struct _virStorageTimestamps { - struct timespec atime; - /* if btime.tv_nsec == -1 then - * birth time is unknown - */ - struct timespec btime; - struct timespec ctime; - struct timespec mtime; -}; - - /* * How the volume's data is stored on underlying * physical devices - can potentially span many diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index f6146d8..137bacc 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1569,6 +1569,8 @@ virStorageSourceClear(virStorageSourcePtr def) VIR_FREE(def->path); virStorageSourcePoolDefFree(def->srcpool); VIR_FREE(def->driverName); + virBitmapFree(def->features); + VIR_FREE(def->compat); virStorageEncryptionFree(def->encryption); if (def->seclabels) { @@ -1576,6 +1578,11 @@ virStorageSourceClear(virStorageSourcePtr def) virSecurityDeviceLabelDefFree(def->seclabels[i]); VIR_FREE(def->seclabels); } + if (def->perms) { + VIR_FREE(def->perms->label); + VIR_FREE(def->perms); + } + VIR_FREE(def->timestamps); virStorageNetHostDefFree(def->nhosts, def->hosts); virStorageSourceAuthClear(def); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 4980960..00d5456 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -85,6 +85,26 @@ enum virStorageFileFeature { VIR_ENUM_DECL(virStorageFileFeature); +typedef struct _virStoragePerms virStoragePerms; +typedef virStoragePerms *virStoragePermsPtr; +struct _virStoragePerms { + mode_t mode; + uid_t uid; + gid_t gid; + char *label; +}; + + +typedef struct _virStorageTimestamps virStorageTimestamps; +typedef virStorageTimestamps *virStorageTimestampsPtr; +struct _virStorageTimestamps { + struct timespec atime; + struct timespec btime; /* birth time unknown if btime.tv_nsec == -1 */ + struct timespec ctime; + struct timespec mtime; +}; + + typedef struct _virStorageFileMetadata virStorageFileMetadata; typedef virStorageFileMetadata *virStorageFileMetadataPtr; struct _virStorageFileMetadata { @@ -206,9 +226,14 @@ struct _virStorageSource { } secret; } auth; virStorageEncryptionPtr encryption; + char *driverName; int format; /* enum virStorageFileFormat */ + virBitmapPtr features; + char *compat; + virStoragePermsPtr perms; + virStorageTimestampsPtr timestamps; size_t nseclabels; virSecurityDeviceLabelDefPtr *seclabels; }; -- 1.9.0

On 04/02/14 05:04, Eric Blake wrote:
Another step towards unification of structures. While we might not expose everything in XML via domain disk as we do for storage volume pointer, both places want to deal with (at least part of) the backing chain; therefore, moving towards a single struct usable from both contexts will make the backing chain code more reusable.
* src/conf/storage_conf.h (_virStoragePerms) (virStorageTimestamps): Move... * src/util/virstoragefile.h: ...here. (_virStorageSource): Add more fields. * src/util/virstoragefile.c (virStorageSourceClear): Clean additional fields.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/storage_conf.h | 23 +---------------------- src/util/virstoragefile.c | 7 +++++++ src/util/virstoragefile.h | 25 +++++++++++++++++++++++++ 3 files changed, 33 insertions(+), 22 deletions(-)
ACK Peter

A fairly smooth transition. And now that domain disks and storage volumes share a common struct, it opens the doors for a future patch to expose more details in the XML for both objects. * src/conf/storage_conf.h (_virStorageVolTarget): Delete. (_virStorageVolDef): Use common type. * src/conf/storage_conf.c (virStorageVolDefFree) (virStorageVolTargetDefFormat): Update clients. * src/storage/storage_backend.h: Likewise. * src/storage/storage_backend.c (virStorageBackendDetectBlockVolFormatFD) (virStorageBackendUpdateVolTargetInfo) (virStorageBackendUpdateVolTargetInfoFD): Likewise. * src/storage/storage_backend_fs.c (virStorageBackendProbeTarget): Likewise. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/storage_conf.c | 20 +++----------------- src/conf/storage_conf.h | 22 ++-------------------- src/storage/storage_backend.c | 6 +++--- src/storage/storage_backend.h | 4 ++-- src/storage/storage_backend_fs.c | 2 +- 5 files changed, 11 insertions(+), 43 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index e4986e6..a96f4d6 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -329,22 +329,8 @@ virStorageVolDefFree(virStorageVolDefPtr def) } VIR_FREE(def->source.extents); - VIR_FREE(def->target.compat); - virBitmapFree(def->target.features); - VIR_FREE(def->target.path); - if (def->target.perms) { - VIR_FREE(def->target.perms->label); - VIR_FREE(def->target.perms); - } - VIR_FREE(def->target.timestamps); - virStorageEncryptionFree(def->target.encryption); - VIR_FREE(def->backingStore.path); - if (def->backingStore.perms) { - VIR_FREE(def->backingStore.perms->label); - VIR_FREE(def->backingStore.perms); - } - VIR_FREE(def->backingStore.timestamps); - virStorageEncryptionFree(def->backingStore.encryption); + virStorageSourceClear(&def->target); + virStorageSourceClear(&def->backingStore); VIR_FREE(def); } @@ -1528,7 +1514,7 @@ virStorageVolTimestampFormat(virBufferPtr buf, const char *name, static int virStorageVolTargetDefFormat(virStorageVolOptionsPtr options, virBufferPtr buf, - virStorageVolTargetPtr def, + virStorageSourcePtr def, const char *type) { virBufferAsprintf(buf, "<%s>\n", type); diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 507f08b..66b5a3b 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -56,24 +56,6 @@ struct _virStorageVolSource { }; -/* - * How the volume appears on the host - */ -typedef struct _virStorageVolTarget virStorageVolTarget; -typedef virStorageVolTarget *virStorageVolTargetPtr; -struct _virStorageVolTarget { - char *path; - int format; /* enum virStorageFileFormat */ - virStoragePermsPtr perms; - virStorageTimestampsPtr timestamps; - - /* The next three are currently only used in vol->target, - * not in vol->backingStore. */ - virStorageEncryptionPtr encryption; - virBitmapPtr features; - char *compat; -}; - typedef struct _virStorageVolDef virStorageVolDef; typedef virStorageVolDef *virStorageVolDefPtr; struct _virStorageVolDef { @@ -87,8 +69,8 @@ struct _virStorageVolDef { unsigned long long capacity; /* bytes */ virStorageVolSource source; - virStorageVolTarget target; - virStorageVolTarget backingStore; + virStorageSource target; + virStorageSource backingStore; }; typedef struct _virStorageVolDefList virStorageVolDefList; diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index c21504d..9abaccd 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1236,7 +1236,7 @@ static struct diskType const disk_types[] = { static int -virStorageBackendDetectBlockVolFormatFD(virStorageVolTargetPtr target, +virStorageBackendDetectBlockVolFormatFD(virStorageSourcePtr target, int fd) { size_t i; @@ -1384,7 +1384,7 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, } int -virStorageBackendUpdateVolTargetInfo(virStorageVolTargetPtr target, +virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, unsigned long long *allocation, unsigned long long *capacity, bool withBlockVolFormat, @@ -1451,7 +1451,7 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, * Returns 0 for success, -1 on a legitimate error condition. */ int -virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, +virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, int fd, struct stat *sb, unsigned long long *allocation, diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index c0d1668..8442c13 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -141,12 +141,12 @@ int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, bool withCapacity, bool withBlockVolFormat, unsigned int openflags); -int virStorageBackendUpdateVolTargetInfo(virStorageVolTargetPtr target, +int virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, unsigned long long *allocation, unsigned long long *capacity, bool withBlockVolFormat, unsigned int openflags); -int virStorageBackendUpdateVolTargetInfoFD(virStorageVolTargetPtr target, +int virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, int fd, struct stat *sb, unsigned long long *allocation, diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index b361804..113593b 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -62,7 +62,7 @@ VIR_LOG_INIT("storage.storage_backend_fs"); ~VIR_STORAGE_VOL_OPEN_ERROR) static int ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) -virStorageBackendProbeTarget(virStorageVolTargetPtr target, +virStorageBackendProbeTarget(virStorageSourcePtr target, char **backingStore, int *backingStoreFormat, unsigned long long *allocation, -- 1.9.0

On 04/02/14 05:05, Eric Blake wrote:
A fairly smooth transition. And now that domain disks and storage volumes share a common struct, it opens the doors for a future patch to expose more details in the XML for both objects.
* src/conf/storage_conf.h (_virStorageVolTarget): Delete. (_virStorageVolDef): Use common type. * src/conf/storage_conf.c (virStorageVolDefFree) (virStorageVolTargetDefFormat): Update clients. * src/storage/storage_backend.h: Likewise. * src/storage/storage_backend.c (virStorageBackendDetectBlockVolFormatFD) (virStorageBackendUpdateVolTargetInfo) (virStorageBackendUpdateVolTargetInfoFD): Likewise. * src/storage/storage_backend_fs.c (virStorageBackendProbeTarget): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/storage_conf.c | 20 +++----------------- src/conf/storage_conf.h | 22 ++-------------------- src/storage/storage_backend.c | 6 +++--- src/storage/storage_backend.h | 4 ++-- src/storage/storage_backend_fs.c | 2 +- 5 files changed, 11 insertions(+), 43 deletions(-)
ACK, Peter

One of the features of qcow2 is that a wrapper file can have more capacity than its backing file from the guest's perspective; what's more, sparse files make tracking allocation of both the active and backing file worthwhile. As such, it makes more sense to show allocation numbers for each file in a chain, and not just the top-level file. This sets up the fields for the tracking, although it does not modify XML to display any new information. * src/util/virstoragefile.h (_virStorageSource): Add fields. * src/conf/storage_conf.h (_virStorageVolDef): Drop redundant fields. * src/storage/storage_backend.c (virStorageBackendCreateBlockFrom) (createRawFile, virStorageBackendCreateQemuImgCmd) (virStorageBackendCreateQcowCreate): Update clients. * src/storage/storage_driver.c (storageVolDelete) (storageVolCreateXML, storageVolCreateXMLFrom, storageVolResize) (storageVolWipeInternal, storageVolGetInfo): Likewise. * src/storage/storage_backend_fs.c (virStorageBackendProbeTarget) (virStorageBackendFileSystemRefresh) (virStorageBackendFileSystemVolResize) (virStorageBackendFileSystemVolRefresh): Likewise. * src/storage/storage_backend_logical.c (virStorageBackendLogicalMakeVol) (virStorageBackendLogicalCreateVol): Likewise. * src/storage/storage_backend_scsi.c (virStorageBackendSCSINewLun): Likewise. * src/storage/storage_backend_mpath.c (virStorageBackendMpathNewVol): Likewise. * src/storage/storage_backend_rbd.c (volStorageBackendRBDRefreshVolInfo) (virStorageBackendRBDCreateImage): Likewise. * src/storage/storage_backend_disk.c (virStorageBackendDiskMakeDataVol) (virStorageBackendDiskCreateVol): Likewise. * src/storage/storage_backend_sheepdog.c (virStorageBackendSheepdogBuildVol) (virStorageBackendSheepdogParseVdiList): Likewise. * src/storage/storage_backend_gluster.c (virStorageBackendGlusterRefreshVol): Likewise. * src/conf/storage_conf.c (virStorageVolDefFormat) (virStorageVolDefParseXML): Likewise. * src/test/test_driver.c (testOpenVolumesForPool) (testStorageVolCreateXML, testStorageVolCreateXMLFrom) (testStorageVolDelete, testStorageVolGetInfo): Likewise. * src/esx/esx_storage_backend_iscsi.c (esxStorageVolGetXMLDesc): Likewise. * src/esx/esx_storage_backend_vmfs.c (esxStorageVolGetXMLDesc) (esxStorageVolCreateXML): Likewise. * src/parallels/parallels_driver.c (parallelsAddHddByVolume): Likewise. * src/parallels/parallels_storage.c (parallelsDiskDescParseNode) (parallelsStorageVolDefineXML, parallelsStorageVolCreateXMLFrom) (parallelsStorageVolDefRemove, parallelsStorageVolGetInfo): Likewise. * src/vbox/vbox_tmpl.c (vboxStorageVolCreateXML) (vboxStorageVolGetXMLDesc): Likewise. * tests/storagebackendsheepdogtest.c (test_vdi_list_parser): Likewise. * src/phyp/phyp_driver.c (phypStorageVolCreateXML): Likewise. --- src/conf/storage_conf.c | 10 ++++----- src/conf/storage_conf.h | 3 --- src/esx/esx_storage_backend_iscsi.c | 5 +++-- src/esx/esx_storage_backend_vmfs.c | 20 ++++++++--------- src/parallels/parallels_driver.c | 2 +- src/parallels/parallels_storage.c | 22 +++++++++---------- src/phyp/phyp_driver.c | 4 ++-- src/storage/storage_backend.c | 24 ++++++++++---------- src/storage/storage_backend_disk.c | 6 ++--- src/storage/storage_backend_fs.c | 6 ++--- src/storage/storage_backend_gluster.c | 6 ++--- src/storage/storage_backend_logical.c | 10 +++++---- src/storage/storage_backend_mpath.c | 6 ++--- src/storage/storage_backend_rbd.c | 11 +++++----- src/storage/storage_backend_scsi.c | 4 ++-- src/storage/storage_backend_sheepdog.c | 10 ++++----- src/storage/storage_driver.c | 40 +++++++++++++++++----------------- src/test/test_driver.c | 16 +++++++------- src/util/virstoragefile.h | 2 ++ src/vbox/vbox_tmpl.c | 11 +++++----- tests/storagebackendsheepdogtest.c | 5 +++-- 21 files changed, 114 insertions(+), 109 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index a96f4d6..ebd42c2 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1317,17 +1317,17 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool, _("missing capacity element")); goto error; } - if (virStorageSize(unit, capacity, &ret->capacity) < 0) + if (virStorageSize(unit, capacity, &ret->target.capacity) < 0) goto error; VIR_FREE(unit); allocation = virXPathString("string(./allocation)", ctxt); if (allocation) { unit = virXPathString("string(./allocation/@unit)", ctxt); - if (virStorageSize(unit, allocation, &ret->allocation) < 0) + if (virStorageSize(unit, allocation, &ret->target.allocation) < 0) goto error; } else { - ret->allocation = ret->capacity; + ret->target.allocation = ret->target.capacity; } ret->target.path = virXPathString("string(./target/path)", ctxt); @@ -1644,9 +1644,9 @@ virStorageVolDefFormat(virStoragePoolDefPtr pool, virBufferAddLit(&buf, "</source>\n"); virBufferAsprintf(&buf, "<capacity unit='bytes'>%llu</capacity>\n", - def->capacity); + def->target.capacity); virBufferAsprintf(&buf, "<allocation unit='bytes'>%llu</allocation>\n", - def->allocation); + def->target.allocation); if (virStorageVolTargetDefFormat(options, &buf, &def->target, "target") < 0) diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 66b5a3b..9ad38e1 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -65,9 +65,6 @@ struct _virStorageVolDef { unsigned int building; - unsigned long long allocation; /* bytes */ - unsigned long long capacity; /* bytes */ - virStorageVolSource source; virStorageSource target; virStorageSource backingStore; diff --git a/src/esx/esx_storage_backend_iscsi.c b/src/esx/esx_storage_backend_iscsi.c index bd9f3b4..5509429 100644 --- a/src/esx/esx_storage_backend_iscsi.c +++ b/src/esx/esx_storage_backend_iscsi.c @@ -1,6 +1,7 @@ /* * esx_storage_backend_iscsi.c: ESX storage backend for iSCSI handling * + * Copyright (C) 2014 Red Hat, Inc. * Copyright (C) 2012 Ata E Husain Bohra <ata.husain@hotmail.com> * * This library is free software; you can redistribute it and/or @@ -677,10 +678,10 @@ esxStorageVolGetXMLDesc(virStorageVolPtr volume, def.target.path = hostScsiDisk->devicePath; - def.capacity = hostScsiDisk->capacity->block->value * + def.target.capacity = hostScsiDisk->capacity->block->value * hostScsiDisk->capacity->blockSize->value; - def.allocation = def.capacity; + def.target.allocation = def.target.capacity; /* iSCSI LUN(s) hosting a datastore will be auto-mounted by ESX host */ def.target.format = VIR_STORAGE_FILE_RAW; diff --git a/src/esx/esx_storage_backend_vmfs.c b/src/esx/esx_storage_backend_vmfs.c index db4d480..6bed3ce 100644 --- a/src/esx/esx_storage_backend_vmfs.c +++ b/src/esx/esx_storage_backend_vmfs.c @@ -2,7 +2,7 @@ * esx_storage_backend_vmfs.c: ESX storage driver backend for * managing VMFS datastores * - * Copyright (C) 2010-2011, 2013 Red Hat, Inc. + * Copyright (C) 2010-2014 Red Hat, Inc. * Copyright (C) 2010-2012 Matthias Bolte <matthias.bolte@googlemail.com> * Copyright (C) 2012 Ata E Husain Bohra <ata.husain@hotmail.com> * @@ -954,13 +954,13 @@ esxStorageVolCreateXML(virStoragePoolPtr pool, } /* From the vSphere API documentation about VirtualDiskType ... */ - if (def->allocation == def->capacity) { + if (def->target.allocation == def->target.capacity) { /* * "A preallocated disk has all space allocated at creation time * and the space is zeroed on demand as the space is used." */ virtualDiskSpec->diskType = (char *)"preallocated"; - } else if (def->allocation == 0) { + } else if (def->target.allocation == 0) { /* * "Space required for thin-provisioned virtual disk is allocated * and zeroed on demand as the space is used." @@ -980,7 +980,7 @@ esxStorageVolCreateXML(virStoragePoolPtr pool, virtualDiskSpec->adapterType = (char *)"busLogic"; virtualDiskSpec->capacityKb->value = - VIR_DIV_UP(def->capacity, 1024); /* Scale from byte to kilobyte */ + VIR_DIV_UP(def->target.capacity, 1024); /* Scale from byte to kilobyte */ if (esxVI_CreateVirtualDisk_Task (priv->primary, datastorePath, @@ -1424,18 +1424,18 @@ esxStorageVolGetXMLDesc(virStorageVolPtr volume, if (vmDiskFileInfo) { /* Scale from kilobyte to byte */ - def.capacity = vmDiskFileInfo->capacityKb->value * 1024; - def.allocation = vmDiskFileInfo->fileSize->value; + def.target.capacity = vmDiskFileInfo->capacityKb->value * 1024; + def.target.allocation = vmDiskFileInfo->fileSize->value; def.target.format = VIR_STORAGE_FILE_VMDK; } else if (isoImageFileInfo) { - def.capacity = fileInfo->fileSize->value; - def.allocation = fileInfo->fileSize->value; + def.target.capacity = fileInfo->fileSize->value; + def.target.allocation = fileInfo->fileSize->value; def.target.format = VIR_STORAGE_FILE_ISO; } else if (floppyImageFileInfo) { - def.capacity = fileInfo->fileSize->value; - def.allocation = fileInfo->fileSize->value; + def.target.capacity = fileInfo->fileSize->value; + def.target.allocation = fileInfo->fileSize->value; def.target.format = VIR_STORAGE_FILE_RAW; } else { diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index a0d5f8c..848ed9f 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -1613,7 +1613,7 @@ static int parallelsAddHddByVolume(parallelsDomObjPtr pdom, virCommandPtr cmd = virCommandNewArgList(PRLCTL, "set", pdom->uuid, "--device-add", "hdd", NULL); - virCommandAddArgFormat(cmd, "--size=%lluM", voldef->capacity >> 20); + virCommandAddArgFormat(cmd, "--size=%lluM", voldef->target.capacity >> 20); if (!(strbus = parallelsGetDiskBusName(disk->bus))) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, diff --git a/src/parallels/parallels_storage.c b/src/parallels/parallels_storage.c index e1579d2..736d060 100644 --- a/src/parallels/parallels_storage.c +++ b/src/parallels/parallels_storage.c @@ -2,7 +2,7 @@ * parallels_storage.c: core driver functions for managing * Parallels Cloud Server hosts * - * Copyright (C) 2013 Red Hat, Inc. + * Copyright (C) 2013-2014 Red Hat, Inc. * Copyright (C) 2012 Parallels, Inc. * * This library is free software; you can redistribute it and/or @@ -257,15 +257,15 @@ static int parallelsDiskDescParseNode(xmlDocPtr xml, ctxt->node = root; if (virXPathULongLong("string(./Disk_Parameters/Disk_size)", - ctxt, &def->capacity) < 0) { + ctxt, &def->target.capacity) < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("failed to get disk size from " "the disk descriptor xml")); goto cleanup; } - def->capacity <<= 9; - def->allocation = def->capacity; + def->target.capacity <<= 9; + def->target.allocation = def->target.capacity; ret = 0; cleanup: xmlXPathFreeContext(ctxt); @@ -1218,7 +1218,7 @@ parallelsStorageVolDefineXML(virStoragePoolObjPtr pool, if (is_new) { /* Make sure enough space */ - if ((pool->def->allocation + privvol->allocation) > + if ((pool->def->allocation + privvol->target.allocation) > pool->def->capacity) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Not enough free space in pool for volume '%s'"), @@ -1245,7 +1245,7 @@ parallelsStorageVolDefineXML(virStoragePoolObjPtr pool, goto cleanup; } - pool->def->allocation += privvol->allocation; + pool->def->allocation += privvol->target.allocation; pool->def->available = (pool->def->capacity - pool->def->allocation); } @@ -1349,7 +1349,7 @@ parallelsStorageVolCreateXMLFrom(virStoragePoolPtr pool, } /* Make sure enough space */ - if ((privpool->def->allocation + privvol->allocation) > + if ((privpool->def->allocation + privvol->target.allocation) > privpool->def->capacity) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Not enough free space in pool for volume '%s'"), @@ -1366,7 +1366,7 @@ parallelsStorageVolCreateXMLFrom(virStoragePoolPtr pool, if (VIR_STRDUP(privvol->key, privvol->target.path) < 0) goto cleanup; - privpool->def->allocation += privvol->allocation; + privpool->def->allocation += privvol->target.allocation; privpool->def->available = (privpool->def->capacity - privpool->def->allocation); @@ -1393,7 +1393,7 @@ int parallelsStorageVolDefRemove(virStoragePoolObjPtr privpool, char *xml_path = NULL; size_t i; - privpool->def->allocation -= privvol->allocation; + privpool->def->allocation -= privvol->target.allocation; privpool->def->available = (privpool->def->capacity - privpool->def->allocation); @@ -1516,8 +1516,8 @@ parallelsStorageVolGetInfo(virStorageVolPtr vol, virStorageVolInfoPtr info) memset(info, 0, sizeof(*info)); info->type = parallelsStorageVolTypeForPool(privpool->def->type); - info->capacity = privvol->capacity; - info->allocation = privvol->allocation; + info->capacity = privvol->target.capacity; + info->allocation = privvol->target.allocation; ret = 0; cleanup: diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 3a5eefd..508e4c0 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -2007,13 +2007,13 @@ phypStorageVolCreateXML(virStoragePoolPtr pool, goto err; } - if (!voldef->capacity) { + if (!voldef->target.capacity) { VIR_ERROR(_("Capacity cannot be empty.")); goto err; } key = phypBuildVolume(pool->conn, voldef->name, spdef->name, - voldef->capacity); + voldef->target.capacity); if (key == NULL) goto err; diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 9abaccd..c8cf50e 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -287,7 +287,7 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn ATTRIBUTE_UNUSED, goto cleanup; } - remain = vol->allocation; + remain = vol->target.allocation; if (inputvol) { int res = virStorageBackendCopyToFD(vol, inputvol, @@ -344,7 +344,7 @@ createRawFile(int fd, virStorageVolDefPtr vol, /* Seek to the final size, so the capacity is available upfront * for progress reporting */ - if (ftruncate(fd, vol->capacity) < 0) { + if (ftruncate(fd, vol->target.capacity) < 0) { ret = -errno; virReportSystemError(errno, _("cannot extend file '%s'"), @@ -361,27 +361,27 @@ createRawFile(int fd, virStorageVolDefPtr vol, * to writing zeroes block by block in case fallocate isn't * available, and since we're going to copy data from another * file it doesn't make sense to write the file twice. */ - if (vol->allocation) { - if (fallocate(fd, 0, 0, vol->allocation) == 0) { + if (vol->target.allocation) { + if (fallocate(fd, 0, 0, vol->target.allocation) == 0) { need_alloc = false; } else if (errno != ENOSYS && errno != EOPNOTSUPP) { ret = -errno; virReportSystemError(errno, _("cannot allocate %llu bytes in file '%s'"), - vol->allocation, vol->target.path); + vol->target.allocation, vol->target.path); goto cleanup; } } #endif - remain = vol->allocation; + remain = vol->target.allocation; if (inputvol) { /* allow zero blocks to be skipped if we've requested sparse * allocation (allocation < capacity) or we have already * been able to allocate the required space. */ bool want_sparse = !need_alloc || - (vol->allocation < inputvol->capacity); + (vol->target.allocation < inputvol->target.capacity); ret = virStorageBackendCopyToFD(vol, inputvol, fd, &remain, want_sparse); if (ret < 0) { @@ -390,7 +390,7 @@ createRawFile(int fd, virStorageVolDefPtr vol, } if (remain && need_alloc) { - if (safezero(fd, vol->allocation - remain, remain) < 0) { + if (safezero(fd, vol->target.allocation - remain, remain) < 0) { ret = -errno; virReportSystemError(errno, _("cannot fill file '%s'"), vol->target.path); @@ -924,7 +924,7 @@ virStorageBackendCreateQemuImgCmd(virConnectPtr conn, } /* Size in KB */ - size_arg = VIR_DIV_UP(vol->capacity, 1024); + size_arg = VIR_DIV_UP(vol->target.capacity, 1024); cmd = virCommandNew(create_tool); @@ -1070,7 +1070,7 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn ATTRIBUTE_UNUSED, /* Size in MB - yes different units to qemu-img :-( */ if (virAsprintf(&size, "%llu", - VIR_DIV_UP(vol->capacity, (1024 * 1024))) < 0) + VIR_DIV_UP(vol->target.capacity, (1024 * 1024))) < 0) return -1; cmd = virCommandNewArgList("qcow-create", size, vol->target.path, NULL); @@ -1424,8 +1424,8 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, int ret; if ((ret = virStorageBackendUpdateVolTargetInfo(&vol->target, - &vol->allocation, - withCapacity ? &vol->capacity : NULL, + &vol->target.allocation, + withCapacity ? &vol->target.capacity : NULL, withBlockVolFormat, openflags)) < 0) return ret; diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 01f1b17..13336fc 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -131,11 +131,11 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, /* The above gets allocation wrong for * extended partitions, so overwrite it */ - vol->allocation = vol->capacity = + vol->target.allocation = vol->target.capacity = (vol->source.extents[0].end - vol->source.extents[0].start); if (STRNEQ(groups[2], "metadata")) - pool->def->allocation += vol->allocation; + pool->def->allocation += vol->target.allocation; if (vol->source.extents[0].end > pool->def->capacity) pool->def->capacity = vol->source.extents[0].end; @@ -649,7 +649,7 @@ virStorageBackendDiskCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED, if (virStorageBackendDiskPartBoundaries(pool, &startOffset, &endOffset, - vol->capacity) != 0) { + vol->target.capacity) != 0) { goto cleanup; } diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 113593b..e8617cb 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -880,8 +880,8 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, if ((ret = virStorageBackendProbeTarget(&vol->target, &backingStore, &backingStoreFormat, - &vol->allocation, - &vol->capacity, + &vol->target.allocation, + &vol->target.capacity, &vol->target.encryption)) < 0) { if (ret == -2) { /* Silently ignore non-regular files, @@ -1280,7 +1280,7 @@ virStorageBackendFileSystemVolResize(virConnectPtr conn ATTRIBUTE_UNUSED, if (vol->target.format == VIR_STORAGE_FILE_RAW) { return virStorageFileResize(vol->target.path, capacity, - vol->allocation, pre_allocate); + vol->target.allocation, pre_allocate); } else { if (pre_allocate) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 19e6a3b..6eed3ec 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -268,8 +268,8 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, goto cleanup; if (virStorageBackendUpdateVolTargetInfoFD(&vol->target, -1, st, - &vol->allocation, - &vol->capacity) < 0) + &vol->target.allocation, + &vol->target.capacity) < 0) goto cleanup; if (virStorageBackendGlusterSetMetadata(state, vol, name) < 0) @@ -311,7 +311,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, vol->backingStore.format = VIR_STORAGE_FILE_RAW; } if (meta->capacity) - vol->capacity = meta->capacity; + vol->target.capacity = meta->capacity; if (meta->encrypted) { if (VIR_ALLOC(vol->target.encryption) < 0) goto cleanup; diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index aea624e..a597e67 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -177,7 +177,7 @@ virStorageBackendLogicalMakeVol(char **const groups, "%s", _("malformed volume extent size value")); goto cleanup; } - if (virStrToLong_ull(groups[8], NULL, 10, &vol->allocation) < 0) { + if (virStrToLong_ull(groups[8], NULL, 10, &vol->target.allocation) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed volume allocation value")); goto cleanup; @@ -744,12 +744,14 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, "--name", vol->name, NULL); virCommandAddArg(cmd, "-L"); - if (vol->capacity != vol->allocation) { + if (vol->target.capacity != vol->target.allocation) { virCommandAddArgFormat(cmd, "%lluK", - VIR_DIV_UP(vol->allocation ? vol->allocation : 1, 1024)); + VIR_DIV_UP(vol->target.allocation + ? vol->target.allocation : 1, 1024)); virCommandAddArg(cmd, "--virtualsize"); } - virCommandAddArgFormat(cmd, "%lluK", VIR_DIV_UP(vol->capacity, 1024)); + virCommandAddArgFormat(cmd, "%lluK", VIR_DIV_UP(vol->target.capacity, + 1024)); if (vol->backingStore.path) virCommandAddArgList(cmd, "-s", vol->backingStore.path, NULL); else diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c index 1242150..8c3b0df 100644 --- a/src/storage/storage_backend_mpath.c +++ b/src/storage/storage_backend_mpath.c @@ -1,7 +1,7 @@ /* * storage_backend_mpath.c: storage backend for multipath handling * - * Copyright (C) 2009-2011, 2013 Red Hat, Inc. + * Copyright (C) 2009-2014 Red Hat, Inc. * Copyright (C) 2009-2008 Dave Allan * * This library is free software; you can redistribute it and/or @@ -71,8 +71,8 @@ virStorageBackendMpathNewVol(virStoragePoolObjPtr pool, if (VIR_APPEND_ELEMENT_COPY(pool->volumes.objs, pool->volumes.count, vol) < 0) goto cleanup; - pool->def->capacity += vol->capacity; - pool->def->allocation += vol->allocation; + pool->def->capacity += vol->target.capacity; + pool->def->allocation += vol->target.allocation; ret = 0; cleanup: diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 34e6f45..029d2f0 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -1,7 +1,7 @@ /* * storage_backend_rbd.c: storage backend for RBD (RADOS Block Device) handling * - * Copyright (C) 2013 Red Hat, Inc. + * Copyright (C) 2013-2014 Red Hat, Inc. * Copyright (C) 2012 Wido den Hollander * * This library is free software; you can redistribute it and/or @@ -300,8 +300,8 @@ static int volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr vol, (unsigned long long)info.obj_size, (unsigned long long)info.num_objs); - vol->capacity = info.size; - vol->allocation = info.obj_size * info.num_objs; + vol->target.capacity = info.size; + vol->target.allocation = info.obj_size * info.num_objs; vol->type = VIR_STORAGE_VOL_NETWORK; VIR_FREE(vol->target.path); @@ -516,7 +516,7 @@ virStorageBackendRBDBuildVol(virConnectPtr conn, VIR_DEBUG("Creating RBD image %s/%s with size %llu", pool->def->source.name, - vol->name, vol->capacity); + vol->name, vol->target.capacity); virCheckFlags(0, -1); @@ -532,7 +532,8 @@ virStorageBackendRBDBuildVol(virConnectPtr conn, goto cleanup; } - r = virStorageBackendRBDCreateImage(ptr.ioctx, vol->name, vol->capacity); + r = virStorageBackendRBDCreateImage(ptr.ioctx, vol->name, + vol->target.capacity); if (r < 0) { virReportSystemError(-r, _("failed to create volume '%s/%s'"), pool->def->source.name, diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 4c2484d..6652a6a 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -213,8 +213,8 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, goto free_vol; } - pool->def->capacity += vol->capacity; - pool->def->allocation += vol->allocation; + pool->def->capacity += vol->target.capacity; + pool->def->allocation += vol->target.allocation; if (VIR_APPEND_ELEMENT(pool->volumes.objs, pool->volumes.count, vol) < 0) { retval = -1; diff --git a/src/storage/storage_backend_sheepdog.c b/src/storage/storage_backend_sheepdog.c index df86916..9419859 100644 --- a/src/storage/storage_backend_sheepdog.c +++ b/src/storage/storage_backend_sheepdog.c @@ -1,7 +1,7 @@ /* * storage_backend_sheepdog.c: storage backend for Sheepdog handling * - * Copyright (C) 2013 Red Hat, Inc. + * Copyright (C) 2013-2014 Red Hat, Inc. * Copyright (C) 2012 Wido den Hollander * Copyright (C) 2012 Frank Spijkerman * Copyright (C) 2012 Sebastian Wiedenroth @@ -267,7 +267,7 @@ virStorageBackendSheepdogBuildVol(virConnectPtr conn, virCheckFlags(0, -1); virCommandPtr cmd = virCommandNewArgList(COLLIE, "vdi", "create", vol->name, NULL); - virCommandAddArgFormat(cmd, "%llu", vol->capacity); + virCommandAddArgFormat(cmd, "%llu", vol->target.capacity); virStorageBackendSheepdogAddHostArg(cmd, pool); if (virCommandRun(cmd, NULL) < 0) goto cleanup; @@ -298,7 +298,7 @@ virStorageBackendSheepdogParseVdiList(virStorageVolDefPtr vol, int id; const char *p, *next; - vol->allocation = vol->capacity = 0; + vol->target.allocation = vol->target.capacity = 0; p = output; do { @@ -329,12 +329,12 @@ virStorageBackendSheepdogParseVdiList(virStorageVolDefPtr vol, p = end + 1; - if (virStrToLong_ull(p, &end, 10, &vol->capacity) < 0) + if (virStrToLong_ull(p, &end, 10, &vol->target.capacity) < 0) return -1; p = end + 1; - if (virStrToLong_ull(p, &end, 10, &vol->allocation) < 0) + if (virStrToLong_ull(p, &end, 10, &vol->target.allocation) < 0) return -1; return 0; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 6fca3d2..2f98f78 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1613,8 +1613,8 @@ storageVolDelete(virStorageVolPtr obj, goto cleanup; /* Update pool metadata */ - pool->def->allocation -= vol->allocation; - pool->def->available += vol->allocation; + pool->def->allocation -= vol->target.allocation; + pool->def->available += vol->target.allocation; for (i = 0; i < pool->volumes.count; i++) { if (pool->volumes.objs[i] == vol) { @@ -1747,8 +1747,8 @@ storageVolCreateXML(virStoragePoolPtr obj, } /* Update pool metadata */ - pool->def->allocation += buildvoldef->allocation; - pool->def->available -= buildvoldef->allocation; + pool->def->allocation += buildvoldef->target.allocation; + pool->def->available -= buildvoldef->target.allocation; VIR_INFO("Creating volume '%s' in storage pool '%s'", volobj->name, pool->def->name); @@ -1841,13 +1841,13 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, } /* Is there ever a valid case for this? */ - if (newvol->capacity < origvol->capacity) - newvol->capacity = origvol->capacity; + if (newvol->target.capacity < origvol->target.capacity) + newvol->target.capacity = origvol->target.capacity; /* Make sure allocation is at least as large as the destination cap, * to make absolutely sure we copy all possible contents */ - if (newvol->allocation < origvol->capacity) - newvol->allocation = origvol->capacity; + if (newvol->target.allocation < origvol->target.capacity) + newvol->target.allocation = origvol->target.capacity; if (!backend->buildVolFrom) { virReportError(VIR_ERR_NO_SUPPORT, @@ -1907,7 +1907,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, origvol->building = 0; newvol->building = 0; - allocation = newvol->allocation; + allocation = newvol->target.allocation; newvol = NULL; pool->asyncjobs--; @@ -2152,20 +2152,20 @@ storageVolResize(virStorageVolPtr obj, } if (flags & VIR_STORAGE_VOL_RESIZE_DELTA) { - abs_capacity = vol->capacity + capacity; + abs_capacity = vol->target.capacity + capacity; flags &= ~VIR_STORAGE_VOL_RESIZE_DELTA; } else { abs_capacity = capacity; } - if (abs_capacity < vol->allocation) { + if (abs_capacity < vol->target.allocation) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("can't shrink capacity below " "existing allocation")); goto cleanup; } - if (abs_capacity < vol->capacity && + if (abs_capacity < vol->target.capacity && !(flags & VIR_STORAGE_VOL_RESIZE_SHRINK)) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("Can't shrink capacity below current " @@ -2173,7 +2173,7 @@ storageVolResize(virStorageVolPtr obj, goto cleanup; } - if (abs_capacity > vol->capacity + pool->def->available) { + if (abs_capacity > vol->target.capacity + pool->def->available) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Not enough space left on storage pool")); goto cleanup; @@ -2189,13 +2189,13 @@ storageVolResize(virStorageVolPtr obj, if (backend->resizeVol(obj->conn, pool, vol, abs_capacity, flags) < 0) goto cleanup; - vol->capacity = abs_capacity; + vol->target.capacity = abs_capacity; if (flags & VIR_STORAGE_VOL_RESIZE_ALLOCATE) - vol->allocation = abs_capacity; + vol->target.allocation = abs_capacity; /* Update pool metadata */ - pool->def->allocation += (abs_capacity - vol->capacity); - pool->def->available -= (abs_capacity - vol->capacity); + pool->def->allocation += (abs_capacity - vol->target.capacity); + pool->def->available -= (abs_capacity - vol->target.capacity); ret = 0; @@ -2388,7 +2388,7 @@ storageVolWipeInternal(virStorageVolDefPtr def, ret = storageWipeExtent(def, fd, 0, - def->allocation, + def->target.allocation, writebuf, st.st_blksize, &bytes_wiped); @@ -2529,8 +2529,8 @@ storageVolGetInfo(virStorageVolPtr obj, memset(info, 0, sizeof(*info)); info->type = vol->type; - info->capacity = vol->capacity; - info->allocation = vol->allocation; + info->capacity = vol->target.capacity; + info->allocation = vol->target.allocation; ret = 0; cleanup: diff --git a/src/test/test_driver.c b/src/test/test_driver.c index ef88d4b..77e30b2 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1260,7 +1260,7 @@ testOpenVolumesForPool(const char *file, if (VIR_APPEND_ELEMENT_COPY(pool->volumes.objs, pool->volumes.count, def) < 0) goto error; - pool->def->allocation += def->allocation; + pool->def->allocation += def->target.allocation; pool->def->available = (pool->def->capacity - pool->def->allocation); def = NULL; @@ -5511,7 +5511,7 @@ testStorageVolCreateXML(virStoragePoolPtr pool, } /* Make sure enough space */ - if ((privpool->def->allocation + privvol->allocation) > + if ((privpool->def->allocation + privvol->target.allocation) > privpool->def->capacity) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Not enough free space in pool for volume '%s'"), @@ -5529,7 +5529,7 @@ testStorageVolCreateXML(virStoragePoolPtr pool, privpool->volumes.count, privvol) < 0) goto cleanup; - privpool->def->allocation += privvol->allocation; + privpool->def->allocation += privvol->target.allocation; privpool->def->available = (privpool->def->capacity - privpool->def->allocation); @@ -5593,7 +5593,7 @@ testStorageVolCreateXMLFrom(virStoragePoolPtr pool, } /* Make sure enough space */ - if ((privpool->def->allocation + privvol->allocation) > + if ((privpool->def->allocation + privvol->target.allocation) > privpool->def->capacity) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Not enough free space in pool for volume '%s'"), @@ -5613,7 +5613,7 @@ testStorageVolCreateXMLFrom(virStoragePoolPtr pool, privpool->volumes.count, privvol) < 0) goto cleanup; - privpool->def->allocation += privvol->allocation; + privpool->def->allocation += privvol->target.allocation; privpool->def->available = (privpool->def->capacity - privpool->def->allocation); @@ -5668,7 +5668,7 @@ testStorageVolDelete(virStorageVolPtr vol, } - privpool->def->allocation -= privvol->allocation; + privpool->def->allocation -= privvol->target.allocation; privpool->def->available = (privpool->def->capacity - privpool->def->allocation); @@ -5738,8 +5738,8 @@ testStorageVolGetInfo(virStorageVolPtr vol, memset(info, 0, sizeof(*info)); info->type = testStorageVolumeTypeForPool(privpool->def->type); - info->capacity = privvol->capacity; - info->allocation = privvol->allocation; + info->capacity = privvol->target.capacity; + info->allocation = privvol->target.allocation; ret = 0; cleanup: diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 00d5456..aa06d41 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -234,6 +234,8 @@ struct _virStorageSource { virStoragePermsPtr perms; virStorageTimestampsPtr timestamps; + unsigned long long allocation; /* in bytes, 0 if unknown */ + unsigned long long capacity; /* in bytes, 0 if unknown */ size_t nseclabels; virSecurityDeviceLabelDefPtr *seclabels; }; diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 4eed78e..a305fe2 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -8786,10 +8786,11 @@ static virStorageVolPtr vboxStorageVolCreateXML(virStoragePoolPtr pool, rc = data->vboxObj->vtbl->CreateHardDisk(data->vboxObj, hddFormatUtf16, hddNameUtf16, &hardDisk); if (NS_SUCCEEDED(rc)) { IProgress *progress = NULL; - PRUint64 logicalSize = VIR_DIV_UP(def->capacity, 1024 * 1024); + PRUint64 logicalSize = VIR_DIV_UP(def->target.capacity, + 1024 * 1024); PRUint32 variant = HardDiskVariant_Standard; - if (def->capacity == def->allocation) + if (def->target.capacity == def->target.allocation) variant = HardDiskVariant_Fixed; #if VBOX_API_VERSION < 4003000 @@ -9142,16 +9143,16 @@ static char *vboxStorageVolGetXMLDesc(virStorageVolPtr vol, unsigned int flags) rc = hardDisk->vtbl->GetLogicalSize(hardDisk, &hddLogicalSize); if (NS_SUCCEEDED(rc) && defOk) { #if VBOX_API_VERSION < 4000000 - def.capacity = hddLogicalSize * 1024 * 1024; /* MB => Bytes */ + def.target.capacity = hddLogicalSize * 1024 * 1024; /* MB => Bytes */ #else /* VBOX_API_VERSION >= 4000000 */ - def.capacity = hddLogicalSize; + def.target.capacity = hddLogicalSize; #endif /* VBOX_API_VERSION >= 4000000 */ } else defOk = 0; rc = VBOX_MEDIUM_FUNC_ARG1(hardDisk, GetSize, &hddActualSize); if (NS_SUCCEEDED(rc) && defOk) - def.allocation = hddActualSize; + def.target.allocation = hddActualSize; else defOk = 0; diff --git a/tests/storagebackendsheepdogtest.c b/tests/storagebackendsheepdogtest.c index e219acb..14fc76d 100644 --- a/tests/storagebackendsheepdogtest.c +++ b/tests/storagebackendsheepdogtest.c @@ -1,6 +1,7 @@ /* * storagebackendsheepdogtest.c: storage backend for Sheepdog handling * + * Copyright (C) 2014 Red Hat, Inc. * Copyright (C) 2012 Sebastian Wiedenroth * * This library is free software; you can redistribute it and/or @@ -114,8 +115,8 @@ test_vdi_list_parser(collie_test test, char *poolxml, char *volxml) goto cleanup; } - if (vol->capacity == test.expected_capacity && - vol->allocation == test.expected_allocation) + if (vol->target.capacity == test.expected_capacity && + vol->target.allocation == test.expected_allocation) ret = 0; cleanup: -- 1.9.0

On 04/02/14 05:05, Eric Blake wrote:
One of the features of qcow2 is that a wrapper file can have more capacity than its backing file from the guest's perspective; what's more, sparse files make tracking allocation of both the active and backing file worthwhile. As such, it makes more sense to show allocation numbers for each file in a chain, and not just the top-level file. This sets up the fields for the tracking, although it does not modify XML to display any new information.
* src/util/virstoragefile.h (_virStorageSource): Add fields. * src/conf/storage_conf.h (_virStorageVolDef): Drop redundant fields. * src/storage/storage_backend.c (virStorageBackendCreateBlockFrom) (createRawFile, virStorageBackendCreateQemuImgCmd) (virStorageBackendCreateQcowCreate): Update clients. * src/storage/storage_driver.c (storageVolDelete) (storageVolCreateXML, storageVolCreateXMLFrom, storageVolResize) (storageVolWipeInternal, storageVolGetInfo): Likewise. * src/storage/storage_backend_fs.c (virStorageBackendProbeTarget) (virStorageBackendFileSystemRefresh) (virStorageBackendFileSystemVolResize) (virStorageBackendFileSystemVolRefresh): Likewise. * src/storage/storage_backend_logical.c (virStorageBackendLogicalMakeVol) (virStorageBackendLogicalCreateVol): Likewise. * src/storage/storage_backend_scsi.c (virStorageBackendSCSINewLun): Likewise. * src/storage/storage_backend_mpath.c (virStorageBackendMpathNewVol): Likewise. * src/storage/storage_backend_rbd.c (volStorageBackendRBDRefreshVolInfo) (virStorageBackendRBDCreateImage): Likewise. * src/storage/storage_backend_disk.c (virStorageBackendDiskMakeDataVol) (virStorageBackendDiskCreateVol): Likewise. * src/storage/storage_backend_sheepdog.c (virStorageBackendSheepdogBuildVol) (virStorageBackendSheepdogParseVdiList): Likewise. * src/storage/storage_backend_gluster.c (virStorageBackendGlusterRefreshVol): Likewise. * src/conf/storage_conf.c (virStorageVolDefFormat) (virStorageVolDefParseXML): Likewise. * src/test/test_driver.c (testOpenVolumesForPool) (testStorageVolCreateXML, testStorageVolCreateXMLFrom) (testStorageVolDelete, testStorageVolGetInfo): Likewise. * src/esx/esx_storage_backend_iscsi.c (esxStorageVolGetXMLDesc): Likewise. * src/esx/esx_storage_backend_vmfs.c (esxStorageVolGetXMLDesc) (esxStorageVolCreateXML): Likewise. * src/parallels/parallels_driver.c (parallelsAddHddByVolume): Likewise. * src/parallels/parallels_storage.c (parallelsDiskDescParseNode) (parallelsStorageVolDefineXML, parallelsStorageVolCreateXMLFrom) (parallelsStorageVolDefRemove, parallelsStorageVolGetInfo): Likewise. * src/vbox/vbox_tmpl.c (vboxStorageVolCreateXML) (vboxStorageVolGetXMLDesc): Likewise. * tests/storagebackendsheepdogtest.c (test_vdi_list_parser): Likewise. * src/phyp/phyp_driver.c (phypStorageVolCreateXML): Likewise. --- src/conf/storage_conf.c | 10 ++++----- src/conf/storage_conf.h | 3 --- src/esx/esx_storage_backend_iscsi.c | 5 +++-- src/esx/esx_storage_backend_vmfs.c | 20 ++++++++--------- src/parallels/parallels_driver.c | 2 +- src/parallels/parallels_storage.c | 22 +++++++++---------- src/phyp/phyp_driver.c | 4 ++-- src/storage/storage_backend.c | 24 ++++++++++---------- src/storage/storage_backend_disk.c | 6 ++--- src/storage/storage_backend_fs.c | 6 ++--- src/storage/storage_backend_gluster.c | 6 ++--- src/storage/storage_backend_logical.c | 10 +++++---- src/storage/storage_backend_mpath.c | 6 ++--- src/storage/storage_backend_rbd.c | 11 +++++----- src/storage/storage_backend_scsi.c | 4 ++-- src/storage/storage_backend_sheepdog.c | 10 ++++----- src/storage/storage_driver.c | 40 +++++++++++++++++----------------- src/test/test_driver.c | 16 +++++++------- src/util/virstoragefile.h | 2 ++ src/vbox/vbox_tmpl.c | 11 +++++----- tests/storagebackendsheepdogtest.c | 5 +++-- 21 files changed, 114 insertions(+), 109 deletions(-)
ACK; Peter

Now that each virStorageSource can track allocation information, and given that we already have the information without extra syscalls, it's easier to just always populate the information directly into the struct than it is to sometimes pass the address of the struct members down the call chain. * src/storage/storage_backend.h (virStorageBackendUpdateVolInfo) (virStorageBackendUpdateVolTargetInfo) (virStorageBackendUpdateVolTargetInfoFD): Update signature. * src/storage/storage_backend.c (virStorageBackendUpdateVolInfo) (virStorageBackendUpdateVolTargetInfo) (virStorageBackendUpdateVolTargetInfoFD): Always populate struct members instead. * src/storage/storage_backend_disk.c (virStorageBackendDiskMakeDataVol): Update client. * src/storage/storage_backend_fs.c (virStorageBackendProbeTarget) (virStorageBackendFileSystemRefresh) (virStorageBackendFileSystemVolRefresh): Likewise. * src/storage/storage_backend_gluster.c (virStorageBackendGlusterRefreshVol): Likewise. * src/storage/storage_backend_logical.c (virStorageBackendLogicalMakeVol): Likewise. * src/storage/storage_backend_mpath.c (virStorageBackendMpathNewVol): Likewise. * src/storage/storage_backend_scsi.c (virStorageBackendSCSINewLun): Likewise. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/storage/storage_backend.c | 74 +++++++++++++---------------------- src/storage/storage_backend.h | 7 +--- src/storage/storage_backend_disk.c | 2 +- src/storage/storage_backend_fs.c | 16 +++----- src/storage/storage_backend_gluster.c | 4 +- src/storage/storage_backend_logical.c | 2 +- src/storage/storage_backend_mpath.c | 2 +- src/storage/storage_backend_scsi.c | 2 +- 8 files changed, 39 insertions(+), 70 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index c8cf50e..8fe3687 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1385,8 +1385,6 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, int virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, - unsigned long long *allocation, - unsigned long long *capacity, bool withBlockVolFormat, unsigned int openflags) { @@ -1397,11 +1395,7 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, goto cleanup; fd = ret; - if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, - fd, - &sb, - allocation, - capacity)) < 0) + if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb)) < 0) goto cleanup; if (withBlockVolFormat) { @@ -1417,22 +1411,18 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, - bool withCapacity, bool withBlockVolFormat, unsigned int openflags) { int ret; if ((ret = virStorageBackendUpdateVolTargetInfo(&vol->target, - &vol->target.allocation, - withCapacity ? &vol->target.capacity : NULL, withBlockVolFormat, openflags)) < 0) return ret; if (vol->backingStore.path && (ret = virStorageBackendUpdateVolTargetInfo(&vol->backingStore, - NULL, NULL, withBlockVolFormat, VIR_STORAGE_VOL_OPEN_DEFAULT)) < 0) return ret; @@ -1453,50 +1443,42 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, int virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, int fd, - struct stat *sb, - unsigned long long *allocation, - unsigned long long *capacity) + struct stat *sb) { #if WITH_SELINUX security_context_t filecon = NULL; #endif - if (allocation) { - if (S_ISREG(sb->st_mode)) { + if (S_ISREG(sb->st_mode)) { #ifndef WIN32 - *allocation = (unsigned long long)sb->st_blocks * - (unsigned long long)DEV_BSIZE; + target->allocation = (unsigned long long)sb->st_blocks * + (unsigned long long)DEV_BSIZE; #else - *allocation = sb->st_size; + target->allocation = sb->st_size; #endif - /* Regular files may be sparse, so logical size (capacity) is not same - * as actual allocation above - */ - if (capacity) - *capacity = sb->st_size; - } else if (S_ISDIR(sb->st_mode)) { - *allocation = 0; - if (capacity) - *capacity = 0; - - } else if (fd >= 0) { - off_t end; - /* XXX this is POSIX compliant, but doesn't work for CHAR files, - * only BLOCK. There is a Linux specific ioctl() for getting - * size of both CHAR / BLOCK devices we should check for in - * configure - */ - end = lseek(fd, 0, SEEK_END); - if (end == (off_t)-1) { - virReportSystemError(errno, - _("cannot seek to end of file '%s'"), - target->path); - return -1; - } - *allocation = end; - if (capacity) - *capacity = end; + /* Regular files may be sparse, so logical size (capacity) is not same + * as actual allocation above + */ + target->capacity = sb->st_size; + } else if (S_ISDIR(sb->st_mode)) { + target->allocation = 0; + target->capacity = 0; + } else if (fd >= 0) { + off_t end; + /* XXX this is POSIX compliant, but doesn't work for CHAR files, + * only BLOCK. There is a Linux specific ioctl() for getting + * size of both CHAR / BLOCK devices we should check for in + * configure + */ + end = lseek(fd, 0, SEEK_END); + if (end == (off_t)-1) { + virReportSystemError(errno, + _("cannot seek to end of file '%s'"), + target->path); + return -1; } + target->allocation = end; + target->capacity = end; } if (!target->perms && VIR_ALLOC(target->perms) < 0) diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 8442c13..89511f8 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -138,19 +138,14 @@ int virStorageBackendVolOpen(const char *path, struct stat *sb, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, - bool withCapacity, bool withBlockVolFormat, unsigned int openflags); int virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, - unsigned long long *allocation, - unsigned long long *capacity, bool withBlockVolFormat, unsigned int openflags); int virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, int fd, - struct stat *sb, - unsigned long long *allocation, - unsigned long long *capacity); + struct stat *sb); char *virStorageBackendStablePath(virStoragePoolObjPtr pool, const char *devpath, diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 13336fc..9cebcca 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -113,7 +113,7 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, } /* Refresh allocation/capacity/perms */ - if (virStorageBackendUpdateVolInfo(vol, true, false, + if (virStorageBackendUpdateVolInfo(vol, false, VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) return -1; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index e8617cb..501fa8d 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -65,8 +65,6 @@ static int ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) virStorageBackendProbeTarget(virStorageSourcePtr target, char **backingStore, int *backingStoreFormat, - unsigned long long *allocation, - unsigned long long *capacity, virStorageEncryptionPtr *encryption) { int fd = -1; @@ -86,9 +84,7 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, goto error; /* Take care to propagate ret, it is not always -1 */ fd = ret; - if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb, - allocation, - capacity)) < 0) { + if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb)) < 0) { goto error; } @@ -143,8 +139,8 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, ret = 0; } - if (capacity && meta && meta->capacity) - *capacity = meta->capacity; + if (meta && meta->capacity) + target->capacity = meta->capacity; if (encryption && meta && meta->encrypted) { if (VIR_ALLOC(*encryption) < 0) @@ -880,8 +876,6 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, if ((ret = virStorageBackendProbeTarget(&vol->target, &backingStore, &backingStoreFormat, - &vol->target.allocation, - &vol->target.capacity, &vol->target.encryption)) < 0) { if (ret == -2) { /* Silently ignore non-regular files, @@ -909,7 +903,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, vol->backingStore.format = backingStoreFormat; if (virStorageBackendUpdateVolTargetInfo(&vol->backingStore, - NULL, NULL, false, + false, VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) { /* The backing file is currently unavailable, the capacity, * allocation, owner, group and mode are unknown. Just log the @@ -1194,7 +1188,7 @@ virStorageBackendFileSystemVolRefresh(virConnectPtr conn, int ret; /* Refresh allocation / permissions info in case its changed */ - ret = virStorageBackendUpdateVolInfo(vol, false, false, + ret = virStorageBackendUpdateVolInfo(vol, false, VIR_STORAGE_VOL_FS_OPEN_FLAGS); if (ret < 0) return ret; diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 6eed3ec..4aec44b 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -267,9 +267,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, if (VIR_ALLOC(vol) < 0) goto cleanup; - if (virStorageBackendUpdateVolTargetInfoFD(&vol->target, -1, st, - &vol->target.allocation, - &vol->target.capacity) < 0) + if (virStorageBackendUpdateVolTargetInfoFD(&vol->target, -1, st) < 0) goto cleanup; if (virStorageBackendGlusterSetMetadata(state, vol, name) < 0) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index a597e67..ed3a012 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -149,7 +149,7 @@ virStorageBackendLogicalMakeVol(char **const groups, if (!vol->key && VIR_STRDUP(vol->key, groups[2]) < 0) goto cleanup; - if (virStorageBackendUpdateVolInfo(vol, true, false, + if (virStorageBackendUpdateVolInfo(vol, false, VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) goto cleanup; diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c index 8c3b0df..f0ed189 100644 --- a/src/storage/storage_backend_mpath.c +++ b/src/storage/storage_backend_mpath.c @@ -60,7 +60,7 @@ virStorageBackendMpathNewVol(virStoragePoolObjPtr pool, if (virAsprintf(&vol->target.path, "/dev/%s", dev) < 0) goto cleanup; - if (virStorageBackendUpdateVolInfo(vol, true, true, + if (virStorageBackendUpdateVolInfo(vol, true, VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) { goto cleanup; } diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 6652a6a..cf546fb 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -199,7 +199,7 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, goto free_vol; } - if (virStorageBackendUpdateVolInfo(vol, true, true, + if (virStorageBackendUpdateVolInfo(vol, true, VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to update volume for '%s'"), -- 1.9.0

On 04/02/14 05:05, Eric Blake wrote:
Now that each virStorageSource can track allocation information, and given that we already have the information without extra syscalls, it's easier to just always populate the information directly into the struct than it is to sometimes pass the address of the struct members down the call chain.
* src/storage/storage_backend.h (virStorageBackendUpdateVolInfo) (virStorageBackendUpdateVolTargetInfo) (virStorageBackendUpdateVolTargetInfoFD): Update signature. * src/storage/storage_backend.c (virStorageBackendUpdateVolInfo) (virStorageBackendUpdateVolTargetInfo) (virStorageBackendUpdateVolTargetInfoFD): Always populate struct members instead. * src/storage/storage_backend_disk.c (virStorageBackendDiskMakeDataVol): Update client. * src/storage/storage_backend_fs.c (virStorageBackendProbeTarget) (virStorageBackendFileSystemRefresh) (virStorageBackendFileSystemVolRefresh): Likewise. * src/storage/storage_backend_gluster.c (virStorageBackendGlusterRefreshVol): Likewise. * src/storage/storage_backend_logical.c (virStorageBackendLogicalMakeVol): Likewise. * src/storage/storage_backend_mpath.c (virStorageBackendMpathNewVol): Likewise. * src/storage/storage_backend_scsi.c (virStorageBackendSCSINewLun): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/storage/storage_backend.c | 74 +++++++++++++---------------------- src/storage/storage_backend.h | 7 +--- src/storage/storage_backend_disk.c | 2 +- src/storage/storage_backend_fs.c | 16 +++----- src/storage/storage_backend_gluster.c | 4 +- src/storage/storage_backend_logical.c | 2 +- src/storage/storage_backend_mpath.c | 2 +- src/storage/storage_backend_scsi.c | 2 +- 8 files changed, 39 insertions(+), 70 deletions(-)
ACK. Peter

On 04/01/2014 11:05 PM, Eric Blake wrote:
Now that each virStorageSource can track allocation information, and given that we already have the information without extra syscalls, it's easier to just always populate the information directly into the struct than it is to sometimes pass the address of the struct members down the call chain.
* src/storage/storage_backend.h (virStorageBackendUpdateVolInfo) (virStorageBackendUpdateVolTargetInfo) (virStorageBackendUpdateVolTargetInfoFD): Update signature. * src/storage/storage_backend.c (virStorageBackendUpdateVolInfo) (virStorageBackendUpdateVolTargetInfo) (virStorageBackendUpdateVolTargetInfoFD): Always populate struct members instead. * src/storage/storage_backend_disk.c (virStorageBackendDiskMakeDataVol): Update client. * src/storage/storage_backend_fs.c (virStorageBackendProbeTarget) (virStorageBackendFileSystemRefresh) (virStorageBackendFileSystemVolRefresh): Likewise. * src/storage/storage_backend_gluster.c (virStorageBackendGlusterRefreshVol): Likewise. * src/storage/storage_backend_logical.c (virStorageBackendLogicalMakeVol): Likewise. * src/storage/storage_backend_mpath.c (virStorageBackendMpathNewVol): Likewise. * src/storage/storage_backend_scsi.c (virStorageBackendSCSINewLun): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/storage/storage_backend.c | 74 +++++++++++++---------------------- src/storage/storage_backend.h | 7 +--- src/storage/storage_backend_disk.c | 2 +- src/storage/storage_backend_fs.c | 16 +++----- src/storage/storage_backend_gluster.c | 4 +- src/storage/storage_backend_logical.c | 2 +- src/storage/storage_backend_mpath.c | 2 +- src/storage/storage_backend_scsi.c | 2 +- 8 files changed, 39 insertions(+), 70 deletions(-)
When running virt-test recently I discovered a regression with this change and the removal of the 'withCapacity' flag. For example using: $ cat testpool.xml <pool type='dir'> <name>TestPool</name> <uuid>6bf80895-10b6-75a6-6059-89fdea2aefb7</uuid> <source> </source> <target> <path>/home/bz1024159/TestPool</path> <permissions> <mode>0755</mode> <owner>0</owner> <group>0</group> </permissions> </target> </pool> $ virsh pool-create testpool.xml $ virsh vol-create-as --pool TestPool temp_vol_1 --capacity 1048576 --allocation 1048576 --format qcow2 $ virsh vol-info --pool TestPool temp_vol_1 Prior to the changes the results are: Name: temp_vol_1 Type: file Capacity: 1.00 MiB Allocation: 196.00 KiB After the changes the results are: Name: temp_vol_1 Type: file Capacity: 192.50 KiB Allocation: 196.00 KiB $ ls -al TestPool total 204 drwxr-xr-x. 2 root root 4096 Apr 15 14:49 . drwxr-xr-x. 3 root root 4096 Apr 8 12:58 .. -rw-------. 1 root root 197120 Apr 15 14:49 temp_vol_1 Essentially after the changes, we return the size of the file instead of the perhaps intended capacity. I've tracked back through the differences, the call to virStorageBackendUpdateVolInfo() used to have a "bool withCapacity" which was removed. This seems to have been used by virStorageBackendFileSystemVolRefresh() where on input the vol->capacity is 1048576 because storageVolGetInfo() will have already grabbed the "vol" information from virStorageVolDefFindByName(). This is the only path where withCapacity was false prior to the changes. Looking at the comments prior to the calls for 'disk' and 'fs', it seems it was desired to not have the capacity updated for the 'fs' path. I have tested putting that flag back into the code and it works - I can send a patch shortly, but figured placing some 'recent' context around the patch would help... John
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index c8cf50e..8fe3687 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1385,8 +1385,6 @@ virStorageBackendVolOpen(const char *path, struct stat *sb,
int virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, - unsigned long long *allocation, - unsigned long long *capacity, bool withBlockVolFormat, unsigned int openflags) { @@ -1397,11 +1395,7 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, goto cleanup; fd = ret;
- if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, - fd, - &sb, - allocation, - capacity)) < 0) + if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb)) < 0) goto cleanup;
if (withBlockVolFormat) { @@ -1417,22 +1411,18 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target,
int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, - bool withCapacity, bool withBlockVolFormat, unsigned int openflags) { int ret;
if ((ret = virStorageBackendUpdateVolTargetInfo(&vol->target, - &vol->target.allocation, - withCapacity ? &vol->target.capacity : NULL, withBlockVolFormat, openflags)) < 0) return ret;
if (vol->backingStore.path && (ret = virStorageBackendUpdateVolTargetInfo(&vol->backingStore, - NULL, NULL, withBlockVolFormat, VIR_STORAGE_VOL_OPEN_DEFAULT)) < 0) return ret; @@ -1453,50 +1443,42 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, int virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, int fd, - struct stat *sb, - unsigned long long *allocation, - unsigned long long *capacity) + struct stat *sb) { #if WITH_SELINUX security_context_t filecon = NULL; #endif
- if (allocation) { - if (S_ISREG(sb->st_mode)) { + if (S_ISREG(sb->st_mode)) { #ifndef WIN32 - *allocation = (unsigned long long)sb->st_blocks * - (unsigned long long)DEV_BSIZE; + target->allocation = (unsigned long long)sb->st_blocks * + (unsigned long long)DEV_BSIZE; #else - *allocation = sb->st_size; + target->allocation = sb->st_size; #endif - /* Regular files may be sparse, so logical size (capacity) is not same - * as actual allocation above - */ - if (capacity) - *capacity = sb->st_size; - } else if (S_ISDIR(sb->st_mode)) { - *allocation = 0; - if (capacity) - *capacity = 0; - - } else if (fd >= 0) { - off_t end; - /* XXX this is POSIX compliant, but doesn't work for CHAR files, - * only BLOCK. There is a Linux specific ioctl() for getting - * size of both CHAR / BLOCK devices we should check for in - * configure - */ - end = lseek(fd, 0, SEEK_END); - if (end == (off_t)-1) { - virReportSystemError(errno, - _("cannot seek to end of file '%s'"), - target->path); - return -1; - } - *allocation = end; - if (capacity) - *capacity = end; + /* Regular files may be sparse, so logical size (capacity) is not same + * as actual allocation above + */ + target->capacity = sb->st_size; + } else if (S_ISDIR(sb->st_mode)) { + target->allocation = 0; + target->capacity = 0; + } else if (fd >= 0) { + off_t end; + /* XXX this is POSIX compliant, but doesn't work for CHAR files, + * only BLOCK. There is a Linux specific ioctl() for getting + * size of both CHAR / BLOCK devices we should check for in + * configure + */ + end = lseek(fd, 0, SEEK_END); + if (end == (off_t)-1) { + virReportSystemError(errno, + _("cannot seek to end of file '%s'"), + target->path); + return -1; } + target->allocation = end; + target->capacity = end; }
if (!target->perms && VIR_ALLOC(target->perms) < 0) diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 8442c13..89511f8 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -138,19 +138,14 @@ int virStorageBackendVolOpen(const char *path, struct stat *sb, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol, - bool withCapacity, bool withBlockVolFormat, unsigned int openflags); int virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, - unsigned long long *allocation, - unsigned long long *capacity, bool withBlockVolFormat, unsigned int openflags); int virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, int fd, - struct stat *sb, - unsigned long long *allocation, - unsigned long long *capacity); + struct stat *sb);
char *virStorageBackendStablePath(virStoragePoolObjPtr pool, const char *devpath, diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 13336fc..9cebcca 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -113,7 +113,7 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool, }
/* Refresh allocation/capacity/perms */ - if (virStorageBackendUpdateVolInfo(vol, true, false, + if (virStorageBackendUpdateVolInfo(vol, false, VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) return -1;
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index e8617cb..501fa8d 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -65,8 +65,6 @@ static int ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) virStorageBackendProbeTarget(virStorageSourcePtr target, char **backingStore, int *backingStoreFormat, - unsigned long long *allocation, - unsigned long long *capacity, virStorageEncryptionPtr *encryption) { int fd = -1; @@ -86,9 +84,7 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, goto error; /* Take care to propagate ret, it is not always -1 */ fd = ret;
- if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb, - allocation, - capacity)) < 0) { + if ((ret = virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb)) < 0) { goto error; }
@@ -143,8 +139,8 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, ret = 0; }
- if (capacity && meta && meta->capacity) - *capacity = meta->capacity; + if (meta && meta->capacity) + target->capacity = meta->capacity;
if (encryption && meta && meta->encrypted) { if (VIR_ALLOC(*encryption) < 0) @@ -880,8 +876,6 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, if ((ret = virStorageBackendProbeTarget(&vol->target, &backingStore, &backingStoreFormat, - &vol->target.allocation, - &vol->target.capacity, &vol->target.encryption)) < 0) { if (ret == -2) { /* Silently ignore non-regular files, @@ -909,7 +903,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, vol->backingStore.format = backingStoreFormat;
if (virStorageBackendUpdateVolTargetInfo(&vol->backingStore, - NULL, NULL, false, + false, VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) { /* The backing file is currently unavailable, the capacity, * allocation, owner, group and mode are unknown. Just log the @@ -1194,7 +1188,7 @@ virStorageBackendFileSystemVolRefresh(virConnectPtr conn, int ret;
/* Refresh allocation / permissions info in case its changed */ - ret = virStorageBackendUpdateVolInfo(vol, false, false, + ret = virStorageBackendUpdateVolInfo(vol, false, VIR_STORAGE_VOL_FS_OPEN_FLAGS); if (ret < 0) return ret; diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 6eed3ec..4aec44b 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -267,9 +267,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, if (VIR_ALLOC(vol) < 0) goto cleanup;
- if (virStorageBackendUpdateVolTargetInfoFD(&vol->target, -1, st, - &vol->target.allocation, - &vol->target.capacity) < 0) + if (virStorageBackendUpdateVolTargetInfoFD(&vol->target, -1, st) < 0) goto cleanup;
if (virStorageBackendGlusterSetMetadata(state, vol, name) < 0) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index a597e67..ed3a012 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -149,7 +149,7 @@ virStorageBackendLogicalMakeVol(char **const groups, if (!vol->key && VIR_STRDUP(vol->key, groups[2]) < 0) goto cleanup;
- if (virStorageBackendUpdateVolInfo(vol, true, false, + if (virStorageBackendUpdateVolInfo(vol, false, VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) goto cleanup;
diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c index 8c3b0df..f0ed189 100644 --- a/src/storage/storage_backend_mpath.c +++ b/src/storage/storage_backend_mpath.c @@ -60,7 +60,7 @@ virStorageBackendMpathNewVol(virStoragePoolObjPtr pool, if (virAsprintf(&vol->target.path, "/dev/%s", dev) < 0) goto cleanup;
- if (virStorageBackendUpdateVolInfo(vol, true, true, + if (virStorageBackendUpdateVolInfo(vol, true, VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) { goto cleanup; } diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 6652a6a..cf546fb 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -199,7 +199,7 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool, goto free_vol; }
- if (virStorageBackendUpdateVolInfo(vol, true, true, + if (virStorageBackendUpdateVolInfo(vol, true, VIR_STORAGE_VOL_OPEN_DEFAULT) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to update volume for '%s'"),

A future patch will merge virStorageFileMetadata and virStorageSource, but I found it easier to do if both structs use the same information for tracking whether a source file needs encryption keys. * src/util/virstoragefile.h (_virStorageFileMetadata): Prepare full encryption struct instead of just a bool. * src/storage/storage_backend_fs.c (virStorageBackendProbeTarget): Use transfer semantics. * src/storage/storage_backend_gluster.c (virStorageBackendGlusterRefreshVol): Likewise. * src/util/virstoragefile.c (virStorageFileGetMetadataInternal): Populate struct. (virStorageFileFreeMetadata): Adjust clients. * tests/virstoragetest.c (testStorageChain): Likewise. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/storage/storage_backend_fs.c | 6 +++--- src/storage/storage_backend_gluster.c | 6 +++--- src/util/virstoragefile.c | 4 +++- src/util/virstoragefile.h | 3 ++- tests/virstoragetest.c | 4 ++-- 5 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 501fa8d..de6521c 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -142,9 +142,9 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, if (meta && meta->capacity) target->capacity = meta->capacity; - if (encryption && meta && meta->encrypted) { - if (VIR_ALLOC(*encryption) < 0) - goto cleanup; + if (encryption && meta && meta->encryption) { + *encryption = meta->encryption; + meta->encryption = NULL; switch (target->format) { case VIR_STORAGE_FILE_QCOW: diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 4aec44b..d131f13 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -310,9 +310,9 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, } if (meta->capacity) vol->target.capacity = meta->capacity; - if (meta->encrypted) { - if (VIR_ALLOC(vol->target.encryption) < 0) - goto cleanup; + if (meta->encryption) { + vol->target.encryption = meta->encryption; + meta->encryption = NULL; if (vol->target.format == VIR_STORAGE_FILE_QCOW || vol->target.format == VIR_STORAGE_FILE_QCOW2) vol->target.encryption->format = VIR_STORAGE_ENCRYPTION_FORMAT_QCOW; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 137bacc..e6a985d 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -837,7 +837,8 @@ virStorageFileGetMetadataInternal(const char *path, crypt_format = virReadBufInt32BE(buf + fileTypeInfo[format].qcowCryptOffset); - meta->encrypted = crypt_format != 0; + if (crypt_format && VIR_ALLOC(meta->encryption) < 0) + goto cleanup; } if (fileTypeInfo[format].getBackingStore != NULL) { @@ -1209,6 +1210,7 @@ virStorageFileFreeMetadata(virStorageFileMetadata *meta) VIR_FREE(meta->compat); VIR_FREE(meta->directory); virBitmapFree(meta->features); + virStorageEncryptionFree(meta->encryption); VIR_FREE(meta); } diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index aa06d41..56105d0 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -114,8 +114,9 @@ struct _virStorageFileMetadata { int backingStoreFormat; /* enum virStorageFileFormat */ bool backingStoreIsFile; virStorageFileMetadataPtr backingMeta; + + virStorageEncryptionPtr encryption; unsigned long long capacity; - bool encrypted; virBitmapPtr features; /* bits described by enum virStorageFileFeature */ char *compat; }; diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 2890651..3089d70 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2013 Red Hat, Inc. + * Copyright (C) 2013-2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -284,7 +284,7 @@ testStorageChain(const void *args) NULLSTR(elt->backingStoreRaw), NULLSTR(elt->directory), elt->backingStoreFormat, elt->backingStoreIsFile, - elt->capacity, elt->encrypted) < 0) { + elt->capacity, !!elt->encryption) < 0) { VIR_FREE(expect); VIR_FREE(actual); goto cleanup; -- 1.9.0

On 04/02/14 05:05, Eric Blake wrote:
A future patch will merge virStorageFileMetadata and virStorageSource, but I found it easier to do if both structs use the same information for tracking whether a source file needs encryption keys.
* src/util/virstoragefile.h (_virStorageFileMetadata): Prepare full encryption struct instead of just a bool. * src/storage/storage_backend_fs.c (virStorageBackendProbeTarget): Use transfer semantics. * src/storage/storage_backend_gluster.c (virStorageBackendGlusterRefreshVol): Likewise. * src/util/virstoragefile.c (virStorageFileGetMetadataInternal): Populate struct. (virStorageFileFreeMetadata): Adjust clients. * tests/virstoragetest.c (testStorageChain): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/storage/storage_backend_fs.c | 6 +++--- src/storage/storage_backend_gluster.c | 6 +++--- src/util/virstoragefile.c | 4 +++- src/util/virstoragefile.h | 3 ++- tests/virstoragetest.c | 4 ++-- 5 files changed, 13 insertions(+), 10 deletions(-)
ACK

--- It's late for me, but I wanted to throw this out there to make sure I'm on the right track. My premise here is that the current virStorageFileMetadata is awkward to work with, because you have to ask the parent for details about the child, instead of directly storing those details in the child. So this patch proposes a change to the metadata struct to make it possible to track details in the right struct. Then I would have a series of patches to make metadata generation populate the duplicate information as documented in the comments here, followed by teaching all clients of the populated data to grab the information from the right struct instead of the parent. At which point, I'd be ready for RFC 9/7... src/util/virstoragefile.c | 4 ++++ src/util/virstoragefile.h | 35 ++++++++++++++++++++++++++++++----- 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index e6a985d..5d5adb4 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1204,6 +1204,10 @@ virStorageFileFreeMetadata(virStorageFileMetadata *meta) if (!meta) return; + VIR_FREE(meta->path); + VIR_FREE(meta->canonName); + VIR_FREE(meta->relDir); + virStorageFileFreeMetadata(meta->backingMeta); VIR_FREE(meta->backingStore); VIR_FREE(meta->backingStoreRaw); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 56105d0..da90374 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -108,17 +108,42 @@ struct _virStorageTimestamps { typedef struct _virStorageFileMetadata virStorageFileMetadata; typedef virStorageFileMetadata *virStorageFileMetadataPtr; struct _virStorageFileMetadata { - char *backingStore; /* Canonical name (absolute file, or protocol) */ - char *backingStoreRaw; /* If file, original name, possibly relative */ - char *directory; /* The directory containing basename of backingStoreRaw */ - int backingStoreFormat; /* enum virStorageFileFormat */ - bool backingStoreIsFile; + /* Name of this file as spelled by the user (top level) or + * metadata of the parent (if this is a backing store). */ + char *path; + /* Canonical name of this file, used to detect loops in the + * backing store chain. */ + char *canonName; + /* Directory to start from if backingStoreRaw is a relative file + * name */ + char *relDir; + /* Name of the backing store recorded in metadata of the parent */ + char *backingStoreRaw; + + /* Backing chain. backingMeta->origName should match + * backingStoreRaw; but the fields are duplicated in the common + * case to make it possible to detect missing backing files, as + * follows: if backingStoreRaw is NULL, this should be NULL. If + * this is NULL and backingStoreRaw is non-NULL, there was an + * error following the chain (such as missing file). Otherwise, + * information about the backing store is here. */ virStorageFileMetadataPtr backingMeta; + /* Details about the current image */ + int type; /* enum virStorageType */ + int format; /* enum virStorageFileFormat */ virStorageEncryptionPtr encryption; unsigned long long capacity; virBitmapPtr features; /* bits described by enum virStorageFileFeature */ char *compat; + + /* Fields I'm trying to delete, because it is confusing to have to + * query the parent metadata for details about the backing + * store. */ + char *backingStore; /* Canonical name (absolute file, or protocol). Should be same as backingMeta->canon */ + char *directory; /* The directory containing basename of backingStoreRaw. Should be same as backingMeta->relDir */ + int backingStoreFormat; /* enum virStorageFileFormat. Should be same as backingMeta->format */ + bool backingStoreIsFile; /* Should be same as backingMeta->type < VIR_STORAGE_TYPE_NETWORK */ }; -- 1.9.0

--- ...continuing my thoughts from 8/7. These are the additional fields required in virStorageSource to copy all fields directly listed in virStorageFileMetadata that are not already rendered redundant, as mentioned in those comments. By populating these fields directly in virStorageSource, I can follow up with some patches to s/virStorageFileMetadata/virStorageSource/, and finally have one struct in use. From there, I now have the situation that following the backing chain in order to do SELinux labeling also leaves the chain populated with the details I need for enhanced XML output of domain <disk> information for a live guest. src/util/virstoragefile.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index da90374..440504b 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -238,7 +238,27 @@ typedef virStorageSource *virStorageSourcePtr; * view. */ struct _virStorageSource { int type; /* enum virStorageType */ + + /* Name of this source, as spelled by the user (for active layer) + * or by metadata of the parent file (for backing store), exposed + * in xml */ char *path; + /* Canonical name of this file, if on local file system; used to + * detect loops in the backing store chain. Not in xml */ + char *canonName; + /* Directory to start from if backingStoreRaw is a relative file + * name. Not in xml */ + char *relDir; + /* Name of backing store recorded in metadata. Not in xml for this + * element, but duplicated in backingMeta->path where it is in xml + * for the child */ + char *backingStoreRaw; + /* Backing store for this element. If backingStoreRaw is non-NULL + * but this is NULL, there was an error following the chain; + * otherwise, the two fields should both be NULL or both be + * non-NULL. */ + virStorageSourcePtr backingMeta; + int protocol; /* enum virStorageNetProtocol */ size_t nhosts; virStorageNetHostDefPtr hosts; -- 1.9.0
participants (3)
-
Eric Blake
-
John Ferlan
-
Peter Krempa