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

[Erik Skultety]

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.

> +    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