[libvirt] [PATCH] test_driver: implement virDomainGetDiskErrors

Ilias Stamatis posted 1 patch 2 weeks ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190507202320.32233-1-stamatis.iliass@gmail.com
src/test/test_driver.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)

[libvirt] [PATCH] test_driver: implement virDomainGetDiskErrors

Posted by Ilias Stamatis 2 weeks ago
Return the number of disks present in the configuration of the fake
driver when called with @errors as NULL and @maxerrors as 0.

Otherwise return 0 as the number of errors encountered.

Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
---
 src/test/test_driver.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 460c896ef6..5fa9ab30f1 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -3046,6 +3046,32 @@ static int testDomainSetAutostart(virDomainPtr domain,
     return 0;
 }
 
+static int testDomainGetDiskErrors(virDomainPtr dom,
+                                   virDomainDiskErrorPtr errors,
+                                   unsigned int maxerrors ATTRIBUTE_UNUSED,
+                                   unsigned int flags)
+{
+    int ret = -1;
+    virDomainObjPtr vm = NULL;
+
+    virCheckFlags(0, -1);
+
+    if (!(vm = testDomObjFromDomain(dom)))
+        goto cleanup;
+
+    if (virDomainObjCheckActive(vm) < 0)
+        goto cleanup;
+
+    if (!errors)
+        ret = vm->def->ndisks;
+    else
+        ret = 0;
+
+ cleanup:
+    virDomainObjEndAPI(&vm);
+    return ret;
+}
+
 static char *testDomainGetSchedulerType(virDomainPtr domain ATTRIBUTE_UNUSED,
                                         int *nparams)
 {
@@ -6833,6 +6859,7 @@ static virHypervisorDriver testHypervisorDriver = {
     .domainUndefineFlags = testDomainUndefineFlags, /* 0.9.4 */
     .domainGetAutostart = testDomainGetAutostart, /* 0.3.2 */
     .domainSetAutostart = testDomainSetAutostart, /* 0.3.2 */
+    .domainGetDiskErrors = testDomainGetDiskErrors, /* 5.4.0 */
     .domainGetSchedulerType = testDomainGetSchedulerType, /* 0.3.2 */
     .domainGetSchedulerParameters = testDomainGetSchedulerParameters, /* 0.3.2 */
     .domainGetSchedulerParametersFlags = testDomainGetSchedulerParametersFlags, /* 0.9.2 */
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] test_driver: implement virDomainGetDiskErrors

Posted by Michal Privoznik 2 weeks ago
On 5/7/19 10:23 PM, Ilias Stamatis wrote:
> Return the number of disks present in the configuration of the fake
> driver when called with @errors as NULL and @maxerrors as 0.
> 
> Otherwise return 0 as the number of errors encountered.
> 
> Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
> ---
>   src/test/test_driver.c | 27 +++++++++++++++++++++++++++
>   1 file changed, 27 insertions(+)
> 
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 460c896ef6..5fa9ab30f1 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -3046,6 +3046,32 @@ static int testDomainSetAutostart(virDomainPtr domain,
>       return 0;
>   }
>   
> +static int testDomainGetDiskErrors(virDomainPtr dom,
> +                                   virDomainDiskErrorPtr errors,
> +                                   unsigned int maxerrors ATTRIBUTE_UNUSED,
> +                                   unsigned int flags)
> +{
> +    int ret = -1;
> +    virDomainObjPtr vm = NULL;
> +
> +    virCheckFlags(0, -1);
> +
> +    if (!(vm = testDomObjFromDomain(dom)))
> +        goto cleanup;
> +
> +    if (virDomainObjCheckActive(vm) < 0)
> +        goto cleanup;
> +
> +    if (!errors)
> +        ret = vm->def->ndisks;
> +    else
> +        ret = 0;

Don't we want to actually set some errors? That might be more helpful 
because mgmt app can actually test if it reports disk errors properly.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] test_driver: implement virDomainGetDiskErrors

Posted by Ilias Stamatis 2 weeks ago
On Thu, May 9, 2019 at 4:36 PM Michal Privoznik <mprivozn@redhat.com> wrote:
>
> On 5/7/19 10:23 PM, Ilias Stamatis wrote:
> > Return the number of disks present in the configuration of the fake
> > driver when called with @errors as NULL and @maxerrors as 0.
> >
> > Otherwise return 0 as the number of errors encountered.
> >
> > Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
> > ---
> >   src/test/test_driver.c | 27 +++++++++++++++++++++++++++
> >   1 file changed, 27 insertions(+)
> >
> > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > index 460c896ef6..5fa9ab30f1 100644
> > --- a/src/test/test_driver.c
> > +++ b/src/test/test_driver.c
> > @@ -3046,6 +3046,32 @@ static int testDomainSetAutostart(virDomainPtr domain,
> >       return 0;
> >   }
> >
> > +static int testDomainGetDiskErrors(virDomainPtr dom,
> > +                                   virDomainDiskErrorPtr errors,
> > +                                   unsigned int maxerrors ATTRIBUTE_UNUSED,
> > +                                   unsigned int flags)
> > +{
> > +    int ret = -1;
> > +    virDomainObjPtr vm = NULL;
> > +
> > +    virCheckFlags(0, -1);
> > +
> > +    if (!(vm = testDomObjFromDomain(dom)))
> > +        goto cleanup;
> > +
> > +    if (virDomainObjCheckActive(vm) < 0)
> > +        goto cleanup;
> > +
> > +    if (!errors)
> > +        ret = vm->def->ndisks;
> > +    else
> > +        ret = 0;
>
> Don't we want to actually set some errors? That might be more helpful
> because mgmt app can actually test if it reports disk errors properly.
>
> Michal

Right, that makes sense.

The number of disks can actually vary because the test connection can
be configured differently, plus I'll implement virDomainAttachDevice
for the test driver too.

The possible error codes are just 2: VIR_DOMAIN_DISK_ERROR_UNSPEC and
VIR_DOMAIN_DISK_ERROR_NO_SPACE

So, should it be randomly decided at runtime whether or not to report
an error for each disk present and which error code will it actually
be?

Or would it be better to take a deterministic approach and report
let's say an error for the first 2 disks (if there are that many at
all), each with a different error code, and return no errors for all
the remaining disks?

Thanks,
Ilias

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] test_driver: implement virDomainGetDiskErrors

