On Thu, Oct 29, 2015 at 14:43:09 +0100, Matthias Gatto wrote:
> Uniformize backing store usage by calling virStorageSourceGetBackingStore
> instead of setting backing store manually.
>
> Signed-off-by: Matthias Gatto <matthias.gatto@xxxxxxxxxxxx>
> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
> ---
> src/conf/domain_conf.c | 7 ++++---
> src/conf/storage_conf.c | 6 +++---
> src/qemu/qemu_cgroup.c | 4 ++--
> src/qemu/qemu_domain.c | 2 +-
> src/qemu/qemu_driver.c | 18 ++++++++++--------
> src/qemu/qemu_monitor_json.c | 4 +++-
> src/security/security_dac.c | 2 +-
> src/security/security_selinux.c | 4 ++--
> src/security/virt-aa-helper.c | 2 +-
> src/storage/storage_backend.c | 20 ++++++++++++--------
> src/storage/storage_backend_fs.c | 31 +++++++++++++++++--------------
> src/storage/storage_backend_gluster.c | 8 +++++---
> src/storage/storage_backend_logical.c | 10 ++++++----
> src/util/virstoragefile.c | 20 ++++++++++----------
> tests/virstoragetest.c | 14 +++++++-------
> 15 files changed, 84 insertions(+), 68 deletions(-)
>
Most of the patch is adding very long lines. Our coding standards state
that the code should not exceed 80 columns. We still try to stick with
this rule although monitors got wider.
I'm not going to point out all the places here, but please try to limit
the line length.
[...]
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 890d8ed..1298e44 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2968,7 +2968,7 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
> if (virStorageSourceIsEmpty(disk->src))
> goto cleanup;
>
> - if (disk->src->backingStore) {
> + if (virStorageSourceGetBackingStore(disk->src, 0)) {
> if (force_probe)
> virStorageSourceBackingStoreClear(disk->src);
Side note:
After you introduce quorums, this function will have to traverse backing
chains for all members of the quorum. I didn't go through all of the
patches though so let's see if you changed it later.
> else
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 80b0886..23f8e8a 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -1158,7 +1158,7 @@ virSecuritySELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr,
> * be tracked in domain XML, at which point labelskip should be a
> * per-file attribute instead of a disk attribute. */
> if (disk_seclabel && disk_seclabel->labelskip &&
> - !src->backingStore)
> + !virStorageSourceGetBackingStore(src, 0))
> return 0;
Same side note as above ...
>
> /* Don't restore labels on readonly/shared disks, because other VMs may
> @@ -1292,7 +1292,7 @@ virSecuritySELinuxSetSecurityDiskLabel(virSecurityManagerPtr mgr,
> bool first = true;
> virStorageSourcePtr next;
>
> - for (next = disk->src; next; next = next->backingStore) {
> + for (next = disk->src; next; next = virStorageSourceGetBackingStore(next, 0)) {
> if (virSecuritySELinuxSetSecurityImageLabelInternal(mgr, def, next,
> first) < 0)
> return -1;
aaand here too.
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index a375fe0..3b504e9 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
[...]
> @@ -973,8 +975,10 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn,
> * backing store, not really sure what use it serves though, and it
> * may cause issues with lvm. Untested essentially.
> */
> - if (inputvol && inputvol->target.backingStore &&
> - STRNEQ_NULLABLE(inputvol->target.backingStore->path, info.backingPath)) {
> + if (inputvol &&
> + virStorageSourceGetBackingStore(&inputvol->target, 0) &&
> + STRNEQ_NULLABLE(virStorageSourceGetBackingStore(&inputvol->target, 0)->path,
> + info.backingPath)) {
looks like a temp variable could make the code more clear
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("a different backing store cannot be specified."));
> return NULL;
[...]
> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
> index 99ea394..cb33ac0 100644
> --- a/src/storage/storage_backend_fs.c
> +++ b/src/storage/storage_backend_fs.c
[...]
> @@ -99,37 +100,39 @@ virStorageBackendProbeTarget(virStorageSourcePtr target,
> if (!(target->backingStore = virStorageSourceNewFromBacking(meta)))
> goto cleanup;
>
> - target->backingStore->format = backingStoreFormat;
> + backingStore = virStorageSourceGetBackingStore(target, 0);
> + backingStore->format = backingStoreFormat;
>
> /* XXX: Remote storage doesn't play nicely with volumes backed by
> * remote storage. To avoid trouble, just fake the backing store is RAW
> * and put the string from the metadata as the path of the target. */
> - if (!virStorageSourceIsLocalStorage(target->backingStore)) {
> - virStorageSourceFree(target->backingStore);
> + if (!virStorageSourceIsLocalStorage(backingStore)) {
> + virStorageSourceFree(backingStore);
So this frees the new backingStore variable, which also corresponds to
target->backingStore at this point, but ...
>
> - if (VIR_ALLOC(target->backingStore) < 0)
> + if (VIR_ALLOC(backingStore) < 0)
... here only the local copy is allocated, so target->backingStore
contains an invalid pointer ...
> goto cleanup;
>
> - target->backingStore->type = VIR_STORAGE_TYPE_NETWORK;
> - target->backingStore->path = meta->backingStoreRaw;
> + target->backingStore = backingStore;
> + backingStore->type = VIR_STORAGE_TYPE_NETWORK;
> + backingStore->path = meta->backingStoreRaw;
> meta->backingStoreRaw = NULL;
> - target->backingStore->format = VIR_STORAGE_FILE_RAW;
> + backingStore->format = VIR_STORAGE_FILE_RAW;
> }
>
> - if (target->backingStore->format == VIR_STORAGE_FILE_AUTO) {
> - if ((rc = virStorageFileProbeFormat(target->backingStore->path,
> + if (backingStore->format == VIR_STORAGE_FILE_AUTO) {
> + if ((rc = virStorageFileProbeFormat(backingStore->path,
> -1, -1)) < 0) {
> /* If the backing file is currently unavailable or is
> * accessed via remote protocol only log an error, fake the
> * format as RAW and continue. Returning -1 here would
> * disable the whole storage pool, making it unavailable for
> * even maintenance. */
> - target->backingStore->format = VIR_STORAGE_FILE_RAW;
> + backingStore->format = VIR_STORAGE_FILE_RAW;
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("cannot probe backing volume format: %s"),
> - target->backingStore->path);
> + backingStore->path);
> } else {
> - target->backingStore->format = rc;
> + backingStore->format = rc;
... and I couldn't find a place where you update it back.
> }
> }
> }
[...]
> diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c
> index 536e617..e7e4e1d 100644
> --- a/src/storage/storage_backend_logical.c
> +++ b/src/storage/storage_backend_logical.c
> @@ -151,11 +151,11 @@ virStorageBackendLogicalMakeVol(char **const groups,
> if (VIR_ALLOC(vol->target.backingStore) < 0)
> goto cleanup;
>
> - if (virAsprintf(&vol->target.backingStore->path, "%s/%s",
> + if (virAsprintf(&virStorageSourceGetBackingStore(&vol->target, 0)->path, "%s/%s",
> pool->def->target.path, groups[1]) < 0)
> goto cleanup;
>
> - vol->target.backingStore->format = VIR_STORAGE_POOL_LOGICAL_LVM2;
> + virStorageSourceGetBackingStore(&vol->target, 0)->format = VIR_STORAGE_POOL_LOGICAL_LVM2;
This is rather ugly. You are using the function twice here and always
dereferencing it's return value. It'd be nicer just to add a temp
variable and use that.
> }
>
> if (!vol->key && VIR_STRDUP(vol->key, groups[2]) < 0)
Peter
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list