[libvirt] [PATCH 2/2] test_driver: implement virDomainGetFSInfo

Erik Skultety eskultet at redhat.com
Tue Jul 2 12:56:16 UTC 2019


On Tue, Jul 02, 2019 at 02:49:02PM +0200, Ilias Stamatis wrote:
> On Tue, Jul 2, 2019 at 2:41 PM Erik Skultety <eskultet at redhat.com> wrote:
> >
> > On Tue, Jun 25, 2019 at 11:58:07PM +0200, Ilias Stamatis wrote:
> > > Always return / and /boot as the mount points imitating a default
> > > Fedora installation. Use the first disk found, otherwise if no disk
> > > device of type VIR_DOMAIN_DISK_DEVICE_DISK is present, return 0 mount
> > > points.
> > >
> > > Signed-off-by: Ilias Stamatis <stamatis.iliass at 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 1b1ff3003e..626449501f 100755
> > > --- a/src/test/test_driver.c
> > > +++ b/src/test/test_driver.c
> > > @@ -3298,6 +3298,71 @@ static int testDomainGetDiskErrors(virDomainPtr dom,
> > >      return ret;
> > >  }
> > >
> > > +
> > > +
> > > +static int
> > > +testDomainGetFSInfo(virDomainPtr dom,
> > > +                    virDomainFSInfoPtr **info,
> > > +                    unsigned int flags)
> > > +{
> > > +    size_t i;
> > > +    char *name;
> > > +    virDomainObjPtr vm;
> > > +    virDomainFSInfoPtr *info_ret = NULL;
> > > +    int ret = -1;
> > > +
> > > +    virCheckFlags(0, -1);
> > > +
> > > +    if (!(vm = testDomObjFromDomain(dom)))
> > > +        return -1;
> > > +
> > > +    if (virDomainObjCheckActive(vm) < 0)
> > > +        goto cleanup;
> > > +
> > > +    *info = NULL;
> > > +
> > > +    for (i = 0; i < vm->def->ndisks; i++) {
> > > +        if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
> > > +            if (VIR_ALLOC_N(info_ret, 2) < 0 || VIR_ALLOC(info_ret[0]) < 0 ||
> > > +                VIR_ALLOC(info_ret[1]) < 0 || VIR_ALLOC(info_ret[0]->devAlias) < 0 ||
> > > +                VIR_ALLOC(info_ret[1]->devAlias) < 0)
> >
> > ^these should be on separate lines to enhance readability.
>
> Sure. I'm not sure if there are certain guidelines for that, but I
> think nothing is mentioned in the "Contributor guidelines".

This is solely because of readability, we should have something like that in
the guidelines.

>
> Many times I'm unsure how to split long lines, especially on if statements.
>
> >
> > > +                goto cleanup;
> > > +
> > > +            name = vm->def->disks[i]->dst;
> > > +
> > > +            if (VIR_STRDUP(info_ret[0]->mountpoint, "/") < 0 ||
> > > +                VIR_STRDUP(info_ret[1]->mountpoint, "/boot") < 0 ||
> > > +                VIR_STRDUP(info_ret[0]->fstype, "ext4") < 0 ||
> > > +                VIR_STRDUP(info_ret[1]->fstype, "ext4") < 0 ||
> > > +                VIR_STRDUP(info_ret[0]->devAlias[0], name) < 0 ||
> > > +                VIR_STRDUP(info_ret[1]->devAlias[0], name) < 0 ||
> > > +                virAsprintf(&info_ret[0]->name, "%s1", name) < 0 ||
> > > +                virAsprintf(&info_ret[1]->name, "%s2", name) < 0)
> > > +                goto cleanup;
> >
> > Okay, alternatively we could have 2 'if' clauses for each mountpoint, this is
> > strictly a personal preference, I think it could read even better if the
> > mountpoints were logically separated, but I won't insist on that.
>
> That makes sense and is perfectly fine with me.
>
> >
> > Let me know whether you're okay with the proposed tweaks.
>
> Will you edit these before merging or do you want me to re-send it?

No need, I'll do that before merging.

Erik

>
> >
> > Reviewed-by: Erik Skultety <eskultet at redhat.com>
> >
> > > +
> > > +            info_ret[0]->ndevAlias = info_ret[1]->ndevAlias = 1;
> > > +
> > > +            VIR_STEAL_PTR(*info, info_ret);
> > > +
> > > +            ret = 2;
> > > +            goto cleanup;
> > > +        }
> > > +    }
> > > +
> > > +    ret = 0;
> > > +
> > > + cleanup:
> > > +    if (info_ret) {
> > > +        virDomainFSInfoFree(info_ret[0]);
> > > +        virDomainFSInfoFree(info_ret[1]);
> > > +        VIR_FREE(info_ret);
> > > +    }
> > > +
> > > +    virDomainObjEndAPI(&vm);
> > > +    return ret;
> > > +}
> > > +
> > > +
> > >  static char *testDomainGetSchedulerType(virDomainPtr domain ATTRIBUTE_UNUSED,
> > >                                          int *nparams)
> > >  {
> > > @@ -7292,6 +7357,7 @@ static virHypervisorDriver testHypervisorDriver = {
> > >      .domainGetAutostart = testDomainGetAutostart, /* 0.3.2 */
> > >      .domainSetAutostart = testDomainSetAutostart, /* 0.3.2 */
> > >      .domainGetDiskErrors = testDomainGetDiskErrors, /* 5.4.0 */
> > > +    .domainGetFSInfo = testDomainGetFSInfo, /* 5.5.0 */
> > >      .domainGetSchedulerType = testDomainGetSchedulerType, /* 0.3.2 */
> > >      .domainGetSchedulerParameters = testDomainGetSchedulerParameters, /* 0.3.2 */
> > >      .domainGetSchedulerParametersFlags = testDomainGetSchedulerParametersFlags, /* 0.9.2 */
> > > --
> > > 2.22.0
> > >
> > > --
> > > libvir-list mailing list
> > > libvir-list at redhat.com
> > > https://www.redhat.com/mailman/listinfo/libvir-list
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list




More information about the libvir-list mailing list