Posted by Michal Privoznik 2 weeks ago
On 5/9/19 7:35 PM, Ilias Stamatis wrote:
> On Thu, May 9, 2019 at 4:36 PM Michal Privoznik <mprivozn@redhat.com> wrote:
>>
>> On 5/7/19 10:23 PM, Ilias Stamatis wrote:
>>> Return the number of disks present in the configuration of the fake
>>> driver when called with @errors as NULL and @maxerrors as 0.
>>>
>>> Otherwise return 0 as the number of errors encountered.
>>>
>>> Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
>>> ---
>>>    src/test/test_driver.c | 27 +++++++++++++++++++++++++++
>>>    1 file changed, 27 insertions(+)
>>>
>>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
>>> index 460c896ef6..5fa9ab30f1 100644
>>> --- a/src/test/test_driver.c
>>> +++ b/src/test/test_driver.c
>>> @@ -3046,6 +3046,32 @@ static int testDomainSetAutostart(virDomainPtr domain,
>>>        return 0;
>>>    }
>>>
>>> +static int testDomainGetDiskErrors(virDomainPtr dom,
>>> +                                   virDomainDiskErrorPtr errors,
>>> +                                   unsigned int maxerrors ATTRIBUTE_UNUSED,
>>> +                                   unsigned int flags)
>>> +{
>>> +    int ret = -1;
>>> +    virDomainObjPtr vm = NULL;
>>> +
>>> +    virCheckFlags(0, -1);
>>> +
>>> +    if (!(vm = testDomObjFromDomain(dom)))
>>> +        goto cleanup;
>>> +
>>> +    if (virDomainObjCheckActive(vm) < 0)
>>> +        goto cleanup;
>>> +
>>> +    if (!errors)
>>> +        ret = vm->def->ndisks;
>>> +    else
>>> +        ret = 0;
>>
>> Don't we want to actually set some errors? That might be more helpful
>> because mgmt app can actually test if it reports disk errors properly.
>>
>> Michal
> 
> Right, that makes sense.
> 
> The number of disks can actually vary because the test connection can
> be configured differently, plus I'll implement virDomainAttachDevice
> for the test driver too.
> 
> The possible error codes are just 2: VIR_DOMAIN_DISK_ERROR_UNSPEC and
> VIR_DOMAIN_DISK_ERROR_NO_SPACE
> 
> So, should it be randomly decided at runtime whether or not to report
> an error for each disk present and which error code will it actually
> be?
> 
> Or would it be better to take a deterministic approach and report
> let's say an error for the first 2 disks (if there are that many at
> all), each with a different error code, and return no errors for all
> the remaining disks?

Yeah, I think the latter makes sense. For simplicity, we can have two 
loops, one inside the other and set the first disk to have no error, the 
second to have VIR_DOMAIN_DISK_ERROR_UNSPEC, the third to have 
VIR_DOMAIN_DISK_ERROR_NO_SPACE, the fourth to have no error again and so 
on. This is perfectly predictible and thus easy to test and it gets a 
result that 'looks' believable.

But whatever you'll come up with will be fine really.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list