7:56 a.m.
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(a)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(a)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(a)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(a)redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list