[libvirt] [PATCH 2/4] test_driver: implement virDomainFSFreeze

[Ilias Stamatis]

On Fri, Aug 2, 2019 at 4:39 PM Erik Skultety <eskultet at 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 at 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