2:23 p.m.
Ilias Stamatis (4):
test_driver: introduce domain-private data
test_driver: implement virDomainFSFreeze
test_driver: implement virDomainFSThaw
test_driver: implement virDomainFSTrim
src/test/test_driver.c | 191 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 190 insertions(+), 1 deletion(-)
--
2.22.0
2:23 p.m.
New subject: [libvirt] [PATCH 1/4] test_driver: introduce domain-private data
---
src/test/test_driver.c | 35 ++++++++++++++++++++++++++++++++++-
1 file changed, 34 insertions(+), 1 deletion(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 49d7030d21..af3503c523 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -384,6 +384,35 @@ testBuildCapabilities(virConnectPtr conn)
}
+typedef struct _testDomainObjPrivate testDomainObjPrivate;
+typedef testDomainObjPrivate *testDomainObjPrivatePtr;
+struct _testDomainObjPrivate {
+ testDriverPtr driver;
+};
+
+
+static void *
+testDomainObjPrivateAlloc(void *opaque)
+{
+ testDomainObjPrivatePtr priv;
+
+ if (VIR_ALLOC(priv) < 0)
+ return NULL;
+
+ priv->driver = opaque;
+
+ return priv;
+}
+
+
+static void
+testDomainObjPrivateFree(void *data)
+{
+ testDomainObjPrivatePtr priv = data;
+ VIR_FREE(priv);
+}
+
+
static testDriverPtr
testDriverNew(void)
{
@@ -399,6 +428,10 @@ testDriverNew(void)
VIR_DOMAIN_DEF_FEATURE_FW_AUTOSELECT |
VIR_DOMAIN_DEF_FEATURE_NET_MODEL_STRING,
};
+ virDomainXMLPrivateDataCallbacks privatecb = {
+ .alloc = testDomainObjPrivateAlloc,
+ .free = testDomainObjPrivateFree,
+ };
testDriverPtr ret;
if (testDriverInitialize() < 0)
@@ -407,7 +440,7 @@ testDriverNew(void)
if (!(ret = virObjectLockableNew(testDriverClass)))
return NULL;
- if (!(ret->xmlopt = virDomainXMLOptionNew(&config, NULL, &ns, NULL, NULL))
||
+ if (!(ret->xmlopt = virDomainXMLOptionNew(&config, &privatecb, &ns,
NULL, NULL)) ||
!(ret->eventState = virObjectEventStateNew()) ||
!(ret->ifaces = virInterfaceObjListNew()) ||
!(ret->domains = virDomainObjListNew()) ||
--
2.22.0
6:23 a.m.
New subject: [libvirt] [PATCH 1/4] test_driver: introduce domain-private data
On Tue, Jul 09, 2019 at 09:23:21PM +0200, Ilias Stamatis wrote:
---
Explaining that the private data will come handy to hold the information
whether filesystems have been frozen would be nice.
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
7:51 a.m.
New subject: [libvirt] [PATCH 1/4] test_driver: introduce domain-private data
On Fri, Aug 2, 2019 at 1:23 PM Erik Skultety <eskultet(a)redhat.com> wrote:
On Tue, Jul 09, 2019 at 09:23:21PM +0200, Ilias Stamatis wrote:
> ---
Explaining that the private data will come handy to hold the information
whether filesystems have been frozen would be nice.
Sure, just maybe let's not make it FS-specific only since we'll use
that for other APIs too and there's nothing related to the FS APIs in
this patch anyways.
How about we add in the commit message something like:
"""
vm-specific data can be used by APIs that need to preserve some state
between calls
Some of them are:
- FS-related APIs for remembering which mountpoints are frozen
- virDomainSetTime / virDomainGetTime for maintaining time information
- virDomainSetIOThreadParams for storing the I/O thread parameters
- virDomainManagedSaveDefineXML for internally storing the VM definition
"""
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
2:23 p.m.
New subject: [libvirt] [PATCH 2/4] test_driver: implement virDomainFSFreeze
On success update the domain-private data. Consider / and /boot to be
the only mountpoints avaiable in order to be consistent with the other
FS-related calls.
Signed-off-by: Ilias Stamatis <stamatis.iliass(a)gmail.com>
---
src/test/test_driver.c | 58 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index af3503c523..8c25c679a5 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -388,6 +388,8 @@ typedef struct _testDomainObjPrivate testDomainObjPrivate;
typedef testDomainObjPrivate *testDomainObjPrivatePtr;
struct _testDomainObjPrivate {
testDriverPtr driver;
+
+ bool frozen[2]; /* used by file system related calls */
};
@@ -400,6 +402,7 @@ testDomainObjPrivateAlloc(void *opaque)
return NULL;
priv->driver = opaque;
+ priv->frozen[0] = priv->frozen[1] = false;
return priv;
}
@@ -3439,6 +3442,60 @@ static int testDomainUndefine(virDomainPtr domain)
return testDomainUndefineFlags(domain, 0);
}
+
+static int
+testDomainFSFreeze(virDomainPtr dom,
+ const char **mountpoints,
+ unsigned int nmountpoints,
+ unsigned int flags)
+{
+ virDomainObjPtr vm;
+ testDomainObjPrivatePtr priv;
+ size_t i;
+ int slash = 0, slash_boot = 0;
+ int ret = -1;
+
+ virCheckFlags(0, -1);
+
+ if (!(vm = testDomObjFromDomain(dom)))
+ goto cleanup;
+
+ if (virDomainObjCheckActive(vm) < 0)
+ goto cleanup;
+
+ priv = vm->privateData;
+
+ if (nmountpoints == 0) {
+ priv->frozen[0] = priv->frozen[1] = true;
+ ret = 2;
+ } else {
+ for (i = 0; i < nmountpoints; i++) {
+ if (STREQ(mountpoints[i], "/")) {
+ slash = 1;
+ } else if (STREQ(mountpoints[i], "/boot")) {
+ slash_boot = 1;
+ } else {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ _("mount point not found: %s"),
+ mountpoints[i]);
+ goto cleanup;
+ }
+ }
+
+ if (slash)
+ priv->frozen[0] = true;
+ if (slash_boot)
+ priv->frozen[1] = true;
+
+ ret = slash + slash_boot;
+ }
+
+ cleanup:
+ virDomainObjEndAPI(&vm);
+ return ret;
+}
+
+
static int testDomainGetAutostart(virDomainPtr domain,
int *autostart)
{
@@ -7681,6 +7738,7 @@ static virHypervisorDriver testHypervisorDriver = {
.domainDefineXMLFlags = testDomainDefineXMLFlags, /* 1.2.12 */
.domainUndefine = testDomainUndefine, /* 0.1.11 */
.domainUndefineFlags = testDomainUndefineFlags, /* 0.9.4 */
+ .domainFSFreeze = testDomainFSFreeze, /* 5.6.0 */
.domainGetAutostart = testDomainGetAutostart, /* 0.3.2 */
.domainSetAutostart = testDomainSetAutostart, /* 0.3.2 */
.domainGetDiskErrors = testDomainGetDiskErrors, /* 5.4.0 */
--
2.22.0
9:39 a.m.
New subject: [libvirt] [PATCH 2/4] test_driver: implement virDomainFSFreeze
On Tue, Jul 09, 2019 at 09:23:22PM +0200, Ilias Stamatis wrote:
On success update the domain-private data. Consider / and /boot to
be
the only mountpoints avaiable in order to be consistent with the other
FS-related calls.
Signed-off-by: Ilias Stamatis <stamatis.iliass(a)gmail.com>
---
src/test/test_driver.c | 58 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index af3503c523..8c25c679a5 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -388,6 +388,8 @@ typedef struct _testDomainObjPrivate testDomainObjPrivate;
typedef testDomainObjPrivate *testDomainObjPrivatePtr;
struct _testDomainObjPrivate {
testDriverPtr driver;
+
+ bool frozen[2]; /* used by file system related calls */
};
@@ -400,6 +402,7 @@ testDomainObjPrivateAlloc(void *opaque)
return NULL;
priv->driver = opaque;
+ priv->frozen[0] = priv->frozen[1] = false;
return priv;
}
@@ -3439,6 +3442,60 @@ static int testDomainUndefine(virDomainPtr domain)
return testDomainUndefineFlags(domain, 0);
}
+
+static int
+testDomainFSFreeze(virDomainPtr dom,
+ const char **mountpoints,
+ unsigned int nmountpoints,
+ unsigned int flags)
+{
+ virDomainObjPtr vm;
+ testDomainObjPrivatePtr priv;
+ size_t i;
+ int slash = 0, slash_boot = 0;
One declaration per line please. Also, we should define these explicitly as
booleans, the standard states these would return 0 and 1 respectively.
+ int ret = -1;
+
+ virCheckFlags(0, -1);
+
+ if (!(vm = testDomObjFromDomain(dom)))
+ goto cleanup;
+
+ if (virDomainObjCheckActive(vm) < 0)
+ goto cleanup;
+
+ priv = vm->privateData;
+
+ if (nmountpoints == 0) {
+ priv->frozen[0] = priv->frozen[1] = true;
+ ret = 2;
Well, this is not always true, e.g. if '/' was frozen before, ret should be 1.
Similarly, if both were frozen prior to calling this API, we should return 0.
+ } else {
+ for (i = 0; i < nmountpoints; i++) {
+ if (STREQ(mountpoints[i], "/")) {
+ slash = 1;
+ } else if (STREQ(mountpoints[i], "/boot")) {
+ slash_boot = 1;
^here too, if the filesystems were already frozen, we should not account for
them in @ret.
+ } else {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ _("mount point not found: %s"),
+ mountpoints[i]);
+ goto cleanup;
+ }
+ }
+
+ if (slash)
+ priv->frozen[0] = true;
+ if (slash_boot)
+ priv->frozen[1] = true;
+
+ ret = slash + slash_boot;
+ }
We could do ^this or alternatively, we could introduce another iterator
@nfreeze, use it within the loop. We could also declare:
bool freeze[2];
//backup the original values:
memcpy(&freeze, priv->frozen, 2);
for (mountpoints) {
//set the helper array
if (mount[i] == / && !freeze[i]) {
freeze[i] = true;
nfreeze++;
}
else if (mount[i] == /boot && !freeze[i]) {
freeze[i] = true;
nfreeze++;
}
}
ret = nfreeze;
//steal the helper copy
memcpy(priv->frozen, &freeze, 2);
return ret;
Erik
7:46 a.m.
New subject: [libvirt] [PATCH 2/4] test_driver: implement virDomainFSFreeze
On Fri, Aug 2, 2019 at 4:39 PM Erik Skultety <eskultet(a)redhat.com> wrote:
On Tue, Jul 09, 2019 at 09:23:22PM +0200, Ilias Stamatis wrote:
> On success update the domain-private data. Consider / and /boot to be
> the only mountpoints avaiable in order to be consistent with the other
> FS-related calls.
>
> Signed-off-by: Ilias Stamatis <stamatis.iliass(a)gmail.com>
> ---
> src/test/test_driver.c | 58 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 58 insertions(+)
>
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index af3503c523..8c25c679a5 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -388,6 +388,8 @@ typedef struct _testDomainObjPrivate testDomainObjPrivate;
> typedef testDomainObjPrivate *testDomainObjPrivatePtr;
> struct _testDomainObjPrivate {
> testDriverPtr driver;
> +
> + bool frozen[2]; /* used by file system related calls */
> };
>
>
> @@ -400,6 +402,7 @@ testDomainObjPrivateAlloc(void *opaque)
> return NULL;
>
> priv->driver = opaque;
> + priv->frozen[0] = priv->frozen[1] = false;
>
> return priv;
> }
> @@ -3439,6 +3442,60 @@ static int testDomainUndefine(virDomainPtr domain)
> return testDomainUndefineFlags(domain, 0);
> }
>
> +
> +static int
> +testDomainFSFreeze(virDomainPtr dom,
> + const char **mountpoints,
> + unsigned int nmountpoints,
> + unsigned int flags)
> +{
> + virDomainObjPtr vm;
> + testDomainObjPrivatePtr priv;
> + size_t i;
> + int slash = 0, slash_boot = 0;
One declaration per line please. Also, we should define these explicitly as
booleans, the standard states these would return 0 and 1 respectively.
I have seen in many places in the codebase many variables declared in
the same line so that's what I thought it was okay when the variables
are somehow coupled or related. But okay, apparently the single
declaration per line rule has been added later.
> + int ret = -1;
> +
> + virCheckFlags(0, -1);
> +
> + if (!(vm = testDomObjFromDomain(dom)))
> + goto cleanup;
> +
> + if (virDomainObjCheckActive(vm) < 0)
> + goto cleanup;
> +
> + priv = vm->privateData;
> +
> + if (nmountpoints == 0) {
> + priv->frozen[0] = priv->frozen[1] = true;
> + ret = 2;
Well, this is not always true, e.g. if '/' was frozen before, ret should be 1.
Similarly, if both were frozen prior to calling this API, we should return 0.
Right. I don't remember why but I thought in the past that it made
sense to report that we froze a partition even if it was already
frozen from before. Maybe for simplicity? I can't recall my original
thinking right now.
But thinking about it again, sure, let's make it freeze only what's
not frozen already. I'm sending a new patch with implementing your
feedback.
Thanks!
Ilias
> + } else {
> + for (i = 0; i < nmountpoints; i++) {
> + if (STREQ(mountpoints[i], "/")) {
> + slash = 1;
> + } else if (STREQ(mountpoints[i], "/boot")) {
> + slash_boot = 1;
^here too, if the filesystems were already frozen, we should not account for
them in @ret.
> + } else {
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + _("mount point not found: %s"),
> + mountpoints[i]);
> + goto cleanup;
> + }
> + }
> +
> + if (slash)
> + priv->frozen[0] = true;
> + if (slash_boot)
> + priv->frozen[1] = true;
> +
> + ret = slash + slash_boot;
> + }
We could do ^this or alternatively, we could introduce another iterator
@nfreeze, use it within the loop. We could also declare:
bool freeze[2];
//backup the original values:
memcpy(&freeze, priv->frozen, 2);
for (mountpoints) {
//set the helper array
if (mount[i] == / && !freeze[i]) {
freeze[i] = true;
nfreeze++;
}
else if (mount[i] == /boot && !freeze[i]) {
freeze[i] = true;
nfreeze++;
}
}
ret = nfreeze;
//steal the helper copy
memcpy(priv->frozen, &freeze, 2);
return ret;
Erik
2:23 p.m.
New subject: [libvirt] [PATCH 3/4] test_driver: implement virDomainFSThaw
Signed-off-by: Ilias Stamatis <stamatis.iliass(a)gmail.com>
---
src/test/test_driver.c | 66 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 66 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 8c25c679a5..097720bb0a 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -3496,6 +3496,71 @@ testDomainFSFreeze(virDomainPtr dom,
}
+static int
+testDomainFSThaw(virDomainPtr dom,
+ const char **mountpoints,
+ unsigned int nmountpoints,
+ unsigned int flags)
+{
+ virDomainObjPtr vm;
+ testDomainObjPrivatePtr priv;
+ size_t i;
+ int slash = 0, slash_boot = 0;
+ int ret = -1;
+
+ virCheckFlags(0, -1);
+
+ if (!(vm = testDomObjFromDomain(dom)))
+ goto cleanup;
+
+ if (virDomainObjCheckActive(vm) < 0)
+ goto cleanup;
+
+ priv = vm->privateData;
+
+ if (nmountpoints == 0) {
+ ret = priv->frozen[0] + priv->frozen[1];
+ priv->frozen[0] = priv->frozen[1] = false;
+ } else {
+ for (i = 0; i < nmountpoints; i++) {
+ if (STREQ(mountpoints[i], "/")) {
+ if (priv->frozen[0]) {
+ slash = 1;
+ } else {
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("mount point is not frozen: /"));
+ goto cleanup;
+ }
+ } else if (STREQ(mountpoints[i], "/boot")) {
+ if (priv->frozen[1]) {
+ slash_boot = 1;
+ } else {
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("mount point is not frozen: /boot"));
+ goto cleanup;
+ }
+ } else {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ _("mount point not found: %s"),
+ mountpoints[i]);
+ goto cleanup;
+ }
+ }
+
+ if (slash)
+ priv->frozen[0] = false;
+ if (slash_boot)
+ priv->frozen[1] = false;
+
+ ret = slash + slash_boot;
+ }
+
+ cleanup:
+ virDomainObjEndAPI(&vm);
+ return ret;
+}
+
+
static int testDomainGetAutostart(virDomainPtr domain,
int *autostart)
{
@@ -7739,6 +7804,7 @@ static virHypervisorDriver testHypervisorDriver = {
.domainUndefine = testDomainUndefine, /* 0.1.11 */
.domainUndefineFlags = testDomainUndefineFlags, /* 0.9.4 */
.domainFSFreeze = testDomainFSFreeze, /* 5.6.0 */
+ .domainFSThaw = testDomainFSThaw, /* 5.6.0 */
.domainGetAutostart = testDomainGetAutostart, /* 0.3.2 */
.domainSetAutostart = testDomainSetAutostart, /* 0.3.2 */
.domainGetDiskErrors = testDomainGetDiskErrors, /* 5.4.0 */
--
2.22.0
9:43 a.m.
New subject: [libvirt] [PATCH 3/4] test_driver: implement virDomainFSThaw
On Tue, Jul 09, 2019 at 09:23:23PM +0200, Ilias Stamatis wrote:
Signed-off-by: Ilias Stamatis <stamatis.iliass(a)gmail.com>
---
src/test/test_driver.c | 66 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 66 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 8c25c679a5..097720bb0a 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -3496,6 +3496,71 @@ testDomainFSFreeze(virDomainPtr dom,
}
+static int
+testDomainFSThaw(virDomainPtr dom,
+ const char **mountpoints,
+ unsigned int nmountpoints,
+ unsigned int flags)
+{
+ virDomainObjPtr vm;
+ testDomainObjPrivatePtr priv;
+ size_t i;
+ int slash = 0, slash_boot = 0;
One declaration per line...
+ int ret = -1;
+
+ virCheckFlags(0, -1);
+
+ if (!(vm = testDomObjFromDomain(dom)))
+ goto cleanup;
+
+ if (virDomainObjCheckActive(vm) < 0)
+ goto cleanup;
+
+ priv = vm->privateData;
+
+ if (nmountpoints == 0) {
+ ret = priv->frozen[0] + priv->frozen[1];
+ priv->frozen[0] = priv->frozen[1] = false;
+ } else {
+ for (i = 0; i < nmountpoints; i++) {
+ if (STREQ(mountpoints[i], "/")) {
+ if (priv->frozen[0]) {
+ slash = 1;
+ } else {
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("mount point is not frozen: /"));
+ goto cleanup;
+ }
I don't think ^this is an error per se, instead we can silently ignore the
mountpoint if it's already been thawed prior to calling this API.
+ } else if (STREQ(mountpoints[i], "/boot")) {
+ if (priv->frozen[1]) {
+ slash_boot = 1;
+ } else {
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("mount point is not frozen: /boot"));
+ goto cleanup;
+ }
Same here...
+ } else {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ _("mount point not found: %s"),
+ mountpoints[i]);
+ goto cleanup;
+ }
+ }
+
+ if (slash)
+ priv->frozen[0] = false;
+ if (slash_boot)
+ priv->frozen[1] = false;
+
+ ret = slash + slash_boot;
+ }
Consider the alternative implementation I hinted in comments to 2/4
Erik
2:23 p.m.
New subject: [libvirt] [PATCH 4/4] test_driver: implement virDomainFSTrim
Signed-off-by: Ilias Stamatis <stamatis.iliass(a)gmail.com>
---
src/test/test_driver.c | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 097720bb0a..b491247e49 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -3561,6 +3561,37 @@ testDomainFSThaw(virDomainPtr dom,
}
+static int
+testDomainFSTrim(virDomainPtr dom,
+ const char *mountPoint,
+ unsigned long long minimum ATTRIBUTE_UNUSED,
+ unsigned int flags)
+{
+ virDomainObjPtr vm;
+ int ret = -1;
+
+ virCheckFlags(0, -1);
+
+ if (!(vm = testDomObjFromDomain(dom)))
+ return -1;
+
+ if (virDomainObjCheckActive(vm) < 0)
+ goto cleanup;
+
+ if (mountPoint && STRNEQ(mountPoint, "/") &&
STRNEQ(mountPoint, "/boot")) {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ _("mount point not found: %s"),
+ mountPoint);
+ goto cleanup;
+ }
+
+ ret = 0;
+ cleanup:
+ virDomainObjEndAPI(&vm);
+ return ret;
+}
+
+
static int testDomainGetAutostart(virDomainPtr domain,
int *autostart)
{
@@ -7805,6 +7836,7 @@ static virHypervisorDriver testHypervisorDriver = {
.domainUndefineFlags = testDomainUndefineFlags, /* 0.9.4 */
.domainFSFreeze = testDomainFSFreeze, /* 5.6.0 */
.domainFSThaw = testDomainFSThaw, /* 5.6.0 */
+ .domainFSTrim = testDomainFSTrim, /* 5.6.0 */
.domainGetAutostart = testDomainGetAutostart, /* 0.3.2 */
.domainSetAutostart = testDomainSetAutostart, /* 0.3.2 */
.domainGetDiskErrors = testDomainGetDiskErrors, /* 5.4.0 */
--
2.22.0
1939
days inactive
1965
days old
10 comments
2 participants
participants (2)
-
Erik Skultety -
Ilias Stamatis