Re: [libvirt] [PATCH v6 02/13] virstoragefile: Always use virStorageSourceGetBackingStore to get backing store
On Mon, Nov 2, 2015 at 8:42 AM, Peter Krempa wrote:
>
>> @@ -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.
>> -target->backingStore->type = VIR_STORAGE_TYPE_NETWORK;
>> -target->backingStore->path = meta->backingStoreRaw;
>> +target->backingStore = backingStore;
I do it here.
Still, you have a good point about temp variables, and long line,
I'm working on this now.
Thank you for the review.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v6 02/13] virstoragefile: Always use virStorageSourceGetBackingStore to get backing store
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
> Signed-off-by: John Ferlan
> ---
> 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);
>
[libvirt] [PATCH v6 02/13] virstoragefile: Always use virStorageSourceGetBackingStore to get backing store
Uniformize backing store usage by calling virStorageSourceGetBackingStore
instead of setting backing store manually.
Signed-off-by: Matthias Gatto
Signed-off-by: John Ferlan
---
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(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9a0c7fc..e0befc3 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -18897,7 +18897,7 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf,
/* We currently don't output seclabels for backing chain element */
if (virDomainDiskSourceFormatInternal(buf, backingStore, 0, 0, true) < 0 ||
virDomainDiskBackingStoreFormat(buf,
-backingStore->backingStore,
+
virStorageSourceGetBackingStore(backingStore, 0),
backingStore->backingStoreRaw,
idx + 1) < 0)
return -1;
@@ -19018,7 +19018,8 @@ virDomainDiskDefFormat(virBufferPtr buf,
/* Don't format backingStore to inactive XMLs until the code for
* persistent storage of backing chains is ready. */
if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE) &&
-virDomainDiskBackingStoreFormat(buf, def->src->backingStore,
+virDomainDiskBackingStoreFormat(buf,
+
virStorageSourceGetBackingStore(def->src, 0),
def->src->backingStoreRaw, 1) < 0)
return -1;
@@ -23317,7 +23318,7 @@ virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
}
}
-for (tmp = disk->src; tmp; tmp = tmp->backingStore) {
+for (tmp = disk->src; tmp; tmp = virStorageSourceGetBackingStore(tmp, 0)) {
int actualType = virStorageSourceGetActualType(tmp);
/* execute the callback only for local storage */
if (actualType != VIR_STORAGE_TYPE_NETWORK &&
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 9b8abea..d048e39 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -1330,7 +1330,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
if (virStorageSize(unit, capacity, &ret->target.capacity) < 0)
goto error;
} else if (!(flags & VIR_VOL_XML_PARSE_NO_CAPACITY) &&
- !((flags & VIR_VOL_XML_PARSE_OPT_CAPACITY) &&
ret->target.backingStore)) {
+ !((flags & VIR_VOL_XML_PARSE_OPT_CAPACITY) &&
virStorageSourceGetBackingStore(&ret->target, 0))) {
virReportError(VIR_ERR_XML_ERROR, "%s", _("missing capacity element"));
goto error;
}
@@ -1644,9 +1644,9 @@ virStorageVolDefFormat(virStoragePoolDefPtr pool,
&def->target, "target") < 0)
goto cleanup;
-if (def->target.backingStore &&
+if (virStorageSourceGetBackingStore(&def->target, 0) &&
virStorageVolTargetDefFormat(options, &buf,
- def->target.backingStore,
+
virStorageSourceGetBackingStore(&def->target, 0),
"backingStore") < 0)
goto cleanup;
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index a8e0b8c..f93782b 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -121,7 +121,7 @@ qemuSetupDiskCgroup(virDomainObjPtr vm,
virStorageSourcePtr next;
bool forceReadonly = false;
-for (next = disk->src; next; next = next->backingStore) {
+for (next = disk->src; next; next = virStorageSourceGetBackingStore(next,
0)) {
if (qemuSetImageCgroupInternal(vm, next, false, forceReadonly) < 0)
return -1;
@@ -139,7 +139,7 @@ qemuTeardownDiskCgroup(virDomainObjPtr vm,
{
virStorageSourcePtr next;
-for (next = disk->src; next; next = next->backingStore) {
+for (next = disk->src; next; next = virStorageSourceGetBackingStore(next,
0)) {
if (qemuSetImageCgroup(vm, next, true) < 0)
return -1;
}
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/q
