Re: [libvirt] [PATCH v5 4/9] virstoragefile: Always use virStorageSourceSetBackingStore to set backing store
On Mon, May 4, 2015 at 7:21 PM, John Ferlan wrote:
>
>
> On 04/23/2015 08:41 AM, Matthias Gatto wrote:
>> Replace the parts of the code where a backing store is set manually
>> with virStorageSourceSetBackingStore
>>
>> Signed-off-by: Matthias Gatto
>> Signed-off-by: John Ferlan
>> ---
>> src/conf/domain_conf.c| 3 ++-
>> src/conf/storage_conf.c | 17 ++---
>> src/qemu/qemu_driver.c| 17 +++--
>> src/storage/storage_backend_fs.c | 7 +--
>> src/storage/storage_backend_gluster.c | 5 +++--
>> src/storage/storage_backend_logical.c | 9 ++---
>> src/storage/storage_driver.c | 3 ++-
>> src/util/virstoragefile.c | 8 +---
>> tests/virstoragetest.c| 4 ++--
>> 9 files changed, 46 insertions(+), 27 deletions(-)
>>
>
> Other than a minor goto error issue in storage_backend_gluster.c - up
> through here things seem fine to me. Doesn't seem to be any new readers
> or setters of "->backingStore" in recent changes (since patches 5-9
> compile too). The only references are now ->backingStoreRaw fetches and
> saves.
>
> However, for patches 5-9 as I understand it have some concerns from
> Peter which hopefully he can elaborate on.
>
> So that progress can be made and readers/writers of backingStore go
> through the virStorageSource{Get|Set}BackingStore API's until the rest
> of the concerns are understood - I'm of the opinion the patches 1-4 can
> be pushed, but I'll wait until Peter chimes in before doing so...
>
> John
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 2a05291..09f0bca 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -6006,7 +6006,8 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt,
>> virDomainDiskBackingStoreParse(ctxt, backingStore) < 0)
>> goto cleanup;
>>
>> -src->backingStore = backingStore;
>> +if (!virStorageSourceSetBackingStore(src, backingStore, 0))
>> +goto cleanup;
>> ret = 0;
>>
>> cleanup:
>> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
>> index 5781374..ca3a6d5 100644
>> --- a/src/conf/storage_conf.c
>> +++ b/src/conf/storage_conf.c
>> @@ -1254,6 +1254,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
>> char *capacity = NULL;
>> char *unit = NULL;
>> char *backingStore = NULL;
>> +virStorageSourcePtr backingStorePtr;
>> xmlNodePtr node;
>> xmlNodePtr *nodes = NULL;
>> size_t i;
>> @@ -1290,20 +1291,22 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
>> }
>>
>> if ((backingStore = virXPathString("string(./backingStore/path)",
>> ctxt))) {
>> -if (VIR_ALLOC(ret->target.backingStore) < 0)
>> +if (VIR_ALLOC(backingStorePtr) < 0)
>> goto error;
>>
>> -ret->target.backingStore->path = backingStore;
>> +if (!virStorageSourceSetBackingStore(&ret->target, backingStorePtr,
>> 0))
>> +goto error;
>> +backingStorePtr->path = backingStore;
>> backingStore = NULL;
>>
>> if (options->formatFromString) {
>> char *format =
>> virXPathString("string(./backingStore/format/@type)", ctxt);
>> if (format == NULL)
>> -ret->target.backingStore->format = options->defaultFormat;
>> +backingStorePtr->format = options->defaultFormat;
>> else
>> -ret->target.backingStore->format =
>> (options->formatFromString)(format);
>> +backingStorePtr->format =
>> (options->formatFromString)(format);
>>
>> -if (ret->target.backingStore->format < 0) {
>> +if (backingStorePtr->format < 0) {
>> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> _("unknown volume format type %s"), format);
>> VIR_FREE(format);
>> @@ -1312,9 +1315,9 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
>> VIR_FREE(format);
>> }
>>
>> -if (VIR_ALLOC(ret->target.backingStore->perms) < 0)
>> +if (VIR_ALLOC(backingStorePtr->perms) < 0)
>> goto error;
>> -if (virStorageDefParsePerms(ctxt, ret->target.backingStore->perms,
>> +if (virStorageDefParsePerms(ctxt, backingStorePtr->perms,
>> "./backingStore/permissions",
>> DEFAULT_VOL_PERM_MODE) < 0)
>> goto error;
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 26be9d6..1a98601 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -14274,13 +14274,18 @@
>> qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
>> /* Update vm in place to match changes. */
>> need_unlink = false;
>>
>> -newDiskSrc->backingStore = disk->src;
>> -disk->src = newDiskSrc;
>> +if (!virStorageSourceSetBackingStore(newDis
Re: [libvirt] [PATCH v5 4/9] virstoragefile: Always use virStorageSourceSetBackingStore to set backing store
On 04/23/2015 08:41 AM, Matthias Gatto wrote:
> Replace the parts of the code where a backing store is set manually
> with virStorageSourceSetBackingStore
>
> Signed-off-by: Matthias Gatto
> Signed-off-by: John Ferlan
> ---
> src/conf/domain_conf.c| 3 ++-
> src/conf/storage_conf.c | 17 ++---
> src/qemu/qemu_driver.c| 17 +++--
> src/storage/storage_backend_fs.c | 7 +--
> src/storage/storage_backend_gluster.c | 5 +++--
> src/storage/storage_backend_logical.c | 9 ++---
> src/storage/storage_driver.c | 3 ++-
> src/util/virstoragefile.c | 8 +---
> tests/virstoragetest.c| 4 ++--
> 9 files changed, 46 insertions(+), 27 deletions(-)
>
Other than a minor goto error issue in storage_backend_gluster.c - up
through here things seem fine to me. Doesn't seem to be any new readers
or setters of "->backingStore" in recent changes (since patches 5-9
compile too). The only references are now ->backingStoreRaw fetches and
saves.
However, for patches 5-9 as I understand it have some concerns from
Peter which hopefully he can elaborate on.
So that progress can be made and readers/writers of backingStore go
through the virStorageSource{Get|Set}BackingStore API's until the rest
of the concerns are understood - I'm of the opinion the patches 1-4 can
be pushed, but I'll wait until Peter chimes in before doing so...
John
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 2a05291..09f0bca 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -6006,7 +6006,8 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt,
> virDomainDiskBackingStoreParse(ctxt, backingStore) < 0)
> goto cleanup;
>
> -src->backingStore = backingStore;
> +if (!virStorageSourceSetBackingStore(src, backingStore, 0))
> +goto cleanup;
> ret = 0;
>
> cleanup:
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 5781374..ca3a6d5 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -1254,6 +1254,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
> char *capacity = NULL;
> char *unit = NULL;
> char *backingStore = NULL;
> +virStorageSourcePtr backingStorePtr;
> xmlNodePtr node;
> xmlNodePtr *nodes = NULL;
> size_t i;
> @@ -1290,20 +1291,22 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
> }
>
> if ((backingStore = virXPathString("string(./backingStore/path)",
> ctxt))) {
> -if (VIR_ALLOC(ret->target.backingStore) < 0)
> +if (VIR_ALLOC(backingStorePtr) < 0)
> goto error;
>
> -ret->target.backingStore->path = backingStore;
> +if (!virStorageSourceSetBackingStore(&ret->target, backingStorePtr,
> 0))
> +goto error;
> +backingStorePtr->path = backingStore;
> backingStore = NULL;
>
> if (options->formatFromString) {
> char *format =
> virXPathString("string(./backingStore/format/@type)", ctxt);
> if (format == NULL)
> -ret->target.backingStore->format = options->defaultFormat;
> +backingStorePtr->format = options->defaultFormat;
> else
> -ret->target.backingStore->format =
> (options->formatFromString)(format);
> +backingStorePtr->format =
> (options->formatFromString)(format);
>
> -if (ret->target.backingStore->format < 0) {
> +if (backingStorePtr->format < 0) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("unknown volume format type %s"), format);
> VIR_FREE(format);
> @@ -1312,9 +1315,9 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
> VIR_FREE(format);
> }
>
> -if (VIR_ALLOC(ret->target.backingStore->perms) < 0)
> +if (VIR_ALLOC(backingStorePtr->perms) < 0)
> goto error;
> -if (virStorageDefParsePerms(ctxt, ret->target.backingStore->perms,
> +if (virStorageDefParsePerms(ctxt, backingStorePtr->perms,
> "./backingStore/permissions",
> DEFAULT_VOL_PERM_MODE) < 0)
> goto error;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 26be9d6..1a98601 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -14274,13 +14274,18 @@
> qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
> /* Update vm in place to match changes. */
> need_unlink = false;
>
> -newDiskSrc->backingStore = disk->src;
> -disk->src = newDiskSrc;
> +if (!virStorageSourceSetBackingStore(newDiskSrc, disk->src, 0)) {
> +ret = -1;
> +goto cleanup;
> +}
> newDiskSrc = NULL;
>
> if (persistDisk) {
> -persistDiskSrc->backi
[libvirt] [PATCH v5 4/9] virstoragefile: Always use virStorageSourceSetBackingStore to set backing store
Replace the parts of the code where a backing store is set manually
with virStorageSourceSetBackingStore
Signed-off-by: Matthias Gatto
Signed-off-by: John Ferlan
---
src/conf/domain_conf.c| 3 ++-
src/conf/storage_conf.c | 17 ++---
src/qemu/qemu_driver.c| 17 +++--
src/storage/storage_backend_fs.c | 7 +--
src/storage/storage_backend_gluster.c | 5 +++--
src/storage/storage_backend_logical.c | 9 ++---
src/storage/storage_driver.c | 3 ++-
src/util/virstoragefile.c | 8 +---
tests/virstoragetest.c| 4 ++--
9 files changed, 46 insertions(+), 27 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 2a05291..09f0bca 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6006,7 +6006,8 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt,
virDomainDiskBackingStoreParse(ctxt, backingStore) < 0)
goto cleanup;
-src->backingStore = backingStore;
+if (!virStorageSourceSetBackingStore(src, backingStore, 0))
+goto cleanup;
ret = 0;
cleanup:
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 5781374..ca3a6d5 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -1254,6 +1254,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
char *capacity = NULL;
char *unit = NULL;
char *backingStore = NULL;
+virStorageSourcePtr backingStorePtr;
xmlNodePtr node;
xmlNodePtr *nodes = NULL;
size_t i;
@@ -1290,20 +1291,22 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
}
if ((backingStore = virXPathString("string(./backingStore/path)", ctxt))) {
-if (VIR_ALLOC(ret->target.backingStore) < 0)
+if (VIR_ALLOC(backingStorePtr) < 0)
goto error;
-ret->target.backingStore->path = backingStore;
+if (!virStorageSourceSetBackingStore(&ret->target, backingStorePtr, 0))
+goto error;
+backingStorePtr->path = backingStore;
backingStore = NULL;
if (options->formatFromString) {
char *format =
virXPathString("string(./backingStore/format/@type)", ctxt);
if (format == NULL)
-ret->target.backingStore->format = options->defaultFormat;
+backingStorePtr->format = options->defaultFormat;
else
-ret->target.backingStore->format =
(options->formatFromString)(format);
+backingStorePtr->format = (options->formatFromString)(format);
-if (ret->target.backingStore->format < 0) {
+if (backingStorePtr->format < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("unknown volume format type %s"), format);
VIR_FREE(format);
@@ -1312,9 +1315,9 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
VIR_FREE(format);
}
-if (VIR_ALLOC(ret->target.backingStore->perms) < 0)
+if (VIR_ALLOC(backingStorePtr->perms) < 0)
goto error;
-if (virStorageDefParsePerms(ctxt, ret->target.backingStore->perms,
+if (virStorageDefParsePerms(ctxt, backingStorePtr->perms,
"./backingStore/permissions",
DEFAULT_VOL_PERM_MODE) < 0)
goto error;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 26be9d6..1a98601 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14274,13 +14274,18 @@
qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
/* Update vm in place to match changes. */
need_unlink = false;
-newDiskSrc->backingStore = disk->src;
-disk->src = newDiskSrc;
+if (!virStorageSourceSetBackingStore(newDiskSrc, disk->src, 0)) {
+ret = -1;
+goto cleanup;
+}
newDiskSrc = NULL;
if (persistDisk) {
-persistDiskSrc->backingStore = persistDisk->src;
-persistDisk->src = persistDiskSrc;
+if (!virStorageSourceSetBackingStore(persistDiskSrc,
+ persistDisk->src, 0)) {
+ret = -1;
+goto cleanup;
+}
persistDiskSrc = NULL;
}
@@ -14323,13 +14328,13 @@
qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver,
/* Update vm in place to match changes. */
tmp = disk->src;
disk->src = virStorageSourceGetBackingStore(tmp, 0);
-tmp->backingStore = NULL;
+ignore_value(virStorageSourceSetBackingStore(tmp, NULL, 0));
virStorageSourceFree(tmp);
if (persistDisk) {
tmp = persistDisk->src;
persistDisk->src = virStorageSourceGetBackingStore(tmp, 0);
-tmp->backingStore = NULL;
+ignore_value(virStorageSourceSetBackingStore(tmp, NULL, 0));
virStorageSourceFree(tmp);
}
}
diff --git a/src/st
