10:35 p.m.
The purpose of these patches is to introduce quorum for libvirt
I've try to follow this proposal:
http://www.redhat.com/archives/libvir-list/2014-May/msg00533.html
This feature ask for 5 task:
1) Allow a _virStorageSource to contain more than one backing store.
Because all the actual libvirt code use the backingStore field
as a pointer and we needs want to change that, I've decide to encapsulate
the backingStore field to simplifie the array manipulation.
2) Add the missing field a quorum need in _virStorageSource and
the VIR_STORAGE_TYPE_QUORUM and VIR_STORAGE_FILE_QUORUM in
their respectives enums.
3) Parse and format the xml
Because a quorum allows to have more than one backing store at the same level
we need to change virDomainDiskDefFormat and virDomainDiskDefParseXML
to call virDomainDiskBackingStoreFormat and virDomainDiskBackingStoreParse
in a loop.
virDomainDiskBackingStoreFormat and virDomainDiskBackingStoreParse can
call themself recursively in a loop because a quorum can contain another
quorum
4) Build qemu string
As for the xml, we have to call the function which create quorum recursively.
But this task have the problem explained here:
http://www.redhat.com/archives/libvir-list/2014-October/msg00529.html
The _virStorageSource missing some informations that can be passed to
a child, and therefore this version of quorum is incomplet.
5) Allow to hotplug/change a disk in a quorum
This part is not present in these patches because for this task
we have to use blockdev-add, and currently libvirt use
device_add for hotpluging that doesn't allow to hotplug quorum childs.
V2:
-Rebase on master
-Add Documentation
V3:
-Transforme the backingStore field in virStorageSource into
an array of pointer instead of a pointer
-Modify virStorageSourceSetBackingStore to allow it to expand
the backingStore size.
V4:
-Rebase on master
V5:
-Rebase on master
-patch 1-4/9: use patchs from John Ferlan
-patch 4/9: check return of virStorageSourceSetBackingStore
-patch 5/9: report type of error on virStorageSourceSetBackingStore
v6:
-Rebase on master
-Remove node-name patch
-patch 06/13: Add virStorageSourceIsRAID
-patch 10/13: Add virDomainDefHasRAID
-patch 11-13/13: Block all unsupported operations
v7:
-Rebase on master.
-Parse unconditionally backing store.
-fold qemuBuildRAIDFileSourceStr into qemuBuildRAIDStr.
-use 0/-1 return values when failing instead of bool.
-virStorageSourceSetBackingStore now free backing store when they are
already set.
Matthias Gatto (13):
virstoragefile: Add virStorageSourceGetBackingStore
virstoragefile: Always use virStorageSourceGetBackingStore to get
backing store
virstoragefile: Add virStorageSourceSetBackingStore
virstoragefile: Always use virStorageSourceSetBackingStore to set
backing store
virstoragefile: change backingStore to backingStores.
virstoragefile: Add virStorageSourceIsRAID
virstoragefile: Add quorum in virstoragefile
domain_conf: Read and Write quorum config
qemu: Add quorum support in qemuBuildDriveDevStr
domain: Add virDomainDefHasRAID
qemu: Forbid blocks operations with quorum
qemu: qemuDomainGetBlockInfo quorum support
qemu: qemuDiskPathToAlias quorum support
docs/formatdomain.html.in | 23 ++++-
docs/schemas/domaincommon.rng | 21 +++-
docs/schemas/storagecommon.rng | 1 +
docs/schemas/storagevol.rng | 1 +
src/conf/domain_conf.c | 174 ++++++++++++++++++++++++----------
src/conf/domain_conf.h | 1 +
src/conf/storage_conf.c | 23 +++--
src/libvirt_private.syms | 4 +
src/qemu/qemu_cgroup.c | 4 +-
src/qemu/qemu_command.c | 78 +++++++++++++++
src/qemu/qemu_domain.c | 2 +-
src/qemu/qemu_driver.c | 72 ++++++++++----
src/qemu/qemu_migration.c | 1 +
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 | 24 +++--
src/storage/storage_backend_fs.c | 45 +++++----
src/storage/storage_backend_gluster.c | 11 ++-
src/storage/storage_backend_logical.c | 15 ++-
src/storage/storage_driver.c | 3 +-
src/util/virstoragefile.c | 121 ++++++++++++++++++++---
src/util/virstoragefile.h | 15 ++-
tests/virstoragetest.c | 18 ++--
25 files changed, 518 insertions(+), 151 deletions(-)
--
2.6.2
10:35 p.m.
New subject: [libvirt] [PATCH v7 01/13] virstoragefile: Add virStorageSourceGetBackingStore
Create virStorageSourceGetBackingStore function in
preparation for quorum:
Actually, if we want to get a backing store inside a virStorageSource
we have to do it manually(src->backingStore = backingStore).
The problem is that with a quorum, a virStorageSource
can contain multiple backing stores, and src->backingStore can
be treated as a virStorageSourcePtr or a virStorageSourcePtrPtr.
Due to these reason, we need to simplify the manipulation of
virStorageSource, and create a function to get the asked
backingStore in a virStorageSource
For now, this function only return the backingStore field
Signed-off-by: Matthias Gatto <matthias.gatto(a)outscale.com>
---
src/libvirt_private.syms | 1 +
src/util/virstoragefile.c | 10 ++++++++++
src/util/virstoragefile.h | 3 +++
3 files changed, 14 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index dd085c3..5354a4a 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2190,6 +2190,7 @@ virStorageSourceClear;
virStorageSourceCopy;
virStorageSourceFree;
virStorageSourceGetActualType;
+virStorageSourceGetBackingStore;
virStorageSourceGetSecurityLabelDef;
virStorageSourceInitChainElement;
virStorageSourceIsEmpty;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 2aa1d90..016beaa 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1809,6 +1809,16 @@ virStorageSourcePoolDefCopy(const virStorageSourcePoolDef *src)
}
+virStorageSourcePtr
+virStorageSourceGetBackingStore(const virStorageSource *src,
+ size_t pos ATTRIBUTE_UNUSED)
+{
+ if (!src)
+ return NULL;
+ return src->backingStore;
+}
+
+
/**
* virStorageSourcePtr:
*
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index b98fe25..8cd4854 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -289,6 +289,9 @@ struct _virStorageSource {
# define DEV_BSIZE 512
# endif
+virStorageSourcePtr virStorageSourceGetBackingStore(const virStorageSource *src,
+ size_t pos);
+
int virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid);
int virStorageFileProbeFormatFromBuf(const char *path,
char *buf,
--
2.6.2
5:57 a.m.
New subject: [libvirt] [PATCH v7 01/13] virstoragefile: Add virStorageSourceGetBackingStore
On 12/03/2015 09:35 AM, Matthias Gatto wrote:
Create virStorageSourceGetBackingStore function in
preparation for quorum:
Actually, if we want to get a backing store inside a virStorageSource
we have to do it manually(src->backingStore = backingStore).
The problem is that with a quorum, a virStorageSource
can contain multiple backing stores, and src->backingStore can
be treated as a virStorageSourcePtr or a virStorageSourcePtrPtr.
Due to these reason, we need to simplify the manipulation of
virStorageSource, and create a function to get the asked
backingStore in a virStorageSource
For now, this function only return the backingStore field
More simply said -
Create helper virStorageSourceGetBackingStore in order to make it easier
to access the storage backingStore pointer. Future patches will adjust
the backingStore pointer to become a table or list of backingStorePtr's
[Sure you're doing it because of the quorum changes, but it's
essentially creating an accessor function]
Signed-off-by: Matthias Gatto <matthias.gatto(a)outscale.com>
---
src/libvirt_private.syms | 1 +
src/util/virstoragefile.c | 10 ++++++++++
src/util/virstoragefile.h | 3 +++
3 files changed, 14 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index dd085c3..5354a4a 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2190,6 +2190,7 @@ virStorageSourceClear;
virStorageSourceCopy;
virStorageSourceFree;
virStorageSourceGetActualType;
+virStorageSourceGetBackingStore;
virStorageSourceGetSecurityLabelDef;
virStorageSourceInitChainElement;
virStorageSourceIsEmpty;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 2aa1d90..016beaa 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1809,6 +1809,16 @@ virStorageSourcePoolDefCopy(const virStorageSourcePoolDef *src)
}
+virStorageSourcePtr
+virStorageSourceGetBackingStore(const virStorageSource *src,
+ size_t pos ATTRIBUTE_UNUSED)
+{
+ if (!src)
I think perhaps Peter's point from his review is
if (!src || pos > 0)
IOW: range checking for pos
Eventually patch 5 will make a real check...
In order to make some progress on this series - at least the first 5 or
6 patches - I can make the change if that is what Peter had in mind...
John
+ return NULL;
+ return src->backingStore;
+}
+
+
/**
* virStorageSourcePtr:
*
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index b98fe25..8cd4854 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -289,6 +289,9 @@ struct _virStorageSource {
# define DEV_BSIZE 512
# endif
+virStorageSourcePtr virStorageSourceGetBackingStore(const virStorageSource *src,
+ size_t pos);
+
int virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid);
int virStorageFileProbeFormatFromBuf(const char *path,
char *buf,
6:07 p.m.
New subject: [libvirt] [PATCH v7 01/13] virstoragefile: Add virStorageSourceGetBackingStore
On Mon, Dec 14, 2015 at 10:57 PM, John Ferlan <jferlan(a)redhat.com> wrote:
On 12/03/2015 09:35 AM, Matthias Gatto wrote:
> Create virStorageSourceGetBackingStore function in
> preparation for quorum:
> Actually, if we want to get a backing store inside a virStorageSource
> we have to do it manually(src->backingStore = backingStore).
> The problem is that with a quorum, a virStorageSource
> can contain multiple backing stores, and src->backingStore can
> be treated as a virStorageSourcePtr or a virStorageSourcePtrPtr.
>
> Due to these reason, we need to simplify the manipulation of
> virStorageSource, and create a function to get the asked
> backingStore in a virStorageSource
>
> For now, this function only return the backingStore field
More simply said -
Create helper virStorageSourceGetBackingStore in order to make it easier
to access the storage backingStore pointer. Future patches will adjust
the backingStore pointer to become a table or list of backingStorePtr's
[Sure you're doing it because of the quorum changes, but it's
essentially creating an accessor function]
>
> Signed-off-by: Matthias Gatto <matthias.gatto(a)outscale.com>
> ---
> src/libvirt_private.syms | 1 +
> src/util/virstoragefile.c | 10 ++++++++++
> src/util/virstoragefile.h | 3 +++
> 3 files changed, 14 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index dd085c3..5354a4a 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2190,6 +2190,7 @@ virStorageSourceClear;
> virStorageSourceCopy;
> virStorageSourceFree;
> virStorageSourceGetActualType;
> +virStorageSourceGetBackingStore;
> virStorageSourceGetSecurityLabelDef;
> virStorageSourceInitChainElement;
> virStorageSourceIsEmpty;
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 2aa1d90..016beaa 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -1809,6 +1809,16 @@ virStorageSourcePoolDefCopy(const virStorageSourcePoolDef
*src)
> }
>
>
> +virStorageSourcePtr
> +virStorageSourceGetBackingStore(const virStorageSource *src,
> + size_t pos ATTRIBUTE_UNUSED)
> +{
> + if (!src)
I think perhaps Peter's point from his review is
if (!src || pos > 0)
IOW: range checking for pos
Eventually patch 5 will make a real check...
In order to make some progress on this series - at least the first 5 or
6 patches - I can make the change if that is what Peter had in mind...
John
> + return NULL;
> + return src->backingStore;
> +}
> +
> +
> /**
> * virStorageSourcePtr:
> *
> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index b98fe25..8cd4854 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -289,6 +289,9 @@ struct _virStorageSource {
> # define DEV_BSIZE 512
> # endif
>
> +virStorageSourcePtr virStorageSourceGetBackingStore(const virStorageSource *src,
> + size_t pos);
> +
> int virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid);
> int virStorageFileProbeFormatFromBuf(const char *path,
> char *buf,
>
Thank you for the review, and your changes on v8.
Sorry for the check, I was thinking it unnecessary because we do it on patch 5.
10:35 p.m.
New subject: [libvirt] [PATCH v7 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 <matthias.gatto(a)outscale.com>
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
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 | 22 ++++++++++++--------
src/storage/storage_backend_fs.c | 38 ++++++++++++++++++++---------------
src/storage/storage_backend_gluster.c | 8 +++++---
src/storage/storage_backend_logical.c | 12 +++++++----
src/util/virstoragefile.c | 20 +++++++++---------
tests/virstoragetest.c | 14 ++++++-------
15 files changed, 93 insertions(+), 70 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e6102a0..5b413b5 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -18625,7 +18625,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;
@@ -18746,7 +18746,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;
@@ -22714,7 +22715,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 b52ce3a..6405944 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 ed21245..28bbe3f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3101,7 +3101,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);
else
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ae1d8e7..8ba7566 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14309,13 +14309,13 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver,
/* Update vm in place to match changes. */
tmp = disk->src;
- disk->src = tmp->backingStore;
+ disk->src = virStorageSourceGetBackingStore(tmp, 0);
tmp->backingStore = NULL;
virStorageSourceFree(tmp);
if (persistDisk) {
tmp = persistDisk->src;
- persistDisk->src = tmp->backingStore;
+ persistDisk->src = virStorageSourceGetBackingStore(tmp, 0);
tmp->backingStore = NULL;
virStorageSourceFree(tmp);
}
@@ -16309,7 +16309,7 @@ qemuDomainBlockPullCommon(virQEMUDriverPtr driver,
goto endjob;
}
- if (virStorageFileGetRelativeBackingPath(disk->src->backingStore,
+ if
(virStorageFileGetRelativeBackingPath(virStorageSourceGetBackingStore(disk->src, 0),
baseSource,
&backingPath) < 0)
goto endjob;
@@ -16662,6 +16662,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
virQEMUDriverConfigPtr cfg = NULL;
const char *format = NULL;
int desttype = virStorageSourceGetActualType(mirror);
+ virStorageSourcePtr backingStore;
/* Preliminaries: find the disk we are editing, sanity checks */
virCheckFlags(VIR_DOMAIN_BLOCK_COPY_SHALLOW |
@@ -16705,8 +16706,9 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
if (qemuDomainDetermineDiskChain(driver, vm, disk, false, true) < 0)
goto endjob;
+ backingStore = virStorageSourceGetBackingStore(disk->src, 0);
/* clear the _SHALLOW flag if there is only one layer */
- if (!disk->src->backingStore)
+ if (!backingStore)
flags &= ~VIR_DOMAIN_BLOCK_COPY_SHALLOW;
/* unless the user provides a pre-created file, shallow copy into a raw
@@ -17113,7 +17115,7 @@ qemuDomainBlockCommit(virDomainPtr dom,
goto endjob;
}
- if (!topSource->backingStore) {
+ if (!virStorageSourceGetBackingStore(topSource, 0)) {
virReportError(VIR_ERR_INVALID_ARG,
_("top '%s' in chain for '%s' has no backing
file"),
topSource->path, path);
@@ -17121,14 +17123,14 @@ qemuDomainBlockCommit(virDomainPtr dom,
}
if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW))
- baseSource = topSource->backingStore;
+ baseSource = virStorageSourceGetBackingStore(topSource, 0);
else if (virStorageFileParseChainIndex(disk->dst, base, &baseIndex) < 0 ||
!(baseSource = virStorageFileChainLookup(disk->src, topSource,
base, baseIndex, NULL)))
goto endjob;
if ((flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW) &&
- baseSource != topSource->backingStore) {
+ baseSource != virStorageSourceGetBackingStore(topSource, 0)) {
virReportError(VIR_ERR_INVALID_ARG,
_("base '%s' is not immediately below '%s' in
chain "
"for '%s'"),
@@ -19406,7 +19408,7 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
goto cleanup;
visited++;
backing_idx++;
- src = src->backingStore;
+ src = virStorageSourceGetBackingStore(src, 0);
}
}
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 86b8c7b..790d7bc 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -3785,7 +3785,9 @@ qemuMonitorJSONDiskNameLookupOne(virJSONValuePtr image,
return NULL;
if (top != target) {
backing = virJSONValueObjectGetObject(image, "backing-image");
- return qemuMonitorJSONDiskNameLookupOne(backing, top->backingStore,
+ virStorageSourcePtr backingStore =
+ virStorageSourceGetBackingStore(top, 0);
+ return qemuMonitorJSONDiskNameLookupOne(backing, backingStore,
target);
}
if (VIR_STRDUP(ret, virJSONValueObjectGetString(image, "filename")) <
0)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index cdde34e..7fdf40f 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -454,7 +454,7 @@ virSecurityDACSetSecurityDiskLabel(virSecurityManagerPtr mgr,
{
virStorageSourcePtr next;
- for (next = disk->src; next; next = next->backingStore) {
+ for (next = disk->src; next; next = virStorageSourceGetBackingStore(next, 0)) {
if (virSecurityDACSetSecurityImageLabel(mgr, def, next) < 0)
return -1;
}
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index b8ebdcc..5d694d8 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1216,7 +1216,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;
/* Don't restore labels on readonly/shared disks, because other VMs may
@@ -1350,7 +1350,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;
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 5de56e5..dddde1c 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -967,7 +967,7 @@ get_files(vahControl * ctl)
/* XXX - if we knew the qemu user:group here we could send it in
* so that the open could be re-tried as that user:group.
*/
- if (!disk->src->backingStore) {
+ if (!virStorageSourceGetBackingStore(disk->src, 0)) {
bool probe = ctl->allowDiskFormatProbing;
virStorageFileGetMetadata(disk->src, -1, -1, probe, false);
}
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 194736b..08ed1dd 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -940,6 +940,7 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn,
.features = vol->target.features,
.nocow = vol->target.nocow,
};
+ virStorageSourcePtr backingStore;
virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL);
@@ -988,12 +989,13 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn,
}
}
- if (vol->target.backingStore) {
+ backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
+ if (backingStore) {
int accessRetCode = -1;
char *absolutePath = NULL;
- info.backingFormat = vol->target.backingStore->format;
- info.backingPath = vol->target.backingStore->path;
+ info.backingFormat = backingStore->format;
+ info.backingPath = backingStore->path;
if (info.preallocate) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -1006,8 +1008,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)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("a different backing store cannot be
specified."));
return NULL;
@@ -1197,7 +1201,7 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn
ATTRIBUTE_UNUSED,
vol->target.format);
return -1;
}
- if (vol->target.backingStore != NULL) {
+ if (virStorageSourceGetBackingStore(&vol->target, 0) != NULL) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("copy-on-write image not supported with "
"qcow-create"));
@@ -1639,14 +1643,16 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol,
unsigned int openflags)
{
int ret;
+ virStorageSourcePtr backingStore;
if ((ret = virStorageBackendUpdateVolTargetInfo(&vol->target,
withBlockVolFormat,
openflags)) < 0)
return ret;
- if (vol->target.backingStore &&
- (ret = virStorageBackendUpdateVolTargetInfo(vol->target.backingStore,
+ backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
+ if (backingStore &&
+ (ret = virStorageBackendUpdateVolTargetInfo(backingStore,
withBlockVolFormat,
VIR_STORAGE_VOL_OPEN_DEFAULT |
VIR_STORAGE_VOL_OPEN_NOERROR) <
0))
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index 99ea394..7b05d4d 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -70,6 +70,7 @@ virStorageBackendProbeTarget(virStorageSourcePtr target,
int ret = -1;
int rc;
virStorageSourcePtr meta = NULL;
+ virStorageSourcePtr backingStore;
struct stat sb;
if (encryption)
@@ -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);
- if (VIR_ALLOC(target->backingStore) < 0)
+ if (VIR_ALLOC(backingStore) < 0)
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;
}
}
}
@@ -860,6 +863,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn
ATTRIBUTE_UNUSED,
struct stat statbuf;
virStorageVolDefPtr vol = NULL;
virStorageSourcePtr target = NULL;
+ virStorageSourcePtr backingStore;
int direrr;
int fd = -1, ret = -1;
@@ -918,10 +922,12 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn
ATTRIBUTE_UNUSED,
if (vol->target.format == VIR_STORAGE_FILE_DIR)
vol->type = VIR_STORAGE_VOL_DIR;
- if (vol->target.backingStore) {
-
ignore_value(virStorageBackendUpdateVolTargetInfo(vol->target.backingStore,
- false,
-
VIR_STORAGE_VOL_OPEN_DEFAULT));
+ backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
+ if (backingStore) {
+ ignore_value(virStorageBackendUpdateVolTargetInfo(
+ backingStore,
+ false,
+ VIR_STORAGE_VOL_OPEN_DEFAULT));
/* If this failed, the backing file is currently unavailable,
* the capacity, allocation, owner, group and mode are unknown.
* An error message was raised, but we just continue. */
diff --git a/src/storage/storage_backend_gluster.c
b/src/storage/storage_backend_gluster.c
index d2e79bc..9bddb3b 100644
--- a/src/storage/storage_backend_gluster.c
+++ b/src/storage/storage_backend_gluster.c
@@ -247,6 +247,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr
state,
char *header = NULL;
ssize_t len = VIR_STORAGE_MAX_HEADER;
int backingFormat;
+ virStorageSourcePtr backingStore;
*volptr = NULL;
@@ -302,13 +303,14 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr
state,
if (meta->backingStoreRaw) {
if (VIR_ALLOC(vol->target.backingStore) < 0)
goto cleanup;
+ backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
- vol->target.backingStore->path = meta->backingStoreRaw;
+ backingStore->path = meta->backingStoreRaw;
if (backingFormat < 0)
- vol->target.backingStore->format = VIR_STORAGE_FILE_RAW;
+ backingStore->format = VIR_STORAGE_FILE_RAW;
else
- vol->target.backingStore->format = backingFormat;
+ backingStore->format = backingFormat;
meta->backingStoreRaw = NULL;
}
diff --git a/src/storage/storage_backend_logical.c
b/src/storage/storage_backend_logical.c
index 536e617..e2a287d 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -88,6 +88,7 @@ virStorageBackendLogicalMakeVol(char **const groups,
size_t i;
int err, nextents, nvars, ret = -1;
const char *attrs = groups[9];
+ virStorageSourcePtr backingStore;
/* Skip inactive volume */
if (attrs[4] != 'a')
@@ -151,11 +152,12 @@ virStorageBackendLogicalMakeVol(char **const groups,
if (VIR_ALLOC(vol->target.backingStore) < 0)
goto cleanup;
- if (virAsprintf(&vol->target.backingStore->path, "%s/%s",
+ backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
+ if (virAsprintf(&backingStore->path, "%s/%s",
pool->def->target.path, groups[1]) < 0)
goto cleanup;
- vol->target.backingStore->format = VIR_STORAGE_POOL_LOGICAL_LVM2;
+ backingStore->format = VIR_STORAGE_POOL_LOGICAL_LVM2;
}
if (!vol->key && VIR_STRDUP(vol->key, groups[2]) < 0)
@@ -732,6 +734,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
virErrorPtr err;
struct stat sb;
bool created = false;
+ virStorageSourcePtr backingStore;
if (vol->target.encryption != NULL) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -764,8 +767,9 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
}
virCommandAddArgFormat(cmd, "%lluK", VIR_DIV_UP(vol->target.capacity,
1024));
- if (vol->target.backingStore)
- virCommandAddArgList(cmd, "-s", vol->target.backingStore->path,
NULL);
+ backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
+ if (backingStore)
+ virCommandAddArgList(cmd, "-s", backingStore->path, NULL);
else
virCommandAddArg(cmd, pool->def->source.name);
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 016beaa..af17d82 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1076,10 +1076,10 @@ virStorageFileChainGetBroken(virStorageSourcePtr chain,
if (!chain)
return 0;
- for (tmp = chain; tmp; tmp = tmp->backingStore) {
+ for (tmp = chain; tmp; tmp = virStorageSourceGetBackingStore(tmp, 0)) {
/* Break when we hit end of chain; report error if we detected
* a missing backing file, infinite loop, or other error */
- if (!tmp->backingStore && tmp->backingStoreRaw) {
+ if (!virStorageSourceGetBackingStore(tmp, 0) && tmp->backingStoreRaw)
{
if (VIR_STRDUP(*brokenFile, tmp->backingStoreRaw) < 0)
return -1;
@@ -1334,8 +1334,8 @@ virStorageFileChainLookup(virStorageSourcePtr chain,
*parent = NULL;
if (startFrom) {
- while (chain && chain != startFrom->backingStore) {
- chain = chain->backingStore;
+ while (chain && chain != virStorageSourceGetBackingStore(startFrom, 0))
{
+ chain = virStorageSourceGetBackingStore(chain, 0);
i++;
}
@@ -1352,7 +1352,7 @@ virStorageFileChainLookup(virStorageSourcePtr chain,
while (chain) {
if (!name && !idx) {
- if (!chain->backingStore)
+ if (!virStorageSourceGetBackingStore(chain, 0))
break;
} else if (idx) {
VIR_DEBUG("%zu: %s", i, chain->path);
@@ -1387,7 +1387,7 @@ virStorageFileChainLookup(virStorageSourcePtr chain,
}
}
*parent = chain;
- chain = chain->backingStore;
+ chain = virStorageSourceGetBackingStore(chain, 0);
i++;
}
@@ -1893,8 +1893,8 @@ virStorageSourceCopy(const virStorageSource *src,
!(ret->auth = virStorageAuthDefCopy(src->auth)))
goto error;
- if (backingChain && src->backingStore) {
- if (!(ret->backingStore = virStorageSourceCopy(src->backingStore,
+ if (backingChain && virStorageSourceGetBackingStore(src, 0)) {
+ if (!(ret->backingStore =
virStorageSourceCopy(virStorageSourceGetBackingStore(src, 0),
true)))
goto error;
}
@@ -2035,7 +2035,7 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def)
VIR_FREE(def->backingStoreRaw);
/* recursively free backing chain */
- virStorageSourceFree(def->backingStore);
+ virStorageSourceFree(virStorageSourceGetBackingStore(def, 0));
def->backingStore = NULL;
}
@@ -2898,7 +2898,7 @@ virStorageFileGetRelativeBackingPath(virStorageSourcePtr top,
*relpath = NULL;
- for (next = top; next; next = next->backingStore) {
+ for (next = top; next; next = virStorageSourceGetBackingStore(next, 0)) {
if (!next->relPath) {
ret = 1;
goto cleanup;
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index 38ce09e..5bd4637 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -403,7 +403,7 @@ testStorageChain(const void *args)
}
VIR_FREE(expect);
VIR_FREE(actual);
- elt = elt->backingStore;
+ elt = virStorageSourceGetBackingStore(elt, 0);
i++;
}
if (i != data->nfiles) {
@@ -1044,8 +1044,8 @@ mymain(void)
ret = -1;
goto cleanup;
}
- chain2 = chain->backingStore;
- chain3 = chain2->backingStore;
+ chain2 = virStorageSourceGetBackingStore(chain, 0);
+ chain3 = virStorageSourceGetBackingStore(chain2, 0);
#define TEST_LOOKUP_TARGET(id, target, from, name, index, result, \
meta, parent) \
@@ -1110,8 +1110,8 @@ mymain(void)
ret = -1;
goto cleanup;
}
- chain2 = chain->backingStore;
- chain3 = chain2->backingStore;
+ chain2 = virStorageSourceGetBackingStore(chain, 0);
+ chain3 = virStorageSourceGetBackingStore(chain2, 0);
TEST_LOOKUP(28, NULL, "bogus", NULL, NULL, NULL);
TEST_LOOKUP(29, chain, "bogus", NULL, NULL, NULL);
@@ -1157,8 +1157,8 @@ mymain(void)
ret = -1;
goto cleanup;
}
- chain2 = chain->backingStore;
- chain3 = chain2->backingStore;
+ chain2 = virStorageSourceGetBackingStore(chain, 0);
+ chain3 = virStorageSourceGetBackingStore(chain2, 0);
TEST_LOOKUP(56, NULL, "bogus", NULL, NULL, NULL);
TEST_LOOKUP(57, NULL, "sub/link2", chain->path, chain, NULL);
--
2.6.2
8:44 a.m.
New subject: [libvirt] [PATCH v7 02/13] virstoragefile: Always use virStorageSourceGetBackingStore to get backing store
On 12/03/2015 09:35 AM, Matthias Gatto wrote:
Uniformize backing store usage by calling
virStorageSourceGetBackingStore
instead of setting backing store manually.
Signed-off-by: Matthias Gatto <matthias.gatto(a)outscale.com>
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
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 | 22 ++++++++++++--------
src/storage/storage_backend_fs.c | 38 ++++++++++++++++++++---------------
src/storage/storage_backend_gluster.c | 8 +++++---
src/storage/storage_backend_logical.c | 12 +++++++----
src/util/virstoragefile.c | 20 +++++++++---------
tests/virstoragetest.c | 14 ++++++-------
15 files changed, 93 insertions(+), 70 deletions(-)
storage_backend_fs.c needed a slight merge with some changes I made...
There's also still a bunch of long lines. I've noted them below.
For each of them I propose adjusting the code a bit to do something like:
virStorageSourcePtr bsp;
...
bsp = virStorageSourceGetBackingStore(backingStore, 0);
then use bsp instead.
I'll add a "merge" patch to this response - it probably won't apply
cleanly since I had the merge issue to deal with first (in
storage_backend_fs.c - virStorageBackendFileSystemRefresh and call to
virStorageBackendUpdateVolTargetInfo). But I think if you update to top
of tree and then apply, a git am should work
John
I only worked on/through the first 2 patches, but can continue through
patch 5 - that'll at least get part of this series in
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e6102a0..5b413b5 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -18625,7 +18625,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),
Long line
backingStore->backingStoreRaw,
idx + 1) < 0)
return -1;
@@ -18746,7 +18746,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),
Long line
def->src->backingStoreRaw, 1) < 0)
return -1;
@@ -22714,7 +22715,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))) {
Really long line
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),
long line
"backingStore") <
0)
goto cleanup;
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index b52ce3a..6405944 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)) {
long line
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)) {
long line
if (qemuSetImageCgroup(vm, next, true) < 0)
return -1;
}
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ed21245..28bbe3f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3101,7 +3101,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);
else
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ae1d8e7..8ba7566 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14309,13 +14309,13 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr
driver,
/* Update vm in place to match changes. */
tmp = disk->src;
- disk->src = tmp->backingStore;
+ disk->src = virStorageSourceGetBackingStore(tmp, 0);
tmp->backingStore = NULL;
virStorageSourceFree(tmp);
if (persistDisk) {
tmp = persistDisk->src;
- persistDisk->src = tmp->backingStore;
+ persistDisk->src = virStorageSourceGetBackingStore(tmp, 0);
tmp->backingStore = NULL;
virStorageSourceFree(tmp);
}
@@ -16309,7 +16309,7 @@ qemuDomainBlockPullCommon(virQEMUDriverPtr driver,
goto endjob;
}
- if (virStorageFileGetRelativeBackingPath(disk->src->backingStore,
+ if
(virStorageFileGetRelativeBackingPath(virStorageSourceGetBackingStore(disk->src, 0),
long line
baseSource,
&backingPath) < 0)
goto endjob;
@@ -16662,6 +16662,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
virQEMUDriverConfigPtr cfg = NULL;
const char *format = NULL;
int desttype = virStorageSourceGetActualType(mirror);
+ virStorageSourcePtr backingStore;
/* Preliminaries: find the disk we are editing, sanity checks */
virCheckFlags(VIR_DOMAIN_BLOCK_COPY_SHALLOW |
@@ -16705,8 +16706,9 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
if (qemuDomainDetermineDiskChain(driver, vm, disk, false, true) < 0)
goto endjob;
+ backingStore = virStorageSourceGetBackingStore(disk->src, 0);
/* clear the _SHALLOW flag if there is only one layer */
- if (!disk->src->backingStore)
+ if (!backingStore)
flags &= ~VIR_DOMAIN_BLOCK_COPY_SHALLOW;
/* unless the user provides a pre-created file, shallow copy into a raw
@@ -17113,7 +17115,7 @@ qemuDomainBlockCommit(virDomainPtr dom,
goto endjob;
}
- if (!topSource->backingStore) {
+ if (!virStorageSourceGetBackingStore(topSource, 0)) {
virReportError(VIR_ERR_INVALID_ARG,
_("top '%s' in chain for '%s' has no backing
file"),
topSource->path, path);
@@ -17121,14 +17123,14 @@ qemuDomainBlockCommit(virDomainPtr dom,
}
if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW))
- baseSource = topSource->backingStore;
+ baseSource = virStorageSourceGetBackingStore(topSource, 0);
else if (virStorageFileParseChainIndex(disk->dst, base, &baseIndex) < 0
||
!(baseSource = virStorageFileChainLookup(disk->src, topSource,
base, baseIndex, NULL)))
goto endjob;
if ((flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW) &&
- baseSource != topSource->backingStore) {
+ baseSource != virStorageSourceGetBackingStore(topSource, 0)) {
virReportError(VIR_ERR_INVALID_ARG,
_("base '%s' is not immediately below '%s'
in chain "
"for '%s'"),
@@ -19406,7 +19408,7 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
goto cleanup;
visited++;
backing_idx++;
- src = src->backingStore;
+ src = virStorageSourceGetBackingStore(src, 0);
}
}
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 86b8c7b..790d7bc 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -3785,7 +3785,9 @@ qemuMonitorJSONDiskNameLookupOne(virJSONValuePtr image,
return NULL;
if (top != target) {
backing = virJSONValueObjectGetObject(image, "backing-image");
- return qemuMonitorJSONDiskNameLookupOne(backing, top->backingStore,
+ virStorageSourcePtr backingStore =
+ virStorageSourceGetBackingStore(top, 0);
+ return qemuMonitorJSONDiskNameLookupOne(backing, backingStore,
target);
}
if (VIR_STRDUP(ret, virJSONValueObjectGetString(image, "filename")) <
0)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index cdde34e..7fdf40f 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -454,7 +454,7 @@ virSecurityDACSetSecurityDiskLabel(virSecurityManagerPtr mgr,
{
virStorageSourcePtr next;
- for (next = disk->src; next; next = next->backingStore) {
+ for (next = disk->src; next; next = virStorageSourceGetBackingStore(next, 0)) {
long line
if (virSecurityDACSetSecurityImageLabel(mgr, def, next) <
0)
return -1;
}
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index b8ebdcc..5d694d8 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1216,7 +1216,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;
/* Don't restore labels on readonly/shared disks, because other VMs may
@@ -1350,7 +1350,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)) {
long line
if (virSecuritySELinuxSetSecurityImageLabelInternal(mgr,
def, next,
first) < 0)
return -1;
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 5de56e5..dddde1c 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -967,7 +967,7 @@ get_files(vahControl * ctl)
/* XXX - if we knew the qemu user:group here we could send it in
* so that the open could be re-tried as that user:group.
*/
- if (!disk->src->backingStore) {
+ if (!virStorageSourceGetBackingStore(disk->src, 0)) {
bool probe = ctl->allowDiskFormatProbing;
virStorageFileGetMetadata(disk->src, -1, -1, probe, false);
}
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 194736b..08ed1dd 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -940,6 +940,7 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn,
.features = vol->target.features,
.nocow = vol->target.nocow,
};
+ virStorageSourcePtr backingStore;
virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL);
@@ -988,12 +989,13 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn,
}
}
- if (vol->target.backingStore) {
+ backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
+ if (backingStore) {
int accessRetCode = -1;
char *absolutePath = NULL;
- info.backingFormat = vol->target.backingStore->format;
- info.backingPath = vol->target.backingStore->path;
+ info.backingFormat = backingStore->format;
+ info.backingPath = backingStore->path;
if (info.preallocate) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -1006,8 +1008,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)) {
long line
This is also another one of those cases where using a temp variable is
going to help.
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("a different backing store cannot be
specified."));
return NULL;
@@ -1197,7 +1201,7 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn
ATTRIBUTE_UNUSED,
vol->target.format);
return -1;
}
- if (vol->target.backingStore != NULL) {
+ if (virStorageSourceGetBackingStore(&vol->target, 0) != NULL) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("copy-on-write image not supported with "
"qcow-create"));
@@ -1639,14 +1643,16 @@ virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol,
unsigned int openflags)
{
int ret;
+ virStorageSourcePtr backingStore;
if ((ret = virStorageBackendUpdateVolTargetInfo(&vol->target,
withBlockVolFormat,
openflags)) < 0)
return ret;
- if (vol->target.backingStore &&
- (ret = virStorageBackendUpdateVolTargetInfo(vol->target.backingStore,
+ backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
+ if (backingStore &&
+ (ret = virStorageBackendUpdateVolTargetInfo(backingStore,
withBlockVolFormat,
VIR_STORAGE_VOL_OPEN_DEFAULT |
VIR_STORAGE_VOL_OPEN_NOERROR) <
0))
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index 99ea394..7b05d4d 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -70,6 +70,7 @@ virStorageBackendProbeTarget(virStorageSourcePtr target,
int ret = -1;
int rc;
virStorageSourcePtr meta = NULL;
+ virStorageSourcePtr backingStore;
struct stat sb;
if (encryption)
@@ -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);
Although you cleaned up the reference Peter noted before - there's
double free problem here if the VIR_ALLOC fails. Currently backingStore
is just the local copy of target->backingStore. If we free it here,
then target->backingStore still contains the address/pointer. If we jump
to cleanup, then target->backingStore could be free'd again
Perhaps one way around this is to do the 'set' code first, but I think
at least the following would be OK after the SourceFree
target->backingStore = NULL;
- if (VIR_ALLOC(target->backingStore) < 0)
+ if (VIR_ALLOC(backingStore) < 0)
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;
}
}
}
@@ -860,6 +863,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn
ATTRIBUTE_UNUSED,
struct stat statbuf;
virStorageVolDefPtr vol = NULL;
virStorageSourcePtr target = NULL;
+ virStorageSourcePtr backingStore;
int direrr;
int fd = -1, ret = -1;
@@ -918,10 +922,12 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn
ATTRIBUTE_UNUSED,
if (vol->target.format == VIR_STORAGE_FILE_DIR)
vol->type = VIR_STORAGE_VOL_DIR;
- if (vol->target.backingStore) {
-
ignore_value(virStorageBackendUpdateVolTargetInfo(vol->target.backingStore,
- false,
-
VIR_STORAGE_VOL_OPEN_DEFAULT));
+ backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
+ if (backingStore) {
+ ignore_value(virStorageBackendUpdateVolTargetInfo(
+ backingStore,
+ false,
+ VIR_STORAGE_VOL_OPEN_DEFAULT));
/* If this failed, the backing file is currently unavailable,
* the capacity, allocation, owner, group and mode are unknown.
* An error message was raised, but we just continue. */
diff --git a/src/storage/storage_backend_gluster.c
b/src/storage/storage_backend_gluster.c
index d2e79bc..9bddb3b 100644
--- a/src/storage/storage_backend_gluster.c
+++ b/src/storage/storage_backend_gluster.c
@@ -247,6 +247,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr
state,
char *header = NULL;
ssize_t len = VIR_STORAGE_MAX_HEADER;
int backingFormat;
+ virStorageSourcePtr backingStore;
*volptr = NULL;
@@ -302,13 +303,14 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr
state,
if (meta->backingStoreRaw) {
if (VIR_ALLOC(vol->target.backingStore) < 0)
goto cleanup;
+ backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
- vol->target.backingStore->path = meta->backingStoreRaw;
+ backingStore->path = meta->backingStoreRaw;
if (backingFormat < 0)
- vol->target.backingStore->format = VIR_STORAGE_FILE_RAW;
+ backingStore->format = VIR_STORAGE_FILE_RAW;
else
- vol->target.backingStore->format = backingFormat;
+ backingStore->format = backingFormat;
meta->backingStoreRaw = NULL;
}
diff --git a/src/storage/storage_backend_logical.c
b/src/storage/storage_backend_logical.c
index 536e617..e2a287d 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -88,6 +88,7 @@ virStorageBackendLogicalMakeVol(char **const groups,
size_t i;
int err, nextents, nvars, ret = -1;
const char *attrs = groups[9];
+ virStorageSourcePtr backingStore;
/* Skip inactive volume */
if (attrs[4] != 'a')
@@ -151,11 +152,12 @@ virStorageBackendLogicalMakeVol(char **const groups,
if (VIR_ALLOC(vol->target.backingStore) < 0)
goto cleanup;
- if (virAsprintf(&vol->target.backingStore->path, "%s/%s",
+ backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
I see you fixed up the references here, but we still have a problem if
backingStore == NULL (now *technically* I know that cannot happen yet,
but at some point in the future it could.
So what should be done if backingStore == NULL here? Something like:
backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
if (backingStore) {
if (virAsprintf(&backingStore->path, "%s/%s",
pool->def->target.path, groups[1]) < 0)
goto cleanup;
backingStore->format = VIR_STORAGE_POOL_LOGICAL_LVM2;
}
+ if (virAsprintf(&backingStore->path,
"%s/%s",
pool->def->target.path, groups[1]) < 0)
goto cleanup;
- vol->target.backingStore->format = VIR_STORAGE_POOL_LOGICAL_LVM2;
+ backingStore->format = VIR_STORAGE_POOL_LOGICAL_LVM2;
}
if (!vol->key && VIR_STRDUP(vol->key, groups[2]) < 0)
@@ -732,6 +734,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
virErrorPtr err;
struct stat sb;
bool created = false;
+ virStorageSourcePtr backingStore;
if (vol->target.encryption != NULL) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -764,8 +767,9 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
}
virCommandAddArgFormat(cmd, "%lluK", VIR_DIV_UP(vol->target.capacity,
1024));
- if (vol->target.backingStore)
- virCommandAddArgList(cmd, "-s", vol->target.backingStore->path,
NULL);
+ backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
+ if (backingStore)
+ virCommandAddArgList(cmd, "-s", backingStore->path, NULL);
else
virCommandAddArg(cmd, pool->def->source.name);
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 016beaa..af17d82 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1076,10 +1076,10 @@ virStorageFileChainGetBroken(virStorageSourcePtr chain,
if (!chain)
return 0;
- for (tmp = chain; tmp; tmp = tmp->backingStore) {
+ for (tmp = chain; tmp; tmp = virStorageSourceGetBackingStore(tmp, 0)) {
/* Break when we hit end of chain; report error if we detected
* a missing backing file, infinite loop, or other error */
- if (!tmp->backingStore && tmp->backingStoreRaw) {
+ if (!virStorageSourceGetBackingStore(tmp, 0) && tmp->backingStoreRaw)
{
if (VIR_STRDUP(*brokenFile, tmp->backingStoreRaw) < 0)
return -1;
@@ -1334,8 +1334,8 @@ virStorageFileChainLookup(virStorageSourcePtr chain,
*parent = NULL;
if (startFrom) {
- while (chain && chain != startFrom->backingStore) {
- chain = chain->backingStore;
+ while (chain && chain != virStorageSourceGetBackingStore(startFrom, 0))
{
+ chain = virStorageSourceGetBackingStore(chain, 0);
i++;
}
@@ -1352,7 +1352,7 @@ virStorageFileChainLookup(virStorageSourcePtr chain,
while (chain) {
if (!name && !idx) {
- if (!chain->backingStore)
+ if (!virStorageSourceGetBackingStore(chain, 0))
break;
} else if (idx) {
VIR_DEBUG("%zu: %s", i, chain->path);
@@ -1387,7 +1387,7 @@ virStorageFileChainLookup(virStorageSourcePtr chain,
}
}
*parent = chain;
- chain = chain->backingStore;
+ chain = virStorageSourceGetBackingStore(chain, 0);
i++;
}
@@ -1893,8 +1893,8 @@ virStorageSourceCopy(const virStorageSource *src,
!(ret->auth = virStorageAuthDefCopy(src->auth)))
goto error;
- if (backingChain && src->backingStore) {
- if (!(ret->backingStore = virStorageSourceCopy(src->backingStore,
+ if (backingChain && virStorageSourceGetBackingStore(src, 0)) {
+ if (!(ret->backingStore =
virStorageSourceCopy(virStorageSourceGetBackingStore(src, 0),
true)))
long line
goto error;
}
@@ -2035,7 +2035,7 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def)
VIR_FREE(def->backingStoreRaw);
/* recursively free backing chain */
- virStorageSourceFree(def->backingStore);
+ virStorageSourceFree(virStorageSourceGetBackingStore(def, 0));
def->backingStore = NULL;
}
@@ -2898,7 +2898,7 @@ virStorageFileGetRelativeBackingPath(virStorageSourcePtr top,
*relpath = NULL;
- for (next = top; next; next = next->backingStore) {
+ for (next = top; next; next = virStorageSourceGetBackingStore(next, 0)) {
if (!next->relPath) {
ret = 1;
goto cleanup;
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index 38ce09e..5bd4637 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -403,7 +403,7 @@ testStorageChain(const void *args)
}
VIR_FREE(expect);
VIR_FREE(actual);
- elt = elt->backingStore;
+ elt = virStorageSourceGetBackingStore(elt, 0);
i++;
}
if (i != data->nfiles) {
@@ -1044,8 +1044,8 @@ mymain(void)
ret = -1;
goto cleanup;
}
- chain2 = chain->backingStore;
- chain3 = chain2->backingStore;
+ chain2 = virStorageSourceGetBackingStore(chain, 0);
+ chain3 = virStorageSourceGetBackingStore(chain2, 0);
#define TEST_LOOKUP_TARGET(id, target, from, name, index, result, \
meta, parent) \
@@ -1110,8 +1110,8 @@ mymain(void)
ret = -1;
goto cleanup;
}
- chain2 = chain->backingStore;
- chain3 = chain2->backingStore;
+ chain2 = virStorageSourceGetBackingStore(chain, 0);
+ chain3 = virStorageSourceGetBackingStore(chain2, 0);
TEST_LOOKUP(28, NULL, "bogus", NULL, NULL, NULL);
TEST_LOOKUP(29, chain, "bogus", NULL, NULL, NULL);
@@ -1157,8 +1157,8 @@ mymain(void)
ret = -1;
goto cleanup;
}
- chain2 = chain->backingStore;
- chain3 = chain2->backingStore;
+ chain2 = virStorageSourceGetBackingStore(chain, 0);
+ chain3 = virStorageSourceGetBackingStore(chain2, 0);
TEST_LOOKUP(56, NULL, "bogus", NULL, NULL, NULL);
TEST_LOOKUP(57, NULL, "sub/link2", chain->path, chain, NULL);
10:35 p.m.
New subject: [libvirt] [PATCH v7 03/13] virstoragefile: Add virStorageSourceSetBackingStore
As explained for virStorageSourceGetBackingStore, quorum allows
multiple backing store, this make the operation to set bs complex
because we have to check if the backingStore is used as an array
or a pointer, and set it differently in both case.
In order to help the manipulation of backing store, I've added a
function virStorageSourceSetBackingStore.
For now virStorageSourceSetBackingStore don't handle the case where
we have more than one backing store in virStorageSource.
Signed-off-by: Matthias Gatto <matthias.gatto(a)outscale.com>
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virstoragefile.c | 10 ++++++++++
src/util/virstoragefile.h | 4 ++++
3 files changed, 15 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 5354a4a..d3baee8 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2200,6 +2200,7 @@ virStorageSourceParseRBDColonString;
virStorageSourcePoolDefFree;
virStorageSourcePoolModeTypeFromString;
virStorageSourcePoolModeTypeToString;
+virStorageSourceSetBackingStore;
virStorageSourceUpdateBlockPhysicalSize;
virStorageTypeFromString;
virStorageTypeToString;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index af17d82..a8a2134 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1819,6 +1819,16 @@ virStorageSourceGetBackingStore(const virStorageSource *src,
}
+int
+virStorageSourceSetBackingStore(virStorageSourcePtr src,
+ virStorageSourcePtr backingStore,
+ size_t pos ATTRIBUTE_UNUSED)
+{
+ src->backingStore = backingStore;
+ return 0;
+}
+
+
/**
* virStorageSourcePtr:
*
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 8cd4854..ce1cb5d 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -289,6 +289,10 @@ struct _virStorageSource {
# define DEV_BSIZE 512
# endif
+int virStorageSourceSetBackingStore(virStorageSourcePtr src,
+ virStorageSourcePtr backingStore,
+ size_t pos);
+
virStorageSourcePtr virStorageSourceGetBackingStore(const virStorageSource *src,
size_t pos);
--
2.6.2
4:48 a.m.
New subject: [libvirt] [PATCH v7 03/13] virstoragefile: Add virStorageSourceSetBackingStore
On 12/03/2015 09:35 AM, Matthias Gatto wrote:
As explained for virStorageSourceGetBackingStore, quorum allows
multiple backing store, this make the operation to set bs complex
because we have to check if the backingStore is used as an array
or a pointer, and set it differently in both case.
In order to help the manipulation of backing store, I've added a
function virStorageSourceSetBackingStore.
For now virStorageSourceSetBackingStore don't handle the case where
we have more than one backing store in virStorageSource.
Signed-off-by: Matthias Gatto <matthias.gatto(a)outscale.com>
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virstoragefile.c | 10 ++++++++++
src/util/virstoragefile.h | 4 ++++
3 files changed, 15 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 5354a4a..d3baee8 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2200,6 +2200,7 @@ virStorageSourceParseRBDColonString;
virStorageSourcePoolDefFree;
virStorageSourcePoolModeTypeFromString;
virStorageSourcePoolModeTypeToString;
+virStorageSourceSetBackingStore;
virStorageSourceUpdateBlockPhysicalSize;
virStorageTypeFromString;
virStorageTypeToString;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index af17d82..a8a2134 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1819,6 +1819,16 @@ virStorageSourceGetBackingStore(const virStorageSource *src,
}
+int
+virStorageSourceSetBackingStore(virStorageSourcePtr src,
+ virStorageSourcePtr backingStore,
+ size_t pos ATTRIBUTE_UNUSED)
+{
As Peter points out in v6 - there's no range checking here.
John
+ src->backingStore = backingStore;
+ return 0;
+}
+
+
/**
* virStorageSourcePtr:
*
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 8cd4854..ce1cb5d 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -289,6 +289,10 @@ struct _virStorageSource {
# define DEV_BSIZE 512
# endif
+int virStorageSourceSetBackingStore(virStorageSourcePtr src,
+ virStorageSourcePtr backingStore,
+ size_t pos);
+
virStorageSourcePtr virStorageSourceGetBackingStore(const virStorageSource *src,
size_t pos);
10:35 p.m.
New subject: [libvirt] [PATCH v7 04/13] 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 <matthias.gatto(a)outscale.com>
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
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 | 5 +++--
src/storage/storage_driver.c | 3 ++-
src/util/virstoragefile.c | 8 +++++---
tests/virstoragetest.c | 4 ++--
9 files changed, 43 insertions(+), 26 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5b413b5..d146811 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6359,7 +6359,8 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt,
virDomainDiskBackingStoreParse(ctxt, backingStore) < 0)
goto cleanup;
- src->backingStore = backingStore;
+ if (virStorageSourceSetBackingStore(src, backingStore, 0) < 0)
+ goto cleanup;
ret = 0;
cleanup:
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index d048e39..b9db5eb 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -1259,6 +1259,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
char *capacity = NULL;
char *unit = NULL;
char *backingStore = NULL;
+ virStorageSourcePtr backingStorePtr;
xmlNodePtr node;
xmlNodePtr *nodes = NULL;
size_t i;
@@ -1295,20 +1296,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) <
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);
@@ -1317,9 +1320,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") < 0)
goto error;
}
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8ba7566..edfd8e6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14261,13 +14261,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) < 0) {
+ ret = -1;
+ goto cleanup;
+ }
newDiskSrc = NULL;
if (persistDisk) {
- persistDiskSrc->backingStore = persistDisk->src;
- persistDisk->src = persistDiskSrc;
+ if (virStorageSourceSetBackingStore(persistDiskSrc,
+ persistDisk->src, 0) < 0) {
+ ret = -1;
+ goto cleanup;
+ }
persistDiskSrc = NULL;
}
@@ -14310,13 +14315,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/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index 7b05d4d..b216e91 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -97,7 +97,9 @@ virStorageBackendProbeTarget(virStorageSourcePtr target,
goto cleanup;
if (meta->backingStoreRaw) {
- if (!(target->backingStore = virStorageSourceNewFromBacking(meta)))
+ if (virStorageSourceSetBackingStore(target,
+ virStorageSourceNewFromBacking(meta),
+ 0) < 0)
goto cleanup;
backingStore = virStorageSourceGetBackingStore(target, 0);
@@ -112,7 +114,8 @@ virStorageBackendProbeTarget(virStorageSourcePtr target,
if (VIR_ALLOC(backingStore) < 0)
goto cleanup;
- target->backingStore = backingStore;
+ if (virStorageSourceSetBackingStore(target, backingStore, 0) < 0)
+ goto cleanup;
backingStore->type = VIR_STORAGE_TYPE_NETWORK;
backingStore->path = meta->backingStoreRaw;
meta->backingStoreRaw = NULL;
diff --git a/src/storage/storage_backend_gluster.c
b/src/storage/storage_backend_gluster.c
index 9bddb3b..8b89433 100644
--- a/src/storage/storage_backend_gluster.c
+++ b/src/storage/storage_backend_gluster.c
@@ -301,9 +301,10 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr
state,
goto cleanup;
if (meta->backingStoreRaw) {
- if (VIR_ALLOC(vol->target.backingStore) < 0)
+ if (VIR_ALLOC(backingStore) < 0)
+ goto cleanup;
+ if (virStorageSourceSetBackingStore(&vol->target, backingStore, 0) <
0)
goto cleanup;
- backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
backingStore->path = meta->backingStoreRaw;
diff --git a/src/storage/storage_backend_logical.c
b/src/storage/storage_backend_logical.c
index e2a287d..eca9730 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -149,10 +149,11 @@ virStorageBackendLogicalMakeVol(char **const groups,
* lv is created with "--virtualsize").
*/
if (groups[1] && STRNEQ(groups[1], "") && (groups[1][0] !=
'[')) {
- if (VIR_ALLOC(vol->target.backingStore) < 0)
+ if (VIR_ALLOC(backingStore) < 0)
goto cleanup;
- backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
+ if (virStorageSourceSetBackingStore(&vol->target, backingStore, 0) <
0)
+ goto cleanup;
if (virAsprintf(&backingStore->path, "%s/%s",
pool->def->target.path, groups[1]) < 0)
goto cleanup;
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index bbf21f6..963e325 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -3064,7 +3064,8 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
goto cleanup;
}
- src->backingStore = backingStore;
+ if (virStorageSourceSetBackingStore(src, backingStore, 0) < 0)
+ goto cleanup;
backingStore = NULL;
ret = 0;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index a8a2134..1299f98 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1904,8 +1904,10 @@ virStorageSourceCopy(const virStorageSource *src,
goto error;
if (backingChain && virStorageSourceGetBackingStore(src, 0)) {
- if (!(ret->backingStore =
virStorageSourceCopy(virStorageSourceGetBackingStore(src, 0),
- true)))
+ if (virStorageSourceSetBackingStore(ret,
+
virStorageSourceCopy(virStorageSourceGetBackingStore(src, 0),
+ true),
+ 0) < 0)
goto error;
}
@@ -2046,7 +2048,7 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def)
/* recursively free backing chain */
virStorageSourceFree(virStorageSourceGetBackingStore(def, 0));
- def->backingStore = NULL;
+ ignore_value(virStorageSourceSetBackingStore(def, NULL, 0));
}
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index 5bd4637..58f505d 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -583,9 +583,9 @@ testPathRelativePrepare(void)
for (i = 0; i < ARRAY_CARDINALITY(backingchain); i++) {
if (i < ARRAY_CARDINALITY(backingchain) - 1)
- backingchain[i].backingStore = &backingchain[i + 1];
+ virStorageSourceSetBackingStore(&backingchain[i], &backingchain[i +
1], 0);
else
- backingchain[i].backingStore = NULL;
+ virStorageSourceSetBackingStore(&backingchain[i], NULL, 0);
backingchain[i].relPath = NULL;
}
--
2.6.2
4:51 a.m.
New subject: [libvirt] [PATCH v7 04/13] virstoragefile: Always use virStorageSourceSetBackingStore to set backing store
On 12/03/2015 09:35 AM, Matthias Gatto wrote:
Replace the parts of the code where a backing store is set manually
with virStorageSourceSetBackingStore
Signed-off-by: Matthias Gatto <matthias.gatto(a)outscale.com>
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
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 | 5 +++--
src/storage/storage_driver.c | 3 ++-
src/util/virstoragefile.c | 8 +++++---
tests/virstoragetest.c | 4 ++--
9 files changed, 43 insertions(+), 26 deletions(-)
Upstream changes and changes from/for patch 2 make this even more
difficult. Rather than hit every change required and in order to make
some progress, it's perhaps easier to repost patches 1-5 and get them
accepted/upstream first. Then focus on the rest afterwards.
I'll cobble together something - it'll differ slightly from what's here.
John
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5b413b5..d146811 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6359,7 +6359,8 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt,
virDomainDiskBackingStoreParse(ctxt, backingStore) < 0)
goto cleanup;
- src->backingStore = backingStore;
+ if (virStorageSourceSetBackingStore(src, backingStore, 0) < 0)
+ goto cleanup;
ret = 0;
cleanup:
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index d048e39..b9db5eb 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -1259,6 +1259,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
char *capacity = NULL;
char *unit = NULL;
char *backingStore = NULL;
+ virStorageSourcePtr backingStorePtr;
xmlNodePtr node;
xmlNodePtr *nodes = NULL;
size_t i;
@@ -1295,20 +1296,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)
< 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);
@@ -1317,9 +1320,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") < 0)
goto error;
}
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8ba7566..edfd8e6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14261,13 +14261,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) < 0) {
+ ret = -1;
+ goto cleanup;
+ }
newDiskSrc = NULL;
if (persistDisk) {
- persistDiskSrc->backingStore = persistDisk->src;
- persistDisk->src = persistDiskSrc;
+ if (virStorageSourceSetBackingStore(persistDiskSrc,
+ persistDisk->src, 0) < 0) {
+ ret = -1;
+ goto cleanup;
+ }
persistDiskSrc = NULL;
}
@@ -14310,13 +14315,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/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index 7b05d4d..b216e91 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -97,7 +97,9 @@ virStorageBackendProbeTarget(virStorageSourcePtr target,
goto cleanup;
if (meta->backingStoreRaw) {
- if (!(target->backingStore = virStorageSourceNewFromBacking(meta)))
+ if (virStorageSourceSetBackingStore(target,
+ virStorageSourceNewFromBacking(meta),
+ 0) < 0)
goto cleanup;
backingStore = virStorageSourceGetBackingStore(target, 0);
@@ -112,7 +114,8 @@ virStorageBackendProbeTarget(virStorageSourcePtr target,
if (VIR_ALLOC(backingStore) < 0)
goto cleanup;
- target->backingStore = backingStore;
+ if (virStorageSourceSetBackingStore(target, backingStore, 0) < 0)
+ goto cleanup;
backingStore->type = VIR_STORAGE_TYPE_NETWORK;
backingStore->path = meta->backingStoreRaw;
meta->backingStoreRaw = NULL;
diff --git a/src/storage/storage_backend_gluster.c
b/src/storage/storage_backend_gluster.c
index 9bddb3b..8b89433 100644
--- a/src/storage/storage_backend_gluster.c
+++ b/src/storage/storage_backend_gluster.c
@@ -301,9 +301,10 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr
state,
goto cleanup;
if (meta->backingStoreRaw) {
- if (VIR_ALLOC(vol->target.backingStore) < 0)
+ if (VIR_ALLOC(backingStore) < 0)
+ goto cleanup;
+ if (virStorageSourceSetBackingStore(&vol->target, backingStore, 0) <
0)
goto cleanup;
- backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
backingStore->path = meta->backingStoreRaw;
diff --git a/src/storage/storage_backend_logical.c
b/src/storage/storage_backend_logical.c
index e2a287d..eca9730 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -149,10 +149,11 @@ virStorageBackendLogicalMakeVol(char **const groups,
* lv is created with "--virtualsize").
*/
if (groups[1] && STRNEQ(groups[1], "") && (groups[1][0] !=
'[')) {
- if (VIR_ALLOC(vol->target.backingStore) < 0)
+ if (VIR_ALLOC(backingStore) < 0)
goto cleanup;
- backingStore = virStorageSourceGetBackingStore(&vol->target, 0);
+ if (virStorageSourceSetBackingStore(&vol->target, backingStore, 0) <
0)
+ goto cleanup;
if (virAsprintf(&backingStore->path, "%s/%s",
pool->def->target.path, groups[1]) < 0)
goto cleanup;
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index bbf21f6..963e325 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -3064,7 +3064,8 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
goto cleanup;
}
- src->backingStore = backingStore;
+ if (virStorageSourceSetBackingStore(src, backingStore, 0) < 0)
+ goto cleanup;
backingStore = NULL;
ret = 0;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index a8a2134..1299f98 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1904,8 +1904,10 @@ virStorageSourceCopy(const virStorageSource *src,
goto error;
if (backingChain && virStorageSourceGetBackingStore(src, 0)) {
- if (!(ret->backingStore =
virStorageSourceCopy(virStorageSourceGetBackingStore(src, 0),
- true)))
+ if (virStorageSourceSetBackingStore(ret,
+
virStorageSourceCopy(virStorageSourceGetBackingStore(src, 0),
+ true),
+ 0) < 0)
goto error;
}
@@ -2046,7 +2048,7 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def)
/* recursively free backing chain */
virStorageSourceFree(virStorageSourceGetBackingStore(def, 0));
- def->backingStore = NULL;
+ ignore_value(virStorageSourceSetBackingStore(def, NULL, 0));
}
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index 5bd4637..58f505d 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -583,9 +583,9 @@ testPathRelativePrepare(void)
for (i = 0; i < ARRAY_CARDINALITY(backingchain); i++) {
if (i < ARRAY_CARDINALITY(backingchain) - 1)
- backingchain[i].backingStore = &backingchain[i + 1];
+ virStorageSourceSetBackingStore(&backingchain[i], &backingchain[i +
1], 0);
else
- backingchain[i].backingStore = NULL;
+ virStorageSourceSetBackingStore(&backingchain[i], NULL, 0);
backingchain[i].relPath = NULL;
}
10:35 p.m.
New subject: [libvirt] [PATCH v7 05/13] virstoragefile: change backingStore to backingStores.
The backingStore field was a virStorageSourcePtr.
Because a quorum can contain more that one backingStore at the same level,
it's now an array of 'virStorageSourcePtr'.
This patch rename src->backingStore to src->backingStores,
Made the necessary changes to virStorageSourceSetBackingStore
and virStorageSourceGetBackingStore.
virStorageSourceSetBackingStore can now expand the size of src->backingStores.
Signed-off-by: Matthias Gatto <matthias.gatto(a)outscale.com>
---
src/storage/storage_backend.c | 2 +-
src/storage/storage_backend_fs.c | 2 +-
src/util/virstoragefile.c | 66 +++++++++++++++++++++++++++++++---------
src/util/virstoragefile.h | 3 +-
4 files changed, 56 insertions(+), 17 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 08ed1dd..d71bb1a 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -498,7 +498,7 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
goto cleanup;
}
- if (vol->target.backingStore) {
+ if (vol->target.backingStores) {
virReportError(VIR_ERR_NO_SUPPORT, "%s",
_("backing storage not supported for raw volumes"));
goto cleanup;
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index b216e91..68419e3 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -1100,7 +1100,7 @@ static int createFileDir(virConnectPtr conn ATTRIBUTE_UNUSED,
return -1;
}
- if (vol->target.backingStore) {
+ if (vol->target.backingStores) {
virReportError(VIR_ERR_NO_SUPPORT, "%s",
_("backing storage not supported for directories
volumes"));
return -1;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 1299f98..8c05786 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1809,22 +1809,50 @@ virStorageSourcePoolDefCopy(const virStorageSourcePoolDef *src)
}
+/**
+ * virStorageSourceGetBackingStore:
+ * @src: virStorageSourcePtr containing the backing stores
+ * @pos: position of the backing store to get
+ *
+ * return the backingStore at the position of @pos
+ */
virStorageSourcePtr
-virStorageSourceGetBackingStore(const virStorageSource *src,
- size_t pos ATTRIBUTE_UNUSED)
+virStorageSourceGetBackingStore(const virStorageSource *src, size_t pos)
{
- if (!src)
+ if (!src || !src->backingStores || pos >= src->nBackingStores)
return NULL;
- return src->backingStore;
+ return src->backingStores[pos];
}
+/**
+ * virStorageSourceSetBackingStore:
+ * @src: virStorageSourcePtr to hold @backingStore
+ * @backingStore: backingStore to store
+ * @pos: position of the backing store to store
+ *
+ * Set @backingStore at @pos in src->backingStores.
+ * If src->backingStores don't have the space to contain
+ * @backingStore, we expand src->backingStores.
+ * If src->backingStores[pos] is alerady set, free it.
+ */
int
virStorageSourceSetBackingStore(virStorageSourcePtr src,
virStorageSourcePtr backingStore,
- size_t pos ATTRIBUTE_UNUSED)
+ size_t pos)
{
- src->backingStore = backingStore;
+ if (!src)
+ return -1;
+
+ if (pos >= src->nBackingStores) {
+ int nbr = pos - src->nBackingStores + 1;
+ if (VIR_EXPAND_N(src->backingStores, src->nBackingStores, nbr) < 0)
+ return -1;
+ }
+
+ if (src->backingStores[pos])
+ virStorageSourceFree(src->backingStores[pos]);
+ src->backingStores[pos] = backingStore;
return 0;
}
@@ -1843,6 +1871,7 @@ virStorageSourceCopy(const virStorageSource *src,
bool backingChain)
{
virStorageSourcePtr ret = NULL;
+ size_t i;
if (VIR_ALLOC(ret) < 0)
return NULL;
@@ -1855,6 +1884,8 @@ virStorageSourceCopy(const virStorageSource *src,
ret->physical = src->physical;
ret->readonly = src->readonly;
ret->shared = src->shared;
+ ret->nBackingStores = src->nBackingStores;
+ ret->threshold = src->threshold;
/* storage driver metadata are not copied */
ret->drv = NULL;
@@ -1903,12 +1934,14 @@ virStorageSourceCopy(const virStorageSource *src,
!(ret->auth = virStorageAuthDefCopy(src->auth)))
goto error;
- if (backingChain && virStorageSourceGetBackingStore(src, 0)) {
- if (virStorageSourceSetBackingStore(ret,
-
virStorageSourceCopy(virStorageSourceGetBackingStore(src, 0),
- true),
- 0) < 0)
- goto error;
+ for (i = 0; i < src->nBackingStores; ++i) {
+ if (backingChain && virStorageSourceGetBackingStore(src, i)) {
+ if (virStorageSourceSetBackingStore(ret,
+
virStorageSourceCopy(virStorageSourceGetBackingStore(src, i),
+ true),
+ 0) < 0)
+ goto error;
+ }
}
return ret;
@@ -2040,6 +2073,8 @@ virStorageSourceIsEmpty(virStorageSourcePtr src)
void
virStorageSourceBackingStoreClear(virStorageSourcePtr def)
{
+ size_t i;
+
if (!def)
return;
@@ -2047,8 +2082,11 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def)
VIR_FREE(def->backingStoreRaw);
/* recursively free backing chain */
- virStorageSourceFree(virStorageSourceGetBackingStore(def, 0));
- ignore_value(virStorageSourceSetBackingStore(def, NULL, 0));
+ for (i = 0; i < def->nBackingStores; ++i)
+ virStorageSourceFree(virStorageSourceGetBackingStore(def, i));
+ if (def->nBackingStores > 0)
+ VIR_FREE(def->backingStores);
+ def->nBackingStores = 0;
}
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index ce1cb5d..290c20f 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -270,7 +270,8 @@ struct _virStorageSource {
bool shared;
/* backing chain of the storage source */
- virStorageSourcePtr backingStore;
+ virStorageSourcePtr *backingStores;
+ size_t nBackingStores;
/* metadata for storage driver access to remote and local volumes */
virStorageDriverDataPtr drv;
--
2.6.2
4:55 a.m.
New subject: [libvirt] [PATCH v7 05/13] virstoragefile: change backingStore to backingStores.
On 12/03/2015 09:35 AM, Matthias Gatto wrote:
The backingStore field was a virStorageSourcePtr.
Because a quorum can contain more that one backingStore at the same level,
it's now an array of 'virStorageSourcePtr'.
This patch rename src->backingStore to src->backingStores,
Made the necessary changes to virStorageSourceSetBackingStore
and virStorageSourceGetBackingStore.
virStorageSourceSetBackingStore can now expand the size of src->backingStores.
Signed-off-by: Matthias Gatto <matthias.gatto(a)outscale.com>
---
src/storage/storage_backend.c | 2 +-
src/storage/storage_backend_fs.c | 2 +-
src/util/virstoragefile.c | 66 +++++++++++++++++++++++++++++++---------
src/util/virstoragefile.h | 3 +-
4 files changed, 56 insertions(+), 17 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 08ed1dd..d71bb1a 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -498,7 +498,7 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
goto cleanup;
}
- if (vol->target.backingStore) {
+ if (vol->target.backingStores) {
virReportError(VIR_ERR_NO_SUPPORT, "%s",
_("backing storage not supported for raw volumes"));
goto cleanup;
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index b216e91..68419e3 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -1100,7 +1100,7 @@ static int createFileDir(virConnectPtr conn ATTRIBUTE_UNUSED,
return -1;
}
- if (vol->target.backingStore) {
+ if (vol->target.backingStores) {
virReportError(VIR_ERR_NO_SUPPORT, "%s",
_("backing storage not supported for directories
volumes"));
return -1;
I would think these two accesses would need a read access rather than
changing from backingStore to backingStores. First clue would be that
now that everything is supposed to be contained within virstoragefile.c
so any change outside that has been something changed since you first
started this adventure.
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 1299f98..8c05786 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1809,22 +1809,50 @@ virStorageSourcePoolDefCopy(const virStorageSourcePoolDef *src)
}
+/**
+ * virStorageSourceGetBackingStore:
+ * @src: virStorageSourcePtr containing the backing stores
+ * @pos: position of the backing store to get
+ *
+ * return the backingStore at the position of @pos
+ */
virStorageSourcePtr
-virStorageSourceGetBackingStore(const virStorageSource *src,
- size_t pos ATTRIBUTE_UNUSED)
+virStorageSourceGetBackingStore(const virStorageSource *src, size_t pos)
{
- if (!src)
+ if (!src || !src->backingStores || pos >= src->nBackingStores)
return NULL;
- return src->backingStore;
+ return src->backingStores[pos];
}
+/**
+ * virStorageSourceSetBackingStore:
+ * @src: virStorageSourcePtr to hold @backingStore
+ * @backingStore: backingStore to store
+ * @pos: position of the backing store to store
+ *
+ * Set @backingStore at @pos in src->backingStores.
+ * If src->backingStores don't have the space to contain
+ * @backingStore, we expand src->backingStores.
+ * If src->backingStores[pos] is alerady set, free it.
+ */
int
virStorageSourceSetBackingStore(virStorageSourcePtr src,
virStorageSourcePtr backingStore,
- size_t pos ATTRIBUTE_UNUSED)
+ size_t pos)
{
- src->backingStore = backingStore;
+ if (!src)
+ return -1;
+
+ if (pos >= src->nBackingStores) {
+ int nbr = pos - src->nBackingStores + 1;
+ if (VIR_EXPAND_N(src->backingStores, src->nBackingStores, nbr) < 0)
+ return -1;
+ }
+
+ if (src->backingStores[pos])
+ virStorageSourceFree(src->backingStores[pos]);
+ src->backingStores[pos] = backingStore;
return 0;
}
@@ -1843,6 +1871,7 @@ virStorageSourceCopy(const virStorageSource *src,
bool backingChain)
{
virStorageSourcePtr ret = NULL;
+ size_t i;
if (VIR_ALLOC(ret) < 0)
return NULL;
@@ -1855,6 +1884,8 @@ virStorageSourceCopy(const virStorageSource *src,
ret->physical = src->physical;
ret->readonly = src->readonly;
ret->shared = src->shared;
+ ret->nBackingStores = src->nBackingStores;
+ ret->threshold = src->threshold;
This appears to be a rogue change... It causes my build to fail. Looks
like it's defined in patch 7 and further used in patch 8 - so those will
need an update once this is pushed.
John
/* storage driver metadata are not copied */
ret->drv = NULL;
@@ -1903,12 +1934,14 @@ virStorageSourceCopy(const virStorageSource *src,
!(ret->auth = virStorageAuthDefCopy(src->auth)))
goto error;
- if (backingChain && virStorageSourceGetBackingStore(src, 0)) {
- if (virStorageSourceSetBackingStore(ret,
-
virStorageSourceCopy(virStorageSourceGetBackingStore(src, 0),
- true),
- 0) < 0)
- goto error;
+ for (i = 0; i < src->nBackingStores; ++i) {
+ if (backingChain && virStorageSourceGetBackingStore(src, i)) {
+ if (virStorageSourceSetBackingStore(ret,
+
virStorageSourceCopy(virStorageSourceGetBackingStore(src, i),
+ true),
+ 0) < 0)
+ goto error;
+ }
}
return ret;
@@ -2040,6 +2073,8 @@ virStorageSourceIsEmpty(virStorageSourcePtr src)
void
virStorageSourceBackingStoreClear(virStorageSourcePtr def)
{
+ size_t i;
+
if (!def)
return;
@@ -2047,8 +2082,11 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def)
VIR_FREE(def->backingStoreRaw);
/* recursively free backing chain */
- virStorageSourceFree(virStorageSourceGetBackingStore(def, 0));
- ignore_value(virStorageSourceSetBackingStore(def, NULL, 0));
+ for (i = 0; i < def->nBackingStores; ++i)
+ virStorageSourceFree(virStorageSourceGetBackingStore(def, i));
+ if (def->nBackingStores > 0)
+ VIR_FREE(def->backingStores);
+ def->nBackingStores = 0;
}
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index ce1cb5d..290c20f 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -270,7 +270,8 @@ struct _virStorageSource {
bool shared;
/* backing chain of the storage source */
- virStorageSourcePtr backingStore;
+ virStorageSourcePtr *backingStores;
+ size_t nBackingStores;
/* metadata for storage driver access to remote and local volumes */
virStorageDriverDataPtr drv;
10:35 p.m.
New subject: [libvirt] [PATCH v7 06/13] virstoragefile: Add virStorageSourceIsRAID
Add a new function which return true if a virStorageSourcePtr is
a RAID.
For now, quorum is the only RAID we have.
This function is usefull, because, a lot of code access directly
to a virStorageSource internal member (like path) with some functions
like "virDomainDiskGetSource".
This beavious won't work with Quorum, and so we need to add
exeptions for these functions, but I'm not convinced by the idea to add a lot
of "disk->format == QUORUM" in all the code that deserve
exeption for Quorum, so I've add a generic function for this.
Signed-off-by: Matthias Gatto <matthias.gatto(a)outscale.com>
---
src/libvirt_private.syms | 1 +
src/util/virstoragefile.c | 27 +++++++++++++++++++++++++++
src/util/virstoragefile.h | 2 ++
3 files changed, 30 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d3baee8..571b6f7 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2195,6 +2195,7 @@ virStorageSourceGetSecurityLabelDef;
virStorageSourceInitChainElement;
virStorageSourceIsEmpty;
virStorageSourceIsLocalStorage;
+virStorageSourceIsRAID;
virStorageSourceNewFromBacking;
virStorageSourceParseRBDColonString;
virStorageSourcePoolDefFree;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 8c05786..0d27ca6 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1808,6 +1808,33 @@ virStorageSourcePoolDefCopy(const virStorageSourcePoolDef *src)
return NULL;
}
+/**
+ * virStorageSourceIsRAID:
+ * return true if the backingStores field inside @src is use
+ * as a child of a contener
+ */
+bool virStorageSourceIsRAID(virStorageSourcePtr src)
+{
+ virStorageType type;
+
+ if (!src)
+ return false;
+ type = virStorageSourceGetActualType(src);
+ switch (type) {
+ case VIR_STORAGE_TYPE_NONE:
+ case VIR_STORAGE_TYPE_FILE:
+ case VIR_STORAGE_TYPE_BLOCK:
+ case VIR_STORAGE_TYPE_DIR:
+ case VIR_STORAGE_TYPE_NETWORK:
+ case VIR_STORAGE_TYPE_VOLUME:
+ case VIR_STORAGE_TYPE_LAST:
+ return false;
+
+ case VIR_STORAGE_TYPE_QUORUM:
+ return true;
+ }
+ return false;
+}
/**
* virStorageSourceGetBackingStore:
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 290c20f..68a21d0 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -290,6 +290,8 @@ struct _virStorageSource {
# define DEV_BSIZE 512
# endif
+bool virStorageSourceIsRAID(virStorageSourcePtr src);
+
int virStorageSourceSetBackingStore(virStorageSourcePtr src,
virStorageSourcePtr backingStore,
size_t pos);
--
2.6.2
4:55 a.m.
New subject: [libvirt] [PATCH v7 06/13] virstoragefile: Add virStorageSourceIsRAID
On 12/03/2015 09:35 AM, Matthias Gatto wrote:
Add a new function which return true if a virStorageSourcePtr is
a RAID.
For now, quorum is the only RAID we have.
This function is usefull, because, a lot of code access directly
to a virStorageSource internal member (like path) with some functions
like "virDomainDiskGetSource".
This beavious won't work with Quorum, and so we need to add
exeptions for these functions, but I'm not convinced by the idea to add a lot
of "disk->format == QUORUM" in all the code that deserve
exeption for Quorum, so I've add a generic function for this.
Signed-off-by: Matthias Gatto <matthias.gatto(a)outscale.com>
---
src/libvirt_private.syms | 1 +
src/util/virstoragefile.c | 27 +++++++++++++++++++++++++++
src/util/virstoragefile.h | 2 ++
3 files changed, 30 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d3baee8..571b6f7 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2195,6 +2195,7 @@ virStorageSourceGetSecurityLabelDef;
virStorageSourceInitChainElement;
virStorageSourceIsEmpty;
virStorageSourceIsLocalStorage;
+virStorageSourceIsRAID;
virStorageSourceNewFromBacking;
virStorageSourceParseRBDColonString;
virStorageSourcePoolDefFree;
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 8c05786..0d27ca6 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1808,6 +1808,33 @@ virStorageSourcePoolDefCopy(const virStorageSourcePoolDef *src)
return NULL;
}
+/**
+ * virStorageSourceIsRAID:
+ * return true if the backingStores field inside @src is use
+ * as a child of a contener
+ */
+bool virStorageSourceIsRAID(virStorageSourcePtr src)
+{
+ virStorageType type;
+
+ if (!src)
+ return false;
+ type = virStorageSourceGetActualType(src);
+ switch (type) {
+ case VIR_STORAGE_TYPE_NONE:
+ case VIR_STORAGE_TYPE_FILE:
+ case VIR_STORAGE_TYPE_BLOCK:
+ case VIR_STORAGE_TYPE_DIR:
+ case VIR_STORAGE_TYPE_NETWORK:
+ case VIR_STORAGE_TYPE_VOLUME:
+ case VIR_STORAGE_TYPE_LAST:
+ return false;
+
+ case VIR_STORAGE_TYPE_QUORUM:
This isn't defined until patch 7...
John
I'm going to stop here, but since I saw this as well I figured I'd note
it. Each patch needs to be compilable and bisectable.
+ return true;
+ }
+ return false;
+}
/**
* virStorageSourceGetBackingStore:
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 290c20f..68a21d0 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -290,6 +290,8 @@ struct _virStorageSource {
# define DEV_BSIZE 512
# endif
+bool virStorageSourceIsRAID(virStorageSourcePtr src);
+
int virStorageSourceSetBackingStore(virStorageSourcePtr src,
virStorageSourcePtr backingStore,
size_t pos);
10:35 p.m.
New subject: [libvirt] [PATCH v7 07/13] virstoragefile: Add quorum in virstoragefile
Add VIR_STORAGE_TYPE_QUORUM in virStorageType.
Add VIR_STORAGE_FILE_QUORUM in virStorageFileFormat.
Add threshold value in _virStorageSource
Signed-off-by: Matthias Gatto <matthias.gatto(a)outscale.com>
---
docs/formatdomain.html.in | 23 ++++++++++++++++++++---
docs/schemas/domaincommon.rng | 21 ++++++++++++++++++++-
docs/schemas/storagecommon.rng | 1 +
docs/schemas/storagevol.rng | 1 +
src/conf/domain_conf.c | 2 ++
src/qemu/qemu_command.c | 1 +
src/qemu/qemu_driver.c | 4 ++++
src/qemu/qemu_migration.c | 1 +
src/util/virstoragefile.c | 8 ++++++--
src/util/virstoragefile.h | 3 +++
10 files changed, 59 insertions(+), 6 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index a8bd48e..9bef852 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1958,8 +1958,9 @@
<dd>
Valid values are "file", "block",
"dir" (<span class="since">since
0.7.5</span>),
- "network" (<span class="since">since
0.8.7</span>), or
- "volume" (<span class="since">since
1.0.5</span>)
+ "network" (<span class="since">since
0.8.7</span>),
+ "volume" (<span class="since">since
1.0.5</span>), or
+ "quorum" (<span class="since">since
1.3.1</span>)
and refer to the underlying source for the disk.
</dd>
<dt><code>device</code> attribute
@@ -2025,6 +2026,14 @@
<code>snapshot='yes'</code> with a transient disk
generally
does not make sense.
</dd>
+ <dt><code>threshold</code> attribute
+ <span class="since">since 1.3.1</span></dt>
+ <dd>
+ Only use with a quorum disk.
+ Indicate the minimum of positive vote a quorum must have to validate
+ a data to be write. The minimum value is "2". The number of
backingStores
+ contain by the quorum must be superior to the threshold.
+ </dd>
</dl>
</dd>
<dt><code>source</code></dt>
@@ -2102,6 +2111,11 @@
</p>
</dd>
+ <dt><code>type='quorum'</code>
+ <span class="since">since 1.3.1</span></dt>
+ <dd>
+ A quorum contain no source element, but a serie of backingStores instead.
+ </dd>
</dl>
With "file", "block", and "volume", one or more
optional
sub-elements <code>seclabel</code>, <a
href="#seclabel">described
@@ -2241,7 +2255,10 @@
property, but using existing external files for snapshot or
block copy operations requires the end user to pre-create the
file correctly). The following attributes and sub-elements are
- supported in <code>backingStore</code>:
+ supported in.
+ <span class="since">Since 1.3.1</span>. This elements is
used to
+ describe a quorum child.
+ <code>backingStore</code>:
<dl>
<dt><code>type</code> attribute</dt>
<dd>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 8d12606..ee08542 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -1240,9 +1240,15 @@
</interleave>
</define>
+ <define name="diskBackingStoreArray">
+ <zeroOrMore>
+ <ref name="diskBackingStore"/>
+ </zeroOrMore>
+ </define>
+
<define name="diskBackingChain">
<choice>
- <ref name="diskBackingStore"/>
+ <ref name="diskBackingStoreArray"/>
<ref name="diskBackingStoreLast"/>
</choice>
</define>
@@ -1284,9 +1290,22 @@
<ref name="diskSourceDir"/>
<ref name="diskSourceNetwork"/>
<ref name="diskSourceVolume"/>
+ <ref name="diskSourceQuorum"/>
</choice>
</define>
+ <define name="diskSourceQuorum">
+ <optional>
+ <attribute name="type">
+ <value>quorum</value>
+ </attribute>
+ <attribute name="threshold">
+ <ref name="unsignedInt"/>
+ </attribute>
+ </optional>
+ </define>
+
+
<define name="diskSourceFile">
<optional>
<attribute name="type">
diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng
index 7c04462..0ebc2ef 100644
--- a/docs/schemas/storagecommon.rng
+++ b/docs/schemas/storagecommon.rng
@@ -76,6 +76,7 @@
<value>fat</value>
<value>vhd</value>
<value>ploop</value>
+ <value>quorum</value>
<ref name='storageFormatBacking'/>
</choice>
</define>
diff --git a/docs/schemas/storagevol.rng b/docs/schemas/storagevol.rng
index 7450547..a718576 100644
--- a/docs/schemas/storagevol.rng
+++ b/docs/schemas/storagevol.rng
@@ -20,6 +20,7 @@
<value>dir</value>
<value>network</value>
<value>netdir</value>
+ <value>quorum</value>
</choice>
</attribute>
</optional>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d146811..e2a1870 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6281,6 +6281,7 @@ virDomainDiskSourceParse(xmlNodePtr node,
if (virDomainDiskSourcePoolDefParse(node, &src->srcpool) < 0)
goto cleanup;
break;
+ case VIR_STORAGE_TYPE_QUORUM:
case VIR_STORAGE_TYPE_NONE:
case VIR_STORAGE_TYPE_LAST:
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -18565,6 +18566,7 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf,
skipSeclabels);
break;
+ case VIR_STORAGE_TYPE_QUORUM:
case VIR_STORAGE_TYPE_NONE:
case VIR_STORAGE_TYPE_LAST:
virReportError(VIR_ERR_INTERNAL_ERROR,
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 4ff31dc..7e5a9ab 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3517,6 +3517,7 @@ qemuGetDriveSourceString(virStorageSourcePtr src,
goto cleanup;
break;
+ case VIR_STORAGE_TYPE_QUORUM:
case VIR_STORAGE_TYPE_VOLUME:
case VIR_STORAGE_TYPE_NONE:
case VIR_STORAGE_TYPE_LAST:
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index edfd8e6..4a56ebd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -13790,6 +13790,7 @@
qemuDomainSnapshotPrepareDiskExternalBackingInactive(virDomainDiskDefPtr disk)
}
break;
+ case VIR_STORAGE_TYPE_QUORUM:
case VIR_STORAGE_TYPE_DIR:
case VIR_STORAGE_TYPE_VOLUME:
case VIR_STORAGE_TYPE_NONE:
@@ -13853,6 +13854,7 @@
qemuDomainSnapshotPrepareDiskExternalOverlayActive(virDomainSnapshotDiskDefPtr d
}
break;
+ case VIR_STORAGE_TYPE_QUORUM:
case VIR_STORAGE_TYPE_DIR:
case VIR_STORAGE_TYPE_VOLUME:
case VIR_STORAGE_TYPE_NONE:
@@ -13877,6 +13879,7 @@
qemuDomainSnapshotPrepareDiskExternalOverlayInactive(virDomainSnapshotDiskDefPtr
case VIR_STORAGE_TYPE_FILE:
return 0;
+ case VIR_STORAGE_TYPE_QUORUM:
case VIR_STORAGE_TYPE_NETWORK:
case VIR_STORAGE_TYPE_DIR:
case VIR_STORAGE_TYPE_VOLUME:
@@ -13996,6 +13999,7 @@ qemuDomainSnapshotPrepareDiskInternal(virConnectPtr conn,
}
break;
+ case VIR_STORAGE_TYPE_QUORUM:
case VIR_STORAGE_TYPE_DIR:
case VIR_STORAGE_TYPE_VOLUME:
case VIR_STORAGE_TYPE_NONE:
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 4519aef..4fa7a2b 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1554,6 +1554,7 @@ qemuMigrationPrecreateDisk(virConnectPtr conn,
case VIR_STORAGE_TYPE_BLOCK:
case VIR_STORAGE_TYPE_DIR:
+ case VIR_STORAGE_TYPE_QUORUM:
case VIR_STORAGE_TYPE_NONE:
case VIR_STORAGE_TYPE_LAST:
virReportError(VIR_ERR_INTERNAL_ERROR,
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 0d27ca6..2a6a9fe 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -54,7 +54,8 @@ VIR_ENUM_IMPL(virStorage, VIR_STORAGE_TYPE_LAST,
"block",
"dir",
"network",
- "volume")
+ "volume",
+ "quorum")
VIR_ENUM_IMPL(virStorageFileFormat,
VIR_STORAGE_FILE_LAST,
@@ -63,7 +64,7 @@ VIR_ENUM_IMPL(virStorageFileFormat,
"cloop", "dmg", "iso",
"vpc", "vdi",
/* Not direct file formats, but used for various drivers */
- "fat", "vhd", "ploop",
+ "fat", "vhd", "ploop", "quorum",
/* Formats with backing file below here */
"cow", "qcow", "qcow2", "qed",
"vmdk")
@@ -2052,6 +2053,9 @@ virStorageSourceIsLocalStorage(virStorageSourcePtr src)
case VIR_STORAGE_TYPE_FILE:
case VIR_STORAGE_TYPE_BLOCK:
case VIR_STORAGE_TYPE_DIR:
+ /* A quorum is actually both, but we consider it as local
+ * because it keep us to add a lot of exeption in the code*/
+ case VIR_STORAGE_TYPE_QUORUM:
return true;
case VIR_STORAGE_TYPE_NETWORK:
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 68a21d0..c9c40ce 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -49,6 +49,7 @@ typedef enum {
VIR_STORAGE_TYPE_DIR,
VIR_STORAGE_TYPE_NETWORK,
VIR_STORAGE_TYPE_VOLUME,
+ VIR_STORAGE_TYPE_QUORUM,
VIR_STORAGE_TYPE_LAST
} virStorageType;
@@ -73,6 +74,7 @@ typedef enum {
VIR_STORAGE_FILE_FAT,
VIR_STORAGE_FILE_VHD,
VIR_STORAGE_FILE_PLOOP,
+ VIR_STORAGE_FILE_QUORUM,
/* Not a format, but a marker: all formats below this point have
* libvirt support for following a backing chain */
@@ -272,6 +274,7 @@ struct _virStorageSource {
/* backing chain of the storage source */
virStorageSourcePtr *backingStores;
size_t nBackingStores;
+ size_t threshold;
/* metadata for storage driver access to remote and local volumes */
virStorageDriverDataPtr drv;
--
2.6.2
10:35 p.m.
New subject: [libvirt] [PATCH v7 08/13] domain_conf: Read and Write quorum config
Add the capabiltty to libvirt to parse and format the quorum syntax
as described here:
http://www.redhat.com/archives/libvir-list/2014-May/msg00533.html
We need a different unique index for each backing store
to manipulate individually each child of a quorum.
If a disk have 2 childs A and B, the index will be 1 for A and 2 for B,
but if A have 2 childs C and D, so A will have the index 1, C and D: 2 and 3
and B will be 4.
Here is a representation of our disk tree and they indexs:
C[2]
/
A[1]
/ \D[3]
HDA
\
B[4]
Signed-off-by: Matthias Gatto <matthias.gatto(a)outscale.com>
---
src/conf/domain_conf.c | 157 +++++++++++++++++++++++++++++++++----------------
1 file changed, 106 insertions(+), 51 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e2a1870..5178e57 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6306,17 +6306,45 @@ virDomainDiskSourceParse(xmlNodePtr node,
static int
+virDomainDiskThresholdParse(virStorageSourcePtr src,
+ xmlXPathContextPtr ctxt)
+{
+ char *threshold = virXPathString("string(./@threshold)", ctxt);
+ int ret;
+
+ if (!threshold)
+ return 0;
+ ret = virStrToLong_ul(threshold, NULL, 10, &src->threshold);
+ if (ret < 0 || src->threshold < 2) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("threshold must be a decimal number greater than 2 "
+ "and less than the number of children"));
+ VIR_FREE(threshold);
+ return -1;
+ }
+ VIR_FREE(threshold);
+ return 0;
+}
+
+#define VIR_DOMAIN_DISK_BACKING_STORE_PARSE_RECALL 1
+
+static int
virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt,
- virStorageSourcePtr src)
+ virStorageSourcePtr src,
+ size_t pos)
{
virStorageSourcePtr backingStore = NULL;
xmlNodePtr save_ctxt = ctxt->node;
xmlNodePtr source;
char *type = NULL;
char *format = NULL;
+ char *path;
int ret = -1;
- if (!(ctxt->node = virXPathNode("./backingStore[*]", ctxt))) {
+ if (virAsprintf(&path, "./backingStore[%lu][*]", pos + 1) < 0)
+ return -1;
+
+ if (!(ctxt->node = virXPathNode(path, ctxt))) {
ret = 0;
goto cleanup;
}
@@ -6350,29 +6378,35 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt,
goto cleanup;
}
- if (!(source = virXPathNode("./source", ctxt))) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("missing disk backing store source"));
+ source = virXPathNode("./source", ctxt);
+
+ if (source && virDomainDiskSourceParse(source, ctxt, backingStore) < 0)
goto cleanup;
- }
- if (virDomainDiskSourceParse(source, ctxt, backingStore) < 0 ||
- virDomainDiskBackingStoreParse(ctxt, backingStore) < 0)
+ if (virDomainDiskBackingStoreParse(ctxt, backingStore, 0) < 0)
goto cleanup;
- if (virStorageSourceSetBackingStore(src, backingStore, 0) < 0)
+ if (virDomainDiskThresholdParse(backingStore, ctxt) < 0)
goto cleanup;
- ret = 0;
+
+ if (virStorageSourceSetBackingStore(src, backingStore, pos) < 0)
+ goto cleanup;
+
+ ret = VIR_DOMAIN_DISK_BACKING_STORE_PARSE_RECALL;
cleanup:
if (ret < 0)
virStorageSourceFree(backingStore);
+ VIR_FREE(path);
VIR_FREE(type);
VIR_FREE(format);
ctxt->node = save_ctxt;
- return ret;
+ if (ret != VIR_DOMAIN_DISK_BACKING_STORE_PARSE_RECALL)
+ return ret;
+ return virDomainDiskBackingStoreParse(ctxt, src, pos + 1);
}
+#undef VIR_DOMAIN_DISK_BACKING_STORE_PARSE_RECALL
#define VENDOR_LEN 8
#define PRODUCT_LEN 16
@@ -6902,12 +6936,16 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
def->device = VIR_DOMAIN_DISK_DEVICE_DISK;
}
+ if (virDomainDiskThresholdParse(def->src, ctxt) < 0)
+ goto error;
+
/* Only CDROM and Floppy devices are allowed missing source path
* to indicate no media present. LUN is for raw access CD-ROMs
* that are not attached to a physical device presently */
if (virStorageSourceIsEmpty(def->src) &&
(def->device == VIR_DOMAIN_DISK_DEVICE_DISK ||
- (flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE))) {
+ (flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE)) &&
+ !(virStorageSourceIsRAID(def->src))) {
virReportError(VIR_ERR_NO_SOURCE,
target ? "%s" : NULL, target);
goto error;
@@ -7250,10 +7288,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
}
}
- if (!(flags & VIR_DOMAIN_DEF_PARSE_DISK_SOURCE)) {
- if (virDomainDiskBackingStoreParse(ctxt, def->src) < 0)
- goto error;
- }
+ if (virDomainDiskBackingStoreParse(ctxt, def->src, 0) < 0)
+ goto error;
cleanup:
VIR_FREE(bus);
@@ -18591,12 +18627,14 @@ virDomainDiskSourceFormat(virBufferPtr buf,
static int
virDomainDiskBackingStoreFormat(virBufferPtr buf,
- virStorageSourcePtr backingStore,
- const char *backingStoreRaw,
- unsigned int idx)
+ virStorageSourcePtr src,
+ unsigned int *idx)
{
+ const char *backingStoreRaw = src->backingStoreRaw;
+ virStorageSourcePtr backingStore = virStorageSourceGetBackingStore(src, 0);
const char *type;
const char *format;
+ size_t i;
if (!backingStore) {
if (!backingStoreRaw)
@@ -18604,37 +18642,48 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf,
return 0;
}
- if (!backingStore->type ||
- !(type = virStorageTypeToString(backingStore->type))) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected disk backing store type %d"),
- backingStore->type);
- return -1;
- }
+ for (i = 0; i < src->nBackingStores; ++i) {
+ backingStore = virStorageSourceGetBackingStore(src, i);
- if (backingStore->format <= 0 ||
- !(format = virStorageFileFormatTypeToString(backingStore->format))) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected disk backing store format %d"),
- backingStore->format);
- return -1;
- }
+ if (!backingStore) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("expected disk backing store"));
+ return -1;
+ }
- virBufferAsprintf(buf, "<backingStore type='%s'
index='%u'>\n",
- type, idx);
- virBufferAdjustIndent(buf, 2);
+ if (!backingStore->type ||
+ !(type = virStorageTypeToString(backingStore->type))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unexpected disk backing store type %d"),
+ backingStore->type);
+ return -1;
+ }
- virBufferAsprintf(buf, "<format type='%s'/>\n", format);
- /* We currently don't output seclabels for backing chain element */
- if (virDomainDiskSourceFormatInternal(buf, backingStore, 0, 0, true) < 0 ||
- virDomainDiskBackingStoreFormat(buf,
- virStorageSourceGetBackingStore(backingStore,
0),
- backingStore->backingStoreRaw,
- idx + 1) < 0)
- return -1;
+ if (backingStore->format <= 0 ||
+ !(format = virStorageFileFormatTypeToString(backingStore->format))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unexpected disk backing store format %d"),
+ backingStore->format);
+ return -1;
+ }
- virBufferAdjustIndent(buf, -2);
- virBufferAddLit(buf, "</backingStore>\n");
+ *idx += 1;
+ virBufferAsprintf(buf, "<backingStore type='%s'
index='%u'",
+ type, *idx);
+ if (backingStore->threshold)
+ virBufferAsprintf(buf, " threshold='%lu'",
backingStore->threshold);
+ virBufferAddLit(buf, ">\n");
+ virBufferAdjustIndent(buf, 2);
+
+ virBufferAsprintf(buf, "<format type='%s'/>\n", format);
+ /* We currently don't output seclabels for backing chain element */
+ if (virDomainDiskSourceFormatInternal(buf, backingStore, 0, 0, true) < 0 ||
+ virDomainDiskBackingStoreFormat(buf, backingStore, idx) < 0)
+ return -1;
+
+ virBufferAdjustIndent(buf, -2);
+ virBufferAddLit(buf, "</backingStore>\n");
+ }
return 0;
}
@@ -18704,6 +18753,10 @@ virDomainDiskDefFormat(virBufferPtr buf,
def->src->readonly))
virBufferAsprintf(buf, " snapshot='%s'",
virDomainSnapshotLocationTypeToString(def->snapshot));
+ if (def->src->threshold) {
+ virBufferAsprintf(buf, " threshold='%lu'",
+ def->src->threshold);
+ }
virBufferAddLit(buf, ">\n");
virBufferAdjustIndent(buf, 2);
@@ -18748,11 +18801,13 @@ 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,
- virStorageSourceGetBackingStore(def->src, 0),
- def->src->backingStoreRaw, 1) < 0)
- return -1;
+ if (!(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE) ||
+ virStorageSourceIsRAID(def->src)) {
+ unsigned int idx = 0;
+
+ if (virDomainDiskBackingStoreFormat(buf, def->src, &idx) < 0)
+ return -1;
+ }
virBufferEscapeString(buf, "<backenddomain name='%s'/>\n",
def->domain_name);
--
2.6.2
10:35 p.m.
New subject: [libvirt] [PATCH v7 09/13] qemu: Add quorum support in qemuBuildDriveDevStr
Allow libvirt to build the quorum string use by qemu.
Add 2astatic function: qemuBuildRAIDStr
qemuBuildRAIDStr is made because a quorum can have another quorum
as a child, so we may need to call qemuBuildRAIDStr recursively.
Signed-off-by: Matthias Gatto <matthias.gatto(a)outscale.com>
---
src/qemu/qemu_command.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 77 insertions(+)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 7e5a9ab..c7d554b 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3596,6 +3596,77 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk)
}
+static int
+qemuBuildRAIDStr(virConnectPtr conn,
+ virDomainDiskDefPtr disk,
+ virStorageSourcePtr src,
+ virBuffer *opt,
+ const char *prefix)
+{
+ char *tmp = NULL;
+ int ret;
+ virStorageSourcePtr backingStore;
+ size_t i;
+ int actualType = virStorageSourceGetActualType(src);
+ char *source = NULL;
+
+ if (actualType == VIR_STORAGE_TYPE_QUORUM) {
+ if (!src->threshold) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("threshold missing in the quorum
configuration"));
+ return -1;
+ }
+ if (src->nBackingStores < 2) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("a quorum must have at last 2 children"));
+ return -1;
+ }
+ if (src->threshold > src->nBackingStores) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("threshold must not exceed the number of
children"));
+ return -1;
+ }
+ virBufferAsprintf(opt, ",%svote-threshold=%lu",
+ prefix, src->threshold);
+ } else if (actualType == VIR_STORAGE_TYPE_DIR) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unsupported disk driver type for '%s'"),
+ virStorageFileFormatTypeToString(src->format));
+ return -1;
+ }
+
+ for (i = 0; i < src->nBackingStores; ++i) {
+ backingStore = virStorageSourceGetBackingStore(src, i);
+ ret = virAsprintf(&tmp, "%schildren.%lu.", prefix, i);
+ if (ret < 0)
+ return -1;
+
+ virBufferAsprintf(opt, ",%schildren.%lu.driver=%s",
+ prefix, i,
+ virStorageFileFormatTypeToString(backingStore->format));
+
+ if (qemuGetDriveSourceString(backingStore, conn, &source) < 0)
+ goto error;
+
+ if (source) {
+ virBufferStrcat(opt, ",", tmp, "file.filename=", NULL);
+ virBufferAdd(opt, source, -1);
+ }
+
+ /* This operation avoid us to made another copy */
+ if (virStorageSourceIsRAID(backingStore)) {
+ if (qemuBuildRAIDStr(conn, disk, backingStore, opt, tmp) < 0)
+ goto error;
+ }
+ VIR_FREE(tmp);
+ }
+ return 0;
+ error:
+ VIR_FREE(tmp);
+ return -1;
+}
+
+
/* Check whether the device address is using either 'ccw' or default s390
* address format and whether that's "legal" for the current qemu and/or
* guest os.machine type. This is the corollary to the code which doesn't
@@ -3764,6 +3835,7 @@ qemuBuildDriveStr(virConnectPtr conn,
goto error;
if (source &&
+ !virStorageSourceIsRAID(disk->src) &&
!((disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY ||
disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) &&
disk->tray_status == VIR_DOMAIN_DISK_TRAY_OPEN)) {
@@ -4110,6 +4182,11 @@ qemuBuildDriveStr(virConnectPtr conn,
disk->blkdeviotune.size_iops_sec);
}
+ if (virStorageSourceIsRAID(disk->src)) {
+ if (qemuBuildRAIDStr(conn, disk, disk->src, &opt, "") < 0)
+ goto error;
+ }
+
if (virBufferCheckError(&opt) < 0)
goto error;
--
2.6.2
10:35 p.m.
New subject: [libvirt] [PATCH v7 10/13] domain: Add virDomainDefHasRAID
This function check if a domain has a RAID as a disk.
This function is useful to block snapshot operations on domain
which contain quorum.
Signed-off-by: Matthias Gatto <matthias.gatto(a)outscale.com>
---
src/conf/domain_conf.c | 13 +++++++++++++
src/conf/domain_conf.h | 1 +
src/libvirt_private.syms | 1 +
3 files changed, 15 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5178e57..69d916d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2573,6 +2573,19 @@ virDomainDefNewFull(const char *name,
}
+bool
+virDomainDefHasRAID(virDomainDefPtr def)
+{
+ size_t i;
+
+ for (i = 0; i < def->ndisks; ++i) {
+ if (virStorageSourceIsRAID(def->disks[i]->src))
+ return true;
+ }
+ return true;
+}
+
+
void virDomainObjAssignDef(virDomainObjPtr domain,
virDomainDefPtr def,
bool live,
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 90d8e13..d193ab4 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2560,6 +2560,7 @@ void virDomainDefClearPCIAddresses(virDomainDefPtr def);
void virDomainDefClearCCWAddresses(virDomainDefPtr def);
void virDomainDefClearDeviceAliases(virDomainDefPtr def);
void virDomainTPMDefFree(virDomainTPMDefPtr def);
+bool virDomainDefHasRAID(virDomainDefPtr def);
typedef int (*virDomainDeviceInfoCallback)(virDomainDefPtr def,
virDomainDeviceDefPtr dev,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 571b6f7..a0c1a88 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -219,6 +219,7 @@ virDomainDefGetMemoryInitial;
virDomainDefGetSecurityLabelDef;
virDomainDefHasDeviceAddress;
virDomainDefHasMemoryHotplug;
+virDomainDefHasRAID;
virDomainDefMaybeAddController;
virDomainDefMaybeAddInput;
virDomainDefNeedsPlacementAdvice;
--
2.6.2
10:35 p.m.
New subject: [libvirt] [PATCH v7 11/13] qemu: Forbid blocks operations with quorum
For now we block all blocks operations with RAID disks.
Quorum doesn't support BlockRebase neither, but
qemuDomainBlockRebase call qemuDomainBlockPullCommon or
qemuDomainBlockCopyCommon which are alerady blocked.
Signed-off-by: Matthias Gatto <matthias.gatto(a)outscale.com>
---
src/qemu/qemu_driver.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4a56ebd..d0f7866 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14682,6 +14682,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
+ if (virDomainDefHasRAID(vm->def)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Snapshot does not support domain with RAID(like quorum)
yet"));
+ goto cleanup;
+ }
+
if (qemuProcessAutoDestroyActive(driver, vm)) {
virReportError(VIR_ERR_OPERATION_INVALID,
"%s", _("domain is marked for auto
destroy"));
@@ -16300,6 +16306,13 @@ qemuDomainBlockPullCommon(virQEMUDriverPtr driver,
goto endjob;
disk = vm->def->disks[idx];
+ if (virStorageSourceIsRAID(disk->src)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("block pull is not yet supported with disk of format
'%s'"),
+ virStorageFileFormatTypeToString(disk->src->format));
+ goto endjob;
+ }
+
if (qemuDomainDiskBlockJobIsActive(disk))
goto endjob;
@@ -16411,6 +16424,13 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
goto endjob;
disk = vm->def->disks[idx];
+ if (virStorageSourceIsRAID(disk->src)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("block job is not yes supported with disk of format
'%s'"),
+ virStorageFileFormatTypeToString(disk->src->format));
+ goto endjob;
+ }
+
if (disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_NONE &&
disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_READY) {
virReportError(VIR_ERR_OPERATION_INVALID,
@@ -16702,6 +16722,14 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
_("block copy is not supported with this QEMU
binary"));
goto endjob;
}
+
+ if (virStorageSourceIsRAID(disk->src)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("block copy is not yet supported with disk of format
'%s'"),
+ virStorageFileFormatTypeToString(disk->src->format));
+ goto endjob;
+ }
+
if (vm->persistent) {
/* XXX if qemu ever lets us start a new domain with mirroring
* already active, we can relax this; but for now, the risk of
--
2.6.2
10:35 p.m.
New subject: [libvirt] [PATCH v7 12/13] qemu: qemuDomainGetBlockInfo quorum support
Signed-off-by: Matthias Gatto <matthias.gatto(a)outscale.com>
---
src/qemu/qemu_driver.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d0f7866..44ce90f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11834,7 +11834,8 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
goto endjob;
}
- if (virStorageSourceIsEmpty(disk->src)) {
+ if (!virStorageSourceIsRAID(disk->src) &&
+ virStorageSourceIsEmpty(disk->src)) {
virReportError(VIR_ERR_INVALID_ARG,
_("disk '%s' does not currently have a source
assigned"),
path);
--
2.6.2
10:35 p.m.
New subject: [libvirt] [PATCH v7 13/13] qemu: qemuDiskPathToAlias quorum support
By adding quorum support to qemuDiskPathToAlias, we're adding support to
qemuDomainGetBlkioParameters, which was returning an error when the domain
was active.
Signed-off-by: Matthias Gatto <matthias.gatto(a)outscale.com>
---
src/qemu/qemu_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 44ce90f..69dfed6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16126,7 +16126,7 @@ qemuDiskPathToAlias(virDomainObjPtr vm, const char *path, int
*idxret)
if (idxret)
*idxret = idx;
- if (virDomainDiskGetSource(disk)) {
+ if (virDomainDiskGetSource(disk) || virStorageSourceIsRAID(disk->src)) {
if (virAsprintf(&ret, "drive-%s", disk->info.alias) < 0)
return NULL;
}
--
2.6.2
4:57 p.m.
ping
On Thu, Dec 3, 2015 at 3:35 PM, Matthias Gatto
<matthias.gatto(a)outscale.com> wrote:
The purpose of these patches is to introduce quorum for libvirt
I've try to follow this proposal:
http://www.redhat.com/archives/libvir-list/2014-May/msg00533.html
This feature ask for 5 task:
1) Allow a _virStorageSource to contain more than one backing store.
Because all the actual libvirt code use the backingStore field
as a pointer and we needs want to change that, I've decide to encapsulate
the backingStore field to simplifie the array manipulation.
2) Add the missing field a quorum need in _virStorageSource and
the VIR_STORAGE_TYPE_QUORUM and VIR_STORAGE_FILE_QUORUM in
their respectives enums.
3) Parse and format the xml
Because a quorum allows to have more than one backing store at the same level
we need to change virDomainDiskDefFormat and virDomainDiskDefParseXML
to call virDomainDiskBackingStoreFormat and virDomainDiskBackingStoreParse
in a loop.
virDomainDiskBackingStoreFormat and virDomainDiskBackingStoreParse can
call themself recursively in a loop because a quorum can contain another
quorum
4) Build qemu string
As for the xml, we have to call the function which create quorum recursively.
But this task have the problem explained here:
http://www.redhat.com/archives/libvir-list/2014-October/msg00529.html
The _virStorageSource missing some informations that can be passed to
a child, and therefore this version of quorum is incomplet.
5) Allow to hotplug/change a disk in a quorum
This part is not present in these patches because for this task
we have to use blockdev-add, and currently libvirt use
device_add for hotpluging that doesn't allow to hotplug quorum childs.
V2:
-Rebase on master
-Add Documentation
V3:
-Transforme the backingStore field in virStorageSource into
an array of pointer instead of a pointer
-Modify virStorageSourceSetBackingStore to allow it to expand
the backingStore size.
V4:
-Rebase on master
V5:
-Rebase on master
-patch 1-4/9: use patchs from John Ferlan
-patch 4/9: check return of virStorageSourceSetBackingStore
-patch 5/9: report type of error on virStorageSourceSetBackingStore
v6:
-Rebase on master
-Remove node-name patch
-patch 06/13: Add virStorageSourceIsRAID
-patch 10/13: Add virDomainDefHasRAID
-patch 11-13/13: Block all unsupported operations
v7:
-Rebase on master.
-Parse unconditionally backing store.
-fold qemuBuildRAIDFileSourceStr into qemuBuildRAIDStr.
-use 0/-1 return values when failing instead of bool.
-virStorageSourceSetBackingStore now free backing store when they are
already set.
Matthias Gatto (13):
virstoragefile: Add virStorageSourceGetBackingStore
virstoragefile: Always use virStorageSourceGetBackingStore to get
backing store
virstoragefile: Add virStorageSourceSetBackingStore
virstoragefile: Always use virStorageSourceSetBackingStore to set
backing store
virstoragefile: change backingStore to backingStores.
virstoragefile: Add virStorageSourceIsRAID
virstoragefile: Add quorum in virstoragefile
domain_conf: Read and Write quorum config
qemu: Add quorum support in qemuBuildDriveDevStr
domain: Add virDomainDefHasRAID
qemu: Forbid blocks operations with quorum
qemu: qemuDomainGetBlockInfo quorum support
qemu: qemuDiskPathToAlias quorum support
docs/formatdomain.html.in | 23 ++++-
docs/schemas/domaincommon.rng | 21 +++-
docs/schemas/storagecommon.rng | 1 +
docs/schemas/storagevol.rng | 1 +
src/conf/domain_conf.c | 174 ++++++++++++++++++++++++----------
src/conf/domain_conf.h | 1 +
src/conf/storage_conf.c | 23 +++--
src/libvirt_private.syms | 4 +
src/qemu/qemu_cgroup.c | 4 +-
src/qemu/qemu_command.c | 78 +++++++++++++++
src/qemu/qemu_domain.c | 2 +-
src/qemu/qemu_driver.c | 72 ++++++++++----
src/qemu/qemu_migration.c | 1 +
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 | 24 +++--
src/storage/storage_backend_fs.c | 45 +++++----
src/storage/storage_backend_gluster.c | 11 ++-
src/storage/storage_backend_logical.c | 15 ++-
src/storage/storage_driver.c | 3 +-
src/util/virstoragefile.c | 121 ++++++++++++++++++++---
src/util/virstoragefile.h | 15 ++-
tests/virstoragetest.c | 18 ++--
25 files changed, 518 insertions(+), 151 deletions(-)
--
2.6.2
1:20 a.m.
On Thu, Dec 03, 2015 at 15:35:10 +0100, Matthias Gatto wrote:
The purpose of these patches is to introduce quorum for libvirt
I've try to follow this proposal:
http://www.redhat.com/archives/libvir-list/2014-May/msg00533.html
TL;DR: I'm concerned that the quorum implementation is not really useful
and will introduce a lot of code with little benefit.
---
So I have a few comments/observations regarding the quorum block driver
in qemu and it's usability.
At first I'd like to as you to describe your use case a bit more. I'm
currently lacking the motivation to do anything about this, as the
series is just partial and I don't really see any advantage of using the
qorum driver at all and can't come up with any useful use case.
Also a good use case is usually a good reason to drive development of a
feature and I'm afraid that this could become abandoned without any real
use.
My problems with supporting the quorum backend are:
1) No traking of integrity
As the quorum members don't have headers, failed quorum members are
not recorded and remembered. The user or management app then has to
do this externally for given storage devices.
2) No internal tracking of quorum members
Members of the quorum don't have any header marking them as such and
thus any images may be mixed together with unforseen/catastrophic
results. Higher level management then needs to take the role of
remembering which images belong together. Reimplementing this looks
like reimplementing a distriuted storage system to me.
3) Lack of auto-resync:
Once the quorum get's few inconsistencies it does not automatically
resync like the linux MD driver. With the current implementation the
only way to resync this would be to issue a block-mirror (blockCopy)
to /dev/null so that all blocks are read and rewritten to the
identical copy. This also requires a user action.
Additionally the member of the quorum is not ignored if it was out
of sync in any previous time without being resynced
allowing for split-brain/corruption scenarios.
4) Necessity for at least 3 copies
Since a majority needs to win in a vote, you need at least 3 member
disks for this to be fault-tolerant.
5) Lack of speedup
Since always all blocks are read from all members and verified the
quorum backend doesn't really add any speed to the reads. This can
be mostly attributed to the fact that fault tracking is not present.
In other cases, due to internal error correcting codes it's very
unlikely that a storage medium would return a corrupted sector
without producing a error.
6) Almost every remote storage technology does quorums internally
Any distributed storage (ceph/rbd, gluster, sheepdog, etc..) provide
the quorum functionality internally with added benefit that their
internal working fixes problems when split of the network occurs.
7) Tools are restricted to qemu and qemu-img
It's a "proprietary" implementation so for a rebuild you have to use
one of the two tools. AFAIK qemu-img is not really user friendly for
the less common disk backends and we don't really provide any
abstraction on top of that. This means that there really aren't any
reasonable tools to do a offline resync. (Okay, if you know which
instance is okay, you can just copy it ...)
This series also lacks implementation of any user/maganement warning
method that a block operation didn't have 100% votes in the quorum
voting thus it's not really possible for the users to do a
rebuild/diagnostic if something fails.
Peter
10:47 p.m.
Hi Peter,
I'm the current maintainer of Quorum in QEMU and I'd like to try to
answer some of your comments.
On Fri, Jan 08, 2016 at 06:20:04PM +0100, Peter Krempa wrote:
So I have a few comments/observations regarding the quorum block
driver in qemu and it's usability.
At first I'd like to as you to describe your use case a bit
more. I'm currently lacking the motivation to do anything about
this, as the series is just partial and I don't really see any
advantage of using the qorum driver at all and can't come up with
any useful use case.
Also a good use case is usually a good reason to drive development
of a feature and I'm afraid that this could become abandoned without
any real use.
The original use case for which Quorum was designed was a data center
doing redundancy with storage in multiple separate rooms shared using
NFS.
One of the issues that the customer was facing was not only problems
in the file servers themselves but -mainly- data corruption accross
the network. Quorum can correct this on the fly and is able to
identify which one of the file servers is causing the problem without
having to rebuild a whole array (like it would be the case with RAID).
Quorum is also used for the COLO block replication functionality
currently being discussed in QEMU:
http://wiki.qemu.org/Features/BlockReplication
1) No traking of integrity
As the quorum members don't have headers, failed quorum members
are not recorded and remembered. The user or management app then
has to do this externally for given storage devices.
2) No internal tracking of quorum members
Members of the quorum don't have any header marking them
as such and thus any images may be mixed together with
unforseen/catastrophic results. Higher level management then
needs to take the role of remembering which images belong
together. Reimplementing this looks like reimplementing a
distriuted storage system to me.
That's right, Quorum does not have its own file format and was
designed to work with any driver or protocol that QEMU supports, so
I'm not sure if there's much that can be done about this.
3) Lack of auto-resync:
Once the quorum get's few inconsistencies it does not
automatically resync like the linux MD driver. With the current
implementation the only way to resync this would be to issue a
block-mirror (blockCopy) to /dev/null so that all blocks are
read and rewritten to the identical copy. This also requires a
user action.
Additionally the member of the quorum is not ignored if it was
out of sync in any previous time without being resynced allowing
for split-brain/corruption scenarios.
Quorum can fix errors on the fly (there's the 'rewrite-corrupted' flag
for that), so in those cases no manual intervention is required.
If we want a way to auto-resync a complete image that should be
doable, I believe it's relatively simple to implement in QEMU
(depending on the semantics).
For the manual resync I also agree that it would be good to have a
simple API to do that in case the user wants to do it manually. That
can be done.
4) Necessity for at least 3 copies
Since a majority needs to win in a vote, you need at least 3
member disks for this to be fault-tolerant.
5) Lack of speedup
Since always all blocks are read from all members and verified
the quorum backend doesn't really add any speed to the
reads. This can be mostly attributed to the fact that fault
tracking is not present.
In other cases, due to internal error correcting codes it's very
unlikely that a storage medium would return a corrupted sector
without producing a error.
4) and 5) are part of the design of Quorum, as I said one the goals
is to detect (and correct) silent data corruption on the fly, not to
speed up disk access or to be space efficient.
6) Almost every remote storage technology does quorums internally
Any distributed storage (ceph/rbd, gluster, sheepdog, etc..)
provide the quorum functionality internally with added benefit
that their internal working fixes problems when split of the
network occurs.
7) Tools are restricted to qemu and qemu-img
It's a "proprietary" implementation so for a rebuild you have
to use one of the two tools. AFAIK qemu-img is not really
user friendly for the less common disk backends and we don't
really provide any abstraction on top of that. This means
that there really aren't any reasonable tools to do a offline
resync. (Okay, if you know which instance is okay, you can just
copy it ...)
Right. If this is important I can propose to write a tool for QEMU to
deal with this. It's probably a good idea anyway.
This series also lacks implementation of any user/maganement
warning method that a block operation didn't have 100% votes in the
quorum voting thus it's not really possible for the users to do a
rebuild/diagnostic if something fails.
I can't say much about this series because I haven't looked into the
code in detail yet, but I'm willing to help fix the existing problems,
add the missing features and improve the code (both in libvirt and
QEMU) if there are no other major blockers.
Thanks,
Berto
12:36 a.m.
On Wed, Jan 20, 2016 at 16:47:56 +0200, Alberto Garcia wrote:
Hi Peter,
Hi Alberto,
I'm the current maintainer of Quorum in QEMU and I'd like to try to
answer some of your comments.
On Fri, Jan 08, 2016 at 06:20:04PM +0100, Peter Krempa wrote:
> So I have a few comments/observations regarding the quorum block
> driver in qemu and it's usability.
>
> At first I'd like to as you to describe your use case a bit
> more. I'm currently lacking the motivation to do anything about
> this, as the series is just partial and I don't really see any
> advantage of using the qorum driver at all and can't come up with
> any useful use case.
>
> Also a good use case is usually a good reason to drive development
> of a feature and I'm afraid that this could become abandoned without
> any real use.
The original use case for which Quorum was designed was a data center
doing redundancy with storage in multiple separate rooms shared using
NFS.
There are quite a few existing networked storage cluster solutions,
wouldn't that be a more reasonable option?
One of the issues that the customer was facing was not only problems
in the file servers themselves but -mainly- data corruption accross
the network. Quorum can correct this on the fly and is able to
Whoah. Data corruption accross network? I'm not quite sure whether I'd
use this to cover up a problem with the storage technology or network
rather than just fix the root cause. If you have 3 copies, and manage to
have a sector where all 3 differ then the quorum driver won't help. And
it will make it even harder to find any possible problems.
identify which one of the file servers is causing the problem
without
having to rebuild a whole array (like it would be the case with RAID).
Libvirt tries to stay out of doing any usage policy, so this might be
considered a feature. The series needs then polishing to add the rebuild
capability and quorum event handling so that sub-quorate failed
operations are properly reported.
I think the rebuild is actually a useful in most cases, since it ensures
that all copies are the same.
Quorum is also used for the COLO block replication functionality
currently being discussed in QEMU:
http://wiki.qemu.org/Features/BlockReplication
Oh, so it actually uses the FIFO mode of quorum which I didn't know
about. So basically the quorum driver for COLO serves as a block
duplicator so that one write is sent to the "primary disk" and second
write is sent using nbd to the arbiter rather than using a
blockdev-mirror job. Interresting approach, but COLO stuf was not really
yet considered in libvirt.
Btw, this series explicitly forbids using less than 2 as vote threshold.
> 1) No traking of integrity
> As the quorum members don't have headers, failed quorum members
> are not recorded and remembered. The user or management app then
> has to do this externally for given storage devices.
>
> 2) No internal tracking of quorum members
> Members of the quorum don't have any header marking them
> as such and thus any images may be mixed together with
> unforseen/catastrophic results. Higher level management then
> needs to take the role of remembering which images belong
> together. Reimplementing this looks like reimplementing a
> distriuted storage system to me.
That's right, Quorum does not have its own file format and was
designed to work with any driver or protocol that QEMU supports, so
I'm not sure if there's much that can be done about this.
> 3) Lack of auto-resync:
> Once the quorum get's few inconsistencies it does not
> automatically resync like the linux MD driver. With the current
> implementation the only way to resync this would be to issue a
> block-mirror (blockCopy) to /dev/null so that all blocks are
> read and rewritten to the identical copy. This also requires a
> user action.
>
> Additionally the member of the quorum is not ignored if it was
> out of sync in any previous time without being resynced allowing
> for split-brain/corruption scenarios.
Quorum can fix errors on the fly (there's the 'rewrite-corrupted' flag
for that), so in those cases no manual intervention is required.
If we want a way to auto-resync a complete image that should be
doable, I believe it's relatively simple to implement in QEMU
(depending on the semantics).
For the manual resync I also agree that it would be good to have a
simple API to do that in case the user wants to do it manually. That
can be done.
This would be beneficial to have if you don't have 'rewrite-corrupted'
enabled. In that case you want a way to enable it and then perhaps
initiate a full read so that every block gets checked.
> 4) Necessity for at least 3 copies
> Since a majority needs to win in a vote, you need at least 3
> member disks for this to be fault-tolerant.
>
> 5) Lack of speedup
> Since always all blocks are read from all members and verified
> the quorum backend doesn't really add any speed to the
> reads. This can be mostly attributed to the fact that fault
> tracking is not present.
>
> In other cases, due to internal error correcting codes it's very
> unlikely that a storage medium would return a corrupted sector
> without producing a error.
4) and 5) are part of the design of Quorum, as I said one the goals
is to detect (and correct) silent data corruption on the fly, not to
speed up disk access or to be space efficient.
I'm thinking more that it tries to cover up possible silent data
corruption. If your storage is prone to corrupt your data without
detecting it, using quorum will make the corruption less likely;
It does not fix it.
> 6) Almost every remote storage technology does quorums
internally
> Any distributed storage (ceph/rbd, gluster, sheepdog, etc..)
> provide the quorum functionality internally with added benefit
> that their internal working fixes problems when split of the
> network occurs.
>
> 7) Tools are restricted to qemu and qemu-img
> It's a "proprietary" implementation so for a rebuild you have
> to use one of the two tools. AFAIK qemu-img is not really
> user friendly for the less common disk backends and we don't
> really provide any abstraction on top of that. This means
> that there really aren't any reasonable tools to do a offline
> resync. (Okay, if you know which instance is okay, you can just
> copy it ...)
Right. If this is important I can propose to write a tool for QEMU to
deal with this. It's probably a good idea anyway.
That is not that important in the end. If you can use it with
qemu-img/qemu-nbd that should be enough basically for the same usecases
as somebody would use QCOW images for.
> This series also lacks implementation of any user/maganement
> warning method that a block operation didn't have 100% votes in the
> quorum voting thus it's not really possible for the users to do a
> rebuild/diagnostic if something fails.
I can't say much about this series because I haven't looked into the
code in detail yet, but I'm willing to help fix the existing problems,
add the missing features and improve the code (both in libvirt and
QEMU) if there are no other major blockers.
There are two things which make me skeptical about quorums and libvirt:
1) Apart from abusing quorums in fifo mode for COLO I still don't think
that they are hugely useful. (no, data corruption on NFS didn't persuade
me)
2) The implementation in this series as in current state adds a lot of
code to mintain that wouldn't much used be and is incomplete in many
aspects:
* no support for setting the FIFO or any other possible mode
* no support for the quorum failure events and reporting
* no way to control 'rewrite-corrupted'
* since we don't use node-names yet, it's not really possible to do
block jobs on quorum disks, thus they are forbidden
* since block jobs are forbidden and rewrite-corrupted can't be
* enabled, no way to do the rebuild
Peter
11:01 p.m.
On Tue, Jan 26, 2016 at 05:36:36PM +0100, Peter Krempa wrote:
Hi Alberto,
Hi and sorry for the late reply,
> I'm the current maintainer of Quorum in QEMU and I'd
like to try
> to answer some of your comments.
>
> > So I have a few comments/observations regarding the quorum block
> > driver in qemu and it's usability.
> >
> > At first I'd like to as you to describe your use case a bit
> > more. I'm currently lacking the motivation to do anything about
> > this, as the series is just partial and I don't really see any
> > advantage of using the qorum driver at all and can't come up
> > with any useful use case.
> >
> > Also a good use case is usually a good reason to drive
> > development of a feature and I'm afraid that this could become
> > abandoned without any real use.
>
> The original use case for which Quorum was designed was a data
> center doing redundancy with storage in multiple separate rooms
> shared using NFS.
There are quite a few existing networked storage cluster solutions,
wouldn't that be a more reasonable option?
I don't know all the details of the setup, but as far as I'm aware
the goal is to have redundancy and high availability and at the same
time keep each one of the servers independent from each other in case
the others crash. It's also easier to set up. I can try to get more
details if you want.
> One of the issues that the customer was facing was not only
> problems in the file servers themselves but -mainly- data
> corruption accross the network. Quorum can correct this on the fly
> and is able to
Whoah. Data corruption accross network? I'm not quite sure whether
I'd use this to cover up a problem with the storage technology or
network rather than just fix the root cause. If you have 3 copies,
and manage to have a sector where all 3 differ then the quorum
driver won't help. And it will make it even harder to find any
possible problems.
But in that case you detect that it went wrong and you get an I/O
error. The problem with silent data corruption is that it can be hard
to detect.
If there's a bit-flip across the network Quorum can detect it,
report it and correct the faulty version without needing to rebuild
everything.
> identify which one of the file servers is causing the problem
> without having to rebuild a whole array (like it would be the case
> with RAID).
Libvirt tries to stay out of doing any usage policy, so this might
be considered a feature. The series needs then polishing to add the
rebuild capability and quorum event handling so that sub-quorate
failed operations are properly reported.
I think the rebuild is actually a useful in most cases, since it
ensures that all copies are the same.
I think the ability to rebuild is in general a good feature, I will
look into it and see how to add it to QEMU first.
> Quorum is also used for the COLO block replication
functionality
> currently being discussed in QEMU:
>
> http://wiki.qemu.org/Features/BlockReplication
Oh, so it actually uses the FIFO mode of quorum which I didn't know
about. So basically the quorum driver for COLO serves as a block
duplicator so that one write is sent to the "primary disk" and
second write is sent using nbd to the arbiter rather than using a
blockdev-mirror job. Interresting approach, but COLO stuf was not
really yet considered in libvirt.
Btw, this series explicitly forbids using less than 2 as vote
threshold.
Yes, I just wanted to point out one other example of how Quorum is
being used. This current series of Quorum for libvirt is not taking
COLO into account at all, in fact it is still under review in QEMU.
> Quorum can fix errors on the fly (there's the
'rewrite-corrupted'
> flag for that), so in those cases no manual intervention is
> required.
>
> If we want a way to auto-resync a complete image that should be
> doable, I believe it's relatively simple to implement in QEMU
> (depending on the semantics).
>
> For the manual resync I also agree that it would be good to
> have a simple API to do that in case the user wants to do it
> manually. That can be done.
This would be beneficial to have if you don't have
'rewrite-corrupted' enabled. In that case you want a way to enable
it and then perhaps initiate a full read so that every block gets
checked.
Exactly. As I said earlier I think it's a good idea.
> > 4) Necessity for at least 3 copies
> > Since a majority needs to win in a vote, you need at least 3
> > member disks for this to be fault-tolerant.
> >
> > 5) Lack of speedup
> > Since always all blocks are read from all members and verified
> > the quorum backend doesn't really add any speed to the
> > reads. This can be mostly attributed to the fact that fault
> > tracking is not present.
> >
> > In other cases, due to internal error correcting codes it's very
> > unlikely that a storage medium would return a corrupted sector
> > without producing a error.
>
> 4) and 5) are part of the design of Quorum, as I said one the
> goals is to detect (and correct) silent data corruption on the
> fly, not to speed up disk access or to be space efficient.
I'm thinking more that it tries to cover up possible silent data
corruption. If your storage is prone to corrupt your data without
detecting it, using quorum will make the corruption less likely; It
does not fix it.
Well, if something is wrong with the hardware you cannot fix that,
but you can report where the error happened and still keep the system
running.
> I can't say much about this series because I haven't
looked into
> the code in detail yet, but I'm willing to help fix the existing
> problems, add the missing features and improve the code (both in
> libvirt and QEMU) if there are no other major blockers.
There are two things which make me skeptical about quorums and
libvirt:
1) Apart from abusing quorums in fifo mode for COLO I still don't
think that they are hugely useful. (no, data corruption on NFS
didn't persuade me)
It is one of the main reasons why Quorum was written. Here's one more
example of silent data corruption over the network:
https://cds.cern.ch/record/2026187/files/Adler32_Data_Corruption.pdf
2) The implementation in this series as in current state adds a lot
of code to mintain that wouldn't much used be and is incomplete in
many aspects:
As I said I didn't look deeply into this series yet, I just tested
it and had an overview of the code, but I'm willing to help make it
better if all else is fine.
* no support for setting the FIFO or any other possible mode
I would say FIFO is not strictly necessary (not for this use case at
least). It can be added later if needed, but it's probably easy enough
to add it now.
* no support for the quorum failure events and reporting
* no way to control 'rewrite-corrupted'
I can look into these.
* since we don't use node-names yet, it's not really
possible to do
block jobs on quorum disks, thus they are forbidden
I'm not sure what's the status of node names in libvirt, I could also
try to help to make it happen.
* since block jobs are forbidden and rewrite-corrupted can't
be
* enabled, no way to do the rebuild
'rewrite-corrupted' can be easily added to the series so I don't
think that's a problem. The block jobs thing I would need to see
first. Would you really need to have node names in order to rebuild a
Quorum?
Regards,
Berto
9:50 p.m.
On Wed, Feb 10, 2016 at 17:01:22 +0200, Alberto Garcia wrote:
On Tue, Jan 26, 2016 at 05:36:36PM +0100, Peter Krempa wrote:
[...]
> Whoah. Data corruption accross network? I'm not quite sure
whether
> I'd use this to cover up a problem with the storage technology or
> network rather than just fix the root cause. If you have 3 copies,
> and manage to have a sector where all 3 differ then the quorum
> driver won't help. And it will make it even harder to find any
> possible problems.
But in that case you detect that it went wrong and you get an I/O
error. The problem with silent data corruption is that it can be hard
to detect.
Yes, and that's why it should be fixed at the network storage technology
layer rather than anywhere else.
If there's a bit-flip across the network Quorum can detect it,
report it and correct the faulty version without needing to rebuild
everything.
I still think that you do wan't to rebulild the whole volume in such
case if you care about your data in the slightest. Otherwise you don't
have to do stuff like this.
[...]
> > Quorum is also used for the COLO block replication
functionality
> > currently being discussed in QEMU:
> >
> > http://wiki.qemu.org/Features/BlockReplication
[...]
Yes, I just wanted to point out one other example of how Quorum is
being used. This current series of Quorum for libvirt is not taking
COLO into account at all, in fact it is still under review in QEMU.
Yes and there are apparently some design issues/major problems. If they
are going to use this, we should probably wait on the result of that
discussion.
[...]
> 1) Apart from abusing quorums in fifo mode for COLO I still
don't
> think that they are hugely useful. (no, data corruption on NFS
> didn't persuade me)
It is one of the main reasons why Quorum was written. Here's one more
example of silent data corruption over the network:
https://cds.cern.ch/record/2026187/files/Adler32_Data_Corruption.pdf
That underlines the fact that the network storage protocol does a
terrible job in this case. Additionally in the described case the
highlighted advantage of adler32 is speed. Patching that with
triplicating the data and necessary bandwidth to transfer it does not
stack with that really well. Additionally the regular use case remains
still broken.
> 2) The implementation in this series as in current state adds a
lot
> of code to mintain that wouldn't much used be and is incomplete in
> many aspects:
[...]
> * no support for the quorum failure events and reporting
> * no way to control 'rewrite-corrupted'
I can look into these.
> * since we don't use node-names yet, it's not really possible to do
> block jobs on quorum disks, thus they are forbidden
I'm not sure what's the status of node names in libvirt, I could also
try to help to make it happen.
They are basically non-existent. To be honest I think that the node name
support stuff and better approach at constructing block devices and
their backing chains and better handling of block jobs should be done
prior to quorum.
This series tries to partially do the stuff that is a plan how to
approach some stuff regarding disks. One of them is that the backing
chain of a disk is persisted in the XML and then fully constructed.
By adding this code the refactor will be even more painful as it will
currently be.
I'm actually planing to do this in short term future, but unfortunately
this is not a weekend project.
> * since block jobs are forbidden and rewrite-corrupted can't be
> * enabled, no way to do the rebuild
'rewrite-corrupted' can be easily added to the series so I don't
think that's a problem. The block jobs thing I would need to see
first. Would you really need to have node names in order to rebuild a
Quorum?
Most probably yes. Without them, it will be just an ugly hack.
Peter
12:15 a.m.
On Thu, Feb 11, 2016 at 02:50:55PM +0100, Peter Krempa wrote:
> > Whoah. Data corruption accross network? I'm not quite
sure
> > whether I'd use this to cover up a problem with the storage
> > technology or network rather than just fix the root cause. If
> > you have 3 copies, and manage to have a sector where all 3
> > differ then the quorum driver won't help. And it will make it
> > even harder to find any possible problems.
>
> But in that case you detect that it went wrong and you get an I/O
> error. The problem with silent data corruption is that it can be
> hard to detect.
Yes, and that's why it should be fixed at the network storage
technology layer rather than anywhere else.
I've had the chance to discuss this a bit with a cloud provider that
is using Quorum.
In their experience they have had problems in their tests with Gluster
or Ceph, particularly when sharing the same images among several
clients. They have experienced major delays and crashes when one of
the nodes fail, and in general they don't consider them stable enough
for their needs. On the other hand NFS is easy to use and manipulate,
robust, and allows the use of hardware appliances.
> If there's a bit-flip across the network Quorum can detect
it,
> report it and correct the faulty version without needing to
> rebuild everything.
I still think that you do wan't to rebulild the whole volume in such
case if you care about your data in the slightest. Otherwise you
don't have to do stuff like this.
In general, yes. But that's right, I agree that having API to deal
with these scenarios is a good idea and I can work on it.
> > * since we don't use node-names yet, it's not
really
> > possible to do block jobs on quorum disks, thus they are
> > forbidden
>
> I'm not sure what's the status of node names in libvirt, I could
> also try to help to make it happen.
They are basically non-existent. To be honest I think that the node
name support stuff and better approach at constructing block devices
and their backing chains and better handling of block jobs should be
done prior to quorum.
This series tries to partially do the stuff that is a plan how to
approach some stuff regarding disks. One of them is that the backing
chain of a disk is persisted in the XML and then fully constructed.
By adding this code the refactor will be even more painful as it
will currently be.
I'm actually planing to do this in short term future, but
unfortunately this is not a weekend project.
>
> > * since block jobs are forbidden and rewrite-corrupted can't
> > * be enabled, no way to do the rebuild
>
> 'rewrite-corrupted' can be easily added to the series so I don't
> think that's a problem. The block jobs thing I would need to
> see first. Would you really need to have node names in order to
> rebuild a Quorum?
Most probably yes. Without them, it will be just an ugly hack.
For the common usage I think you can use the device name just fine,
but if you have a scenario where a Quorum is part of a backing chain
then if wouldn't work without node names.
Berto
3176
days inactive
3258
days old
27 comments
4 participants
participants (4)
-
Alberto Garcia -
John Ferlan -
Matthias Gatto -
Peter Krempa