Re: [libvirt] [PATCH 18/30] storagefile: Add externalDataStore member
On 10/11/19 10:04 AM, Michal Privoznik wrote:
On 10/10/19 5:09 PM, Daniel Henrique Barboza wrote:
On 10/7/19 6:49 PM, Cole Robinson wrote:
Add the plumbing to track a externalDataStoreRaw as a virStorageSource
Signed-off-by: Cole Robinson
---
src/util/virstoragefile.c | 9 +
src/util/virstoragefile.h | 3 +++
2 files changed, 12 insertions(+)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 7ae6719dd6..ce669b6e0b 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2339,6 +2339,12 @@ virStorageSourceCopy(const virStorageSource
*src,
return NULL;
}
+ if (src->externalDataStore) {
+ if (!(def->externalDataStore =
virStorageSourceCopy(src->externalDataStore,
+ true)))
+ return NULL;
+ }
+
VIR_STEAL_PTR(ret, def);
return ret;
}
@@ -2560,6 +2566,9 @@ virStorageSourceClear(virStorageSourcePtr def)
VIR_FREE(def->timestamps);
VIR_FREE(def->externalDataStoreRaw);
+ virObjectUnref(def->externalDataStore);
+ def->externalDataStore = NULL;
Is this assign to NULL necessary? virObjectUnref will call VIR_FREE() in
def->externalDataStore if there is no more references to it (which will
assign it to NULL in the end).
It is. Point of virXXXClear() functions is to clear passed struct.
That is, in theory (I'm not saying it's the case of
virStorageSourceDef and also I'm not saying it isn't), it should be
possible to re-use a structure after it's been cleared. But for that
to happen, all poitners must be reset to NULL regardless of
refcounters and stuff.
Got it. I didn't get across an example of this re-use yet, didn't know
it is a
valid possibility.
However, there's a memset() called at the end of this function which
sets all members of this struct to 0, so in the end I agree that the
line is redundant, but for a different reason :-D
I was right in the end! That all I'm going to remember ;)
DHB
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 18/30] storagefile: Add externalDataStore member
On 10/10/19 5:09 PM, Daniel Henrique Barboza wrote:
On 10/7/19 6:49 PM, Cole Robinson wrote:
Add the plumbing to track a externalDataStoreRaw as a virStorageSource
Signed-off-by: Cole Robinson
---
src/util/virstoragefile.c | 9 +
src/util/virstoragefile.h | 3 +++
2 files changed, 12 insertions(+)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 7ae6719dd6..ce669b6e0b 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2339,6 +2339,12 @@ virStorageSourceCopy(const virStorageSource *src,
return NULL;
}
+ if (src->externalDataStore) {
+ if (!(def->externalDataStore =
virStorageSourceCopy(src->externalDataStore,
+ true)))
+ return NULL;
+ }
+
VIR_STEAL_PTR(ret, def);
return ret;
}
@@ -2560,6 +2566,9 @@ virStorageSourceClear(virStorageSourcePtr def)
VIR_FREE(def->timestamps);
VIR_FREE(def->externalDataStoreRaw);
+ virObjectUnref(def->externalDataStore);
+ def->externalDataStore = NULL;
Is this assign to NULL necessary? virObjectUnref will call VIR_FREE() in
def->externalDataStore if there is no more references to it (which will
assign it to NULL in the end).
It is. Point of virXXXClear() functions is to clear passed struct. That
is, in theory (I'm not saying it's the case of virStorageSourceDef and
also I'm not saying it isn't), it should be possible to re-use a
structure after it's been cleared. But for that to happen, all poitners
must be reset to NULL regardless of refcounters and stuff.
However, there's a memset() called at the end of this function which
sets all members of this struct to 0, so in the end I agree that the
line is redundant, but for a different reason :-D
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 18/30] storagefile: Add externalDataStore member
On 10/7/19 6:49 PM, Cole Robinson wrote:
Add the plumbing to track a externalDataStoreRaw as a virStorageSource
Signed-off-by: Cole Robinson
---
src/util/virstoragefile.c | 9 +
src/util/virstoragefile.h | 3 +++
2 files changed, 12 insertions(+)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 7ae6719dd6..ce669b6e0b 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2339,6 +2339,12 @@ virStorageSourceCopy(const virStorageSource *src,
return NULL;
}
+if (src->externalDataStore) {
+if (!(def->externalDataStore =
virStorageSourceCopy(src->externalDataStore,
+true)))
+return NULL;
+}
+
VIR_STEAL_PTR(ret, def);
return ret;
}
@@ -2560,6 +2566,9 @@ virStorageSourceClear(virStorageSourcePtr def)
VIR_FREE(def->timestamps);
VIR_FREE(def->externalDataStoreRaw);
+virObjectUnref(def->externalDataStore);
+def->externalDataStore = NULL;
Is this assign to NULL necessary? virObjectUnref will call VIR_FREE() in
def->externalDataStore if there is no more references to it (which will
assign it to NULL in the end).
LGTM otherwise
Reviewed-by: Daniel Henrique Barboza
+
virStorageNetHostDefFree(def->nhosts, def->hosts);
virStorageAuthDefFree(def->auth);
virObjectUnref(def->privateData);
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index bbff511657..d84dad052d 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -292,6 +292,9 @@ struct _virStorageSource {
/* backing chain of the storage source */
virStorageSourcePtr backingStore;
+/* external data store storage source */
+virStorageSourcePtr externalDataStore;
+
/* metadata for storage driver access to remote and local volumes */
virStorageDriverDataPtr drv;
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 18/30] storagefile: Add externalDataStore member
Add the plumbing to track a externalDataStoreRaw as a virStorageSource
Signed-off-by: Cole Robinson
---
src/util/virstoragefile.c | 9 +
src/util/virstoragefile.h | 3 +++
2 files changed, 12 insertions(+)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 7ae6719dd6..ce669b6e0b 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2339,6 +2339,12 @@ virStorageSourceCopy(const virStorageSource *src,
return NULL;
}
+if (src->externalDataStore) {
+if (!(def->externalDataStore =
virStorageSourceCopy(src->externalDataStore,
+true)))
+return NULL;
+}
+
VIR_STEAL_PTR(ret, def);
return ret;
}
@@ -2560,6 +2566,9 @@ virStorageSourceClear(virStorageSourcePtr def)
VIR_FREE(def->timestamps);
VIR_FREE(def->externalDataStoreRaw);
+virObjectUnref(def->externalDataStore);
+def->externalDataStore = NULL;
+
virStorageNetHostDefFree(def->nhosts, def->hosts);
virStorageAuthDefFree(def->auth);
virObjectUnref(def->privateData);
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index bbff511657..d84dad052d 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -292,6 +292,9 @@ struct _virStorageSource {
/* backing chain of the storage source */
virStorageSourcePtr backingStore;
+/* external data store storage source */
+virStorageSourcePtr externalDataStore;
+
/* metadata for storage driver access to remote and local volumes */
virStorageDriverDataPtr drv;
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
