[libvirt] [PATCH 0/4] test_driver: implement FS-related APIs
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
--- 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
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@redhat.com>
On Fri, Aug 2, 2019 at 1:23 PM Erik Skultety <eskultet@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@redhat.com>
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@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
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@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
On Fri, Aug 2, 2019 at 4:39 PM Erik Skultety <eskultet@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@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
Signed-off-by: Ilias Stamatis <stamatis.iliass@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
On Tue, Jul 09, 2019 at 09:23:23PM +0200, Ilias Stamatis wrote:
Signed-off-by: Ilias Stamatis <stamatis.iliass@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
Signed-off-by: Ilias Stamatis <stamatis.iliass@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
participants (2)
-
Erik Skultety -
Ilias Stamatis