On Fri, Aug 2, 2019 at 4:39 PM Erik Skultety <eskultet@xxxxxxxxxx> 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@xxxxxxxxx> > > --- > > 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list