[libvirt] [PATCHv2 6/7] conf: drop redundant parameters during probe
John Ferlan
jferlan at redhat.com
Wed Apr 23 13:18:22 UTC 2014
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 at 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'"),
>
More information about the libvir-list
mailing list