1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH v2] test_driver: implement virDomainGetDiskErrors

Ilias Stamatis posted 1 patch 5 years, 6 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190512232613.30322-1-stamatis.iliass@gmail.com
src/test/test_driver.c | 42 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)
[libvirt] [PATCH v2] test_driver: implement virDomainGetDiskErrors
Posted by Ilias Stamatis 5 years, 6 months ago 
Return the number of disks present in the configuration of the test
domain when called with @errors as NULL and @maxerrors as 0.

Otherwise report an error for every second disk, assigning available
error codes in a cyclic order.

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

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index a06d1fc402..527c2f5d3b 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -3046,6 +3046,47 @@ static int testDomainSetAutostart(virDomainPtr domain,
     return 0;
 }

+static int testDomainGetDiskErrors(virDomainPtr dom,
+                                   virDomainDiskErrorPtr errors,
+                                   unsigned int maxerrors,
+                                   unsigned int flags)
+{
+    virDomainObjPtr vm = NULL;
+    int ret = -1;
+    size_t i;
+    int n = 0;
+    int codes[] = {VIR_DOMAIN_DISK_ERROR_UNSPEC, VIR_DOMAIN_DISK_ERROR_NO_SPACE};
+    size_t ncodes = sizeof(codes) / sizeof(codes[0]);
+
+    virCheckFlags(0, -1);
+
+    if (!(vm = testDomObjFromDomain(dom)))
+        goto cleanup;
+
+    if (virDomainObjCheckActive(vm) < 0)
+        goto cleanup;
+
+    if (!errors) {
+        ret = vm->def->ndisks;
+    } else {
+        for (i = 1; i < vm->def->ndisks && n < maxerrors; i += 2) {
+            if (VIR_STRDUP(errors[n].disk, vm->def->disks[i]->dst) < 0)
+                goto cleanup;
+            errors[n].error = codes[n % ncodes];
+            n++;
+        }
+        ret = n;
+    }
+
+ cleanup:
+    virDomainObjEndAPI(&vm);
+    if (ret < 0) {
+        for (i = 0; i < n; i++)
+            VIR_FREE(errors[i].disk);
+    }
+    return ret;
+}
+
 static char *testDomainGetSchedulerType(virDomainPtr domain ATTRIBUTE_UNUSED,
                                         int *nparams)
 {
@@ -6832,6 +6873,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 v2] test_driver: implement virDomainGetDiskErrors
Posted by Michal Privoznik 5 years, 6 months ago 
On 5/13/19 1:26 AM, Ilias Stamatis wrote:
> Return the number of disks present in the configuration of the test
> domain when called with @errors as NULL and @maxerrors as 0.
> 
> Otherwise report an error for every second disk, assigning available
> error codes in a cyclic order.
> 
> Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
> ---
>   src/test/test_driver.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 42 insertions(+)
> 
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index a06d1fc402..527c2f5d3b 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -3046,6 +3046,47 @@ static int testDomainSetAutostart(virDomainPtr domain,
>       return 0;
>   }
> 
> +static int testDomainGetDiskErrors(virDomainPtr dom,
> +                                   virDomainDiskErrorPtr errors,
> +                                   unsigned int maxerrors,
> +                                   unsigned int flags)
> +{
> +    virDomainObjPtr vm = NULL;
> +    int ret = -1;
> +    size_t i;
> +    int n = 0;
> +    int codes[] = {VIR_DOMAIN_DISK_ERROR_UNSPEC, VIR_DOMAIN_DISK_ERROR_NO_SPACE};
> +    size_t ncodes = sizeof(codes) / sizeof(codes[0]);

We have ARRAY_CARDINALITY() for this.
Also, there's no need to create @codes array as we'll see later.

> +
> +    virCheckFlags(0, -1);
> +
> +    if (!(vm = testDomObjFromDomain(dom)))
> +        goto cleanup;
> +
> +    if (virDomainObjCheckActive(vm) < 0)
> +        goto cleanup;
> +
> +    if (!errors) {
> +        ret = vm->def->ndisks;

Tiny nitpick, we tend to put the longer body first, which in this case 
is the else branch.

> +    } else {
> +        for (i = 1; i < vm->def->ndisks && n < maxerrors; i += 2) {
> +            if (VIR_STRDUP(errors[n].disk, vm->def->disks[i]->dst) < 0)
> +                goto cleanup;
> +            errors[n].error = codes[n % ncodes];

This can be simplified. This enum in question is guaranteed to have 
continuous items and _LAST to be the last item. Hence, this can be 
rewritten as:

errors[i].error = (i % (VIR_DOMAIN_DISK_ERROR_LAST - 1)) + 1;

But if we want to simplify it a little bit more (so that 
VIR_DOMAIN_DISK_ERROR_NONE is possible too) we can do it like this:

errors[i].error = i % VIR_DOMAIN_DISK_ERROR_LAST;

This immediatelly drops @codes, @ncodes and @n variables.
Also, there's MIN() macro ;-)
Yeah, libvirt has a lot of marcos and helpers and it's not obvious at 
the first glance, but I guess that's like so with every bigger project, 
isn't it?

> +            n++;
> +        }
> +        ret = n;
> +    }
> +
> + cleanup:
> +    virDomainObjEndAPI(&vm);
> +    if (ret < 0) {
> +        for (i = 0; i < n; i++)
> +            VIR_FREE(errors[i].disk);
> +    }
> +    return ret;
> +}
> +
>   static char *testDomainGetSchedulerType(virDomainPtr domain ATTRIBUTE_UNUSED,
>                                           int *nparams)
>   {
> @@ -6832,6 +6873,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 */


I'm cleaning the patch as outlined, ACKing and pushing.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] test_driver: implement virDomainGetDiskErrors
Posted by Ilias Stamatis 5 years, 6 months ago 
On Mon, May 13, 2019 at 2:38 PM Michal Privoznik <mprivozn@redhat.com> wrote:
>
> On 5/13/19 1:26 AM, Ilias Stamatis wrote:
> > Return the number of disks present in the configuration of the test
> > domain when called with @errors as NULL and @maxerrors as 0.
> >
> > Otherwise report an error for every second disk, assigning available
> > error codes in a cyclic order.
> >
> > Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
> > ---
> >   src/test/test_driver.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 42 insertions(+)
> >
> > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > index a06d1fc402..527c2f5d3b 100644
> > --- a/src/test/test_driver.c
> > +++ b/src/test/test_driver.c
> > @@ -3046,6 +3046,47 @@ static int testDomainSetAutostart(virDomainPtr domain,
> >       return 0;
> >   }
> >
> > +static int testDomainGetDiskErrors(virDomainPtr dom,
> > +                                   virDomainDiskErrorPtr errors,
> > +                                   unsigned int maxerrors,
> > +                                   unsigned int flags)
> > +{
> > +    virDomainObjPtr vm = NULL;
> > +    int ret = -1;
> > +    size_t i;
> > +    int n = 0;
> > +    int codes[] = {VIR_DOMAIN_DISK_ERROR_UNSPEC, VIR_DOMAIN_DISK_ERROR_NO_SPACE};
> > +    size_t ncodes = sizeof(codes) / sizeof(codes[0]);
>
> We have ARRAY_CARDINALITY() for this.
> Also, there's no need to create @codes array as we'll see later.

Aah, okay, I wasn't aware of this macro!

>
> > +
> > +    virCheckFlags(0, -1);
> > +
> > +    if (!(vm = testDomObjFromDomain(dom)))
> > +        goto cleanup;
> > +
> > +    if (virDomainObjCheckActive(vm) < 0)
> > +        goto cleanup;
> > +
> > +    if (!errors) {
> > +        ret = vm->def->ndisks;
>
> Tiny nitpick, we tend to put the longer body first, which in this case
> is the else branch.

Hmm, I would normally do it like that but I noticed this in the "Curly
braces" section of the Contributor guidelines [1]:

"""Also, if the else block is much shorter than the if block, consider
negating the if-condition and swapping the bodies, putting the short
block first and making the longer, multi-line block be the else
block."""

So what is the case?

>
> > +    } else {
> > +        for (i = 1; i < vm->def->ndisks && n < maxerrors; i += 2) {
> > +            if (VIR_STRDUP(errors[n].disk, vm->def->disks[i]->dst) < 0)
> > +                goto cleanup;
> > +            errors[n].error = codes[n % ncodes];
>
> This can be simplified. This enum in question is guaranteed to have
> continuous items and _LAST to be the last item. Hence, this can be
> rewritten as:
>
> errors[i].error = (i % (VIR_DOMAIN_DISK_ERROR_LAST - 1)) + 1;
>

I am aware of this but I wasn't sure if it would be very clean if I
started adding -1s and +1s in places.

> But if we want to simplify it a little bit more (so that
> VIR_DOMAIN_DISK_ERROR_NONE is possible too) we can do it like this:
>
> errors[i].error = i % VIR_DOMAIN_DISK_ERROR_LAST;

Hmm, in the documentation for this function [2] it is stated that:

"""Disks with no error will not be returned in the @errors array."""

So it seems to me that VIR_DOMAIN_DISK_ERROR_NONE shouldn't be
included. What do you think?

>
> This immediatelly drops @codes, @ncodes and @n variables.
> Also, there's MIN() macro ;-)

Cool, but where exactly do you suggest using MIN() here? Or you're
just mentioning it?

> Yeah, libvirt has a lot of marcos and helpers and it's not obvious at
> the first glance, but I guess that's like so with every bigger project,
> isn't it?

Yes, and I find these wrappers very useful / handy ;)
>
> > +            n++;
> > +        }
> > +        ret = n;
> > +    }
> > +
> > + cleanup:
> > +    virDomainObjEndAPI(&vm);
> > +    if (ret < 0) {
> > +        for (i = 0; i < n; i++)
> > +            VIR_FREE(errors[i].disk);
> > +    }
> > +    return ret;
> > +}
> > +
> >   static char *testDomainGetSchedulerType(virDomainPtr domain ATTRIBUTE_UNUSED,
> >                                           int *nparams)
> >   {
> > @@ -6832,6 +6873,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 */
>
>
> I'm cleaning the patch as outlined, ACKing and pushing.

Sure, or I could fix it if you wanted.

Thank you very much!
Ilias

[1] https://libvirt.org/hacking.html#curly_braces
[2] https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainGetDiskErrors
>
> Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] test_driver: implement virDomainGetDiskErrors
Posted by John Ferlan 5 years, 6 months ago 

On 5/13/19 9:04 AM, Ilias Stamatis wrote:
> On Mon, May 13, 2019 at 2:38 PM Michal Privoznik <mprivozn@redhat.com> wrote:
>>
>> On 5/13/19 1:26 AM, Ilias Stamatis wrote:
>>> Return the number of disks present in the configuration of the test
>>> domain when called with @errors as NULL and @maxerrors as 0.
>>>
>>> Otherwise report an error for every second disk, assigning available
>>> error codes in a cyclic order.
>>>
>>> Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
>>> ---
>>>   src/test/test_driver.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 42 insertions(+)
>>>
>>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
>>> index a06d1fc402..527c2f5d3b 100644
>>> --- a/src/test/test_driver.c
>>> +++ b/src/test/test_driver.c
>>> @@ -3046,6 +3046,47 @@ static int testDomainSetAutostart(virDomainPtr domain,
>>>       return 0;
>>>   }
>>>
>>> +static int testDomainGetDiskErrors(virDomainPtr dom,
>>> +                                   virDomainDiskErrorPtr errors,
>>> +                                   unsigned int maxerrors,
>>> +                                   unsigned int flags)
>>> +{

[...]

>>> +            n++;
>>> +        }
>>> +        ret = n;
>>> +    }
>>> +
>>> + cleanup:
>>> +    virDomainObjEndAPI(&vm);
>>> +    if (ret < 0) {
>>> +        for (i = 0; i < n; i++)
>>> +            VIR_FREE(errors[i].disk);
>>> +    }

The above got changed to :

+ cleanup:
+    virDomainObjEndAPI(&vm);
+    if (ret < 0) {
+        for (i = 0; i < MIN(vm->def->ndisks, maxerrors); i++)
+            VIR_FREE(errors[i].disk);
+    }

and Coverity got a wee bit grumpy for a couple of reasons...

  - The virDomainObjEndAPI will set @vm = NULL which makes the MIN
statement quite unhappy if ret < 0
  - However, just moving that to after the if condition isn't good
enough since the testDomObjFromDomain could causes us to jump to
cleanup: with @vm = NULL (easily solved by return -1 there instead).

John



[...]

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] test_driver: implement virDomainGetDiskErrors
Posted by Ilias Stamatis 5 years, 6 months ago 
On Tue, May 14, 2019 at 12:40 PM John Ferlan <jferlan@redhat.com> wrote:
>
>
>
> On 5/13/19 9:04 AM, Ilias Stamatis wrote:
> > On Mon, May 13, 2019 at 2:38 PM Michal Privoznik <mprivozn@redhat.com> wrote:
> >>
> >> On 5/13/19 1:26 AM, Ilias Stamatis wrote:
> >>> Return the number of disks present in the configuration of the test
> >>> domain when called with @errors as NULL and @maxerrors as 0.
> >>>
> >>> Otherwise report an error for every second disk, assigning available
> >>> error codes in a cyclic order.
> >>>
> >>> Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
> >>> ---
> >>>   src/test/test_driver.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> >>>   1 file changed, 42 insertions(+)
> >>>
> >>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> >>> index a06d1fc402..527c2f5d3b 100644
> >>> --- a/src/test/test_driver.c
> >>> +++ b/src/test/test_driver.c
> >>> @@ -3046,6 +3046,47 @@ static int testDomainSetAutostart(virDomainPtr domain,
> >>>       return 0;
> >>>   }
> >>>
> >>> +static int testDomainGetDiskErrors(virDomainPtr dom,
> >>> +                                   virDomainDiskErrorPtr errors,
> >>> +                                   unsigned int maxerrors,
> >>> +                                   unsigned int flags)
> >>> +{
>
> [...]
>
> >>> +            n++;
> >>> +        }
> >>> +        ret = n;
> >>> +    }
> >>> +
> >>> + cleanup:
> >>> +    virDomainObjEndAPI(&vm);
> >>> +    if (ret < 0) {
> >>> +        for (i = 0; i < n; i++)
> >>> +            VIR_FREE(errors[i].disk);
> >>> +    }
>
> The above got changed to :
>
> + cleanup:
> +    virDomainObjEndAPI(&vm);
> +    if (ret < 0) {
> +        for (i = 0; i < MIN(vm->def->ndisks, maxerrors); i++)
> +            VIR_FREE(errors[i].disk);
> +    }

I think this change is incorrect and a bug lies in here.

If VIR_STRDUP fails above, memory for less than MIN(vm->def->ndisks,
maxerrors) will have been allocated, and then in the cleanup code
we'll call VIR_FREE with pointers that haven't been previously
allocated.

>
> and Coverity got a wee bit grumpy for a couple of reasons...
>
>   - The virDomainObjEndAPI will set @vm = NULL which makes the MIN
> statement quite unhappy if ret < 0
>   - However, just moving that to after the if condition isn't good
> enough since the testDomObjFromDomain could causes us to jump to
> cleanup: with @vm = NULL (easily solved by return -1 there instead).
>
> John
>
>
>
> [...]

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] test_driver: implement virDomainGetDiskErrors
Posted by Michal Privoznik 5 years, 6 months ago 
On 5/14/19 12:50 PM, Ilias Stamatis wrote:
> On Tue, May 14, 2019 at 12:40 PM John Ferlan <jferlan@redhat.com> wrote:
>>
>>
>>
>> On 5/13/19 9:04 AM, Ilias Stamatis wrote:
>>> On Mon, May 13, 2019 at 2:38 PM Michal Privoznik <mprivozn@redhat.com> wrote:
>>>>
>>>> On 5/13/19 1:26 AM, Ilias Stamatis wrote:
>>>>> Return the number of disks present in the configuration of the test
>>>>> domain when called with @errors as NULL and @maxerrors as 0.
>>>>>
>>>>> Otherwise report an error for every second disk, assigning available
>>>>> error codes in a cyclic order.
>>>>>
>>>>> Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
>>>>> ---
>>>>>    src/test/test_driver.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>>>>    1 file changed, 42 insertions(+)
>>>>>
>>>>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
>>>>> index a06d1fc402..527c2f5d3b 100644
>>>>> --- a/src/test/test_driver.c
>>>>> +++ b/src/test/test_driver.c
>>>>> @@ -3046,6 +3046,47 @@ static int testDomainSetAutostart(virDomainPtr domain,
>>>>>        return 0;
>>>>>    }
>>>>>
>>>>> +static int testDomainGetDiskErrors(virDomainPtr dom,
>>>>> +                                   virDomainDiskErrorPtr errors,
>>>>> +                                   unsigned int maxerrors,
>>>>> +                                   unsigned int flags)
>>>>> +{
>>
>> [...]
>>
>>>>> +            n++;
>>>>> +        }
>>>>> +        ret = n;
>>>>> +    }
>>>>> +
>>>>> + cleanup:
>>>>> +    virDomainObjEndAPI(&vm);
>>>>> +    if (ret < 0) {
>>>>> +        for (i = 0; i < n; i++)
>>>>> +            VIR_FREE(errors[i].disk);
>>>>> +    }
>>
>> The above got changed to :
>>
>> + cleanup:
>> +    virDomainObjEndAPI(&vm);
>> +    if (ret < 0) {
>> +        for (i = 0; i < MIN(vm->def->ndisks, maxerrors); i++)
>> +            VIR_FREE(errors[i].disk);
>> +    }
> 
> I think this change is incorrect and a bug lies in here.
> 
> If VIR_STRDUP fails above, memory for less than MIN(vm->def->ndisks,
> maxerrors) will have been allocated, and then in the cleanup code
> we'll call VIR_FREE with pointers that haven't been previously
> allocated.

That isn't a problem. User has to passed an array that we can touch. If 
they store some data in it, well, their fault - how are we supposed to 
return anything if we can't touch the array?

> 
>>
>> and Coverity got a wee bit grumpy for a couple of reasons...
>>
>>    - The virDomainObjEndAPI will set @vm = NULL which makes the MIN
>> statement quite unhappy if ret < 0
>>    - However, just moving that to after the if condition isn't good
>> enough since the testDomObjFromDomain could causes us to jump to
>> cleanup: with @vm = NULL (easily solved by return -1 there instead).

Yep, I'll be posting patch soon.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] test_driver: implement virDomainGetDiskErrors
Posted by Ilias Stamatis 5 years, 6 months ago 
On Tue, May 14, 2019 at 5:04 PM Michal Privoznik <mprivozn@redhat.com> wrote:
>
> On 5/14/19 12:50 PM, Ilias Stamatis wrote:
> > On Tue, May 14, 2019 at 12:40 PM John Ferlan <jferlan@redhat.com> wrote:
> >>
> >>
> >>
> >> On 5/13/19 9:04 AM, Ilias Stamatis wrote:
> >>> On Mon, May 13, 2019 at 2:38 PM Michal Privoznik <mprivozn@redhat.com> wrote:
> >>>>
> >>>> On 5/13/19 1:26 AM, Ilias Stamatis wrote:
> >>>>> Return the number of disks present in the configuration of the test
> >>>>> domain when called with @errors as NULL and @maxerrors as 0.
> >>>>>
> >>>>> Otherwise report an error for every second disk, assigning available
> >>>>> error codes in a cyclic order.
> >>>>>
> >>>>> Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
> >>>>> ---
> >>>>>    src/test/test_driver.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> >>>>>    1 file changed, 42 insertions(+)
> >>>>>
> >>>>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> >>>>> index a06d1fc402..527c2f5d3b 100644
> >>>>> --- a/src/test/test_driver.c
> >>>>> +++ b/src/test/test_driver.c
> >>>>> @@ -3046,6 +3046,47 @@ static int testDomainSetAutostart(virDomainPtr domain,
> >>>>>        return 0;
> >>>>>    }
> >>>>>
> >>>>> +static int testDomainGetDiskErrors(virDomainPtr dom,
> >>>>> +                                   virDomainDiskErrorPtr errors,
> >>>>> +                                   unsigned int maxerrors,
> >>>>> +                                   unsigned int flags)
> >>>>> +{
> >>
> >> [...]
> >>
> >>>>> +            n++;
> >>>>> +        }
> >>>>> +        ret = n;
> >>>>> +    }
> >>>>> +
> >>>>> + cleanup:
> >>>>> +    virDomainObjEndAPI(&vm);
> >>>>> +    if (ret < 0) {
> >>>>> +        for (i = 0; i < n; i++)
> >>>>> +            VIR_FREE(errors[i].disk);
> >>>>> +    }
> >>
> >> The above got changed to :
> >>
> >> + cleanup:
> >> +    virDomainObjEndAPI(&vm);
> >> +    if (ret < 0) {
> >> +        for (i = 0; i < MIN(vm->def->ndisks, maxerrors); i++)
> >> +            VIR_FREE(errors[i].disk);
> >> +    }
> >
> > I think this change is incorrect and a bug lies in here.
> >
> > If VIR_STRDUP fails above, memory for less than MIN(vm->def->ndisks,
> > maxerrors) will have been allocated, and then in the cleanup code
> > we'll call VIR_FREE with pointers that haven't been previously
> > allocated.
>
> That isn't a problem. User has to passed an array that we can touch. If
> they store some data in it, well, their fault - how are we supposed to
> return anything if we can't touch the array?

I'm not sure I understand exactly what you mean.

We can touch the array of course.

What I'm saying is that we allocate memory with VIR_STRDUP for each
errors[i].disk, but if the call fails we free this memory on our own.

However how it is implemented now we might call VIR_FREE on pointers
for which we have *not* allocated any memory.

Because in the first loop, VIR_STRDUP might fail and send us to
"cleanup". But then on cleanup we iterate over the whole errors array.

Isn't this incorrect? Do I understand something wrong?

>
> >
> >>
> >> and Coverity got a wee bit grumpy for a couple of reasons...
> >>
> >>    - The virDomainObjEndAPI will set @vm = NULL which makes the MIN
> >> statement quite unhappy if ret < 0
> >>    - However, just moving that to after the if condition isn't good
> >> enough since the testDomObjFromDomain could causes us to jump to
> >> cleanup: with @vm = NULL (easily solved by return -1 there instead).
>
> Yep, I'll be posting patch soon.
>
> Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] test_driver: implement virDomainGetDiskErrors
Posted by Michal Privoznik 5 years, 6 months ago 
On 5/14/19 5:24 PM, Ilias Stamatis wrote:
> On Tue, May 14, 2019 at 5:04 PM Michal Privoznik <mprivozn@redhat.com> wrote:
>>
>> On 5/14/19 12:50 PM, Ilias Stamatis wrote:
>>> On Tue, May 14, 2019 at 12:40 PM John Ferlan <jferlan@redhat.com> wrote:
>>>>
>>>>
>>>>
>>>> On 5/13/19 9:04 AM, Ilias Stamatis wrote:
>>>>> On Mon, May 13, 2019 at 2:38 PM Michal Privoznik <mprivozn@redhat.com> wrote:
>>>>>>
>>>>>> On 5/13/19 1:26 AM, Ilias Stamatis wrote:
>>>>>>> Return the number of disks present in the configuration of the test
>>>>>>> domain when called with @errors as NULL and @maxerrors as 0.
>>>>>>>
>>>>>>> Otherwise report an error for every second disk, assigning available
>>>>>>> error codes in a cyclic order.
>>>>>>>
>>>>>>> Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
>>>>>>> ---
>>>>>>>     src/test/test_driver.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>>>>>>     1 file changed, 42 insertions(+)
>>>>>>>
>>>>>>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
>>>>>>> index a06d1fc402..527c2f5d3b 100644
>>>>>>> --- a/src/test/test_driver.c
>>>>>>> +++ b/src/test/test_driver.c
>>>>>>> @@ -3046,6 +3046,47 @@ static int testDomainSetAutostart(virDomainPtr domain,
>>>>>>>         return 0;
>>>>>>>     }
>>>>>>>
>>>>>>> +static int testDomainGetDiskErrors(virDomainPtr dom,
>>>>>>> +                                   virDomainDiskErrorPtr errors,
>>>>>>> +                                   unsigned int maxerrors,
>>>>>>> +                                   unsigned int flags)
>>>>>>> +{
>>>>
>>>> [...]
>>>>
>>>>>>> +            n++;
>>>>>>> +        }
>>>>>>> +        ret = n;
>>>>>>> +    }
>>>>>>> +
>>>>>>> + cleanup:
>>>>>>> +    virDomainObjEndAPI(&vm);
>>>>>>> +    if (ret < 0) {
>>>>>>> +        for (i = 0; i < n; i++)
>>>>>>> +            VIR_FREE(errors[i].disk);
>>>>>>> +    }
>>>>
>>>> The above got changed to :
>>>>
>>>> + cleanup:
>>>> +    virDomainObjEndAPI(&vm);
>>>> +    if (ret < 0) {
>>>> +        for (i = 0; i < MIN(vm->def->ndisks, maxerrors); i++)
>>>> +            VIR_FREE(errors[i].disk);
>>>> +    }
>>>
>>> I think this change is incorrect and a bug lies in here.
>>>
>>> If VIR_STRDUP fails above, memory for less than MIN(vm->def->ndisks,
>>> maxerrors) will have been allocated, and then in the cleanup code
>>> we'll call VIR_FREE with pointers that haven't been previously
>>> allocated.
>>
>> That isn't a problem. User has to passed an array that we can touch. If
>> they store some data in it, well, their fault - how are we supposed to
>> return anything if we can't touch the array?
> 
> I'm not sure I understand exactly what you mean.
> 
> We can touch the array of course.
> 
> What I'm saying is that we allocate memory with VIR_STRDUP for each
> errors[i].disk, but if the call fails we free this memory on our own.
> 
> However how it is implemented now we might call VIR_FREE on pointers
> for which we have *not* allocated any memory.
> 
> Because in the first loop, VIR_STRDUP might fail and send us to
> "cleanup". But then on cleanup we iterate over the whole errors array.
> 
> Isn't this incorrect? Do I understand something wrong?


Ah, now I get it. If user passes an array that is not zeroed out then we 
might end up passing a random pointer to free(). How about this then?

     if (ret < 0) {
         while (i > 0)
             VIR_FREE(errors[i--].disk);
     }

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] test_driver: implement virDomainGetDiskErrors
Posted by Ilias Stamatis 5 years, 6 months ago 
On Wed, May 15, 2019 at 10:14 AM Michal Privoznik <mprivozn@redhat.com> wrote:
>
> On 5/14/19 5:24 PM, Ilias Stamatis wrote:
> > On Tue, May 14, 2019 at 5:04 PM Michal Privoznik <mprivozn@redhat.com> wrote:
> >>
> >> On 5/14/19 12:50 PM, Ilias Stamatis wrote:
> >>> On Tue, May 14, 2019 at 12:40 PM John Ferlan <jferlan@redhat.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 5/13/19 9:04 AM, Ilias Stamatis wrote:
> >>>>> On Mon, May 13, 2019 at 2:38 PM Michal Privoznik <mprivozn@redhat.com> wrote:
> >>>>>>
> >>>>>> On 5/13/19 1:26 AM, Ilias Stamatis wrote:
> >>>>>>> Return the number of disks present in the configuration of the test
> >>>>>>> domain when called with @errors as NULL and @maxerrors as 0.
> >>>>>>>
> >>>>>>> Otherwise report an error for every second disk, assigning available
> >>>>>>> error codes in a cyclic order.
> >>>>>>>
> >>>>>>> Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
> >>>>>>> ---
> >>>>>>>     src/test/test_driver.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> >>>>>>>     1 file changed, 42 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> >>>>>>> index a06d1fc402..527c2f5d3b 100644
> >>>>>>> --- a/src/test/test_driver.c
> >>>>>>> +++ b/src/test/test_driver.c
> >>>>>>> @@ -3046,6 +3046,47 @@ static int testDomainSetAutostart(virDomainPtr domain,
> >>>>>>>         return 0;
> >>>>>>>     }
> >>>>>>>
> >>>>>>> +static int testDomainGetDiskErrors(virDomainPtr dom,
> >>>>>>> +                                   virDomainDiskErrorPtr errors,
> >>>>>>> +                                   unsigned int maxerrors,
> >>>>>>> +                                   unsigned int flags)
> >>>>>>> +{
> >>>>
> >>>> [...]
> >>>>
> >>>>>>> +            n++;
> >>>>>>> +        }
> >>>>>>> +        ret = n;
> >>>>>>> +    }
> >>>>>>> +
> >>>>>>> + cleanup:
> >>>>>>> +    virDomainObjEndAPI(&vm);
> >>>>>>> +    if (ret < 0) {
> >>>>>>> +        for (i = 0; i < n; i++)
> >>>>>>> +            VIR_FREE(errors[i].disk);
> >>>>>>> +    }
> >>>>
> >>>> The above got changed to :
> >>>>
> >>>> + cleanup:
> >>>> +    virDomainObjEndAPI(&vm);
> >>>> +    if (ret < 0) {
> >>>> +        for (i = 0; i < MIN(vm->def->ndisks, maxerrors); i++)
> >>>> +            VIR_FREE(errors[i].disk);
> >>>> +    }
> >>>
> >>> I think this change is incorrect and a bug lies in here.
> >>>
> >>> If VIR_STRDUP fails above, memory for less than MIN(vm->def->ndisks,
> >>> maxerrors) will have been allocated, and then in the cleanup code
> >>> we'll call VIR_FREE with pointers that haven't been previously
> >>> allocated.
> >>
> >> That isn't a problem. User has to passed an array that we can touch. If
> >> they store some data in it, well, their fault - how are we supposed to
> >> return anything if we can't touch the array?
> >
> > I'm not sure I understand exactly what you mean.
> >
> > We can touch the array of course.
> >
> > What I'm saying is that we allocate memory with VIR_STRDUP for each
> > errors[i].disk, but if the call fails we free this memory on our own.
> >
> > However how it is implemented now we might call VIR_FREE on pointers
> > for which we have *not* allocated any memory.
> >
> > Because in the first loop, VIR_STRDUP might fail and send us to
> > "cleanup". But then on cleanup we iterate over the whole errors array.
> >
> > Isn't this incorrect? Do I understand something wrong?
>
>
> Ah, now I get it. If user passes an array that is not zeroed out then we
> might end up passing a random pointer to free(). How about this then?
>
>      if (ret < 0) {
>          while (i > 0)
>              VIR_FREE(errors[i--].disk);
>      }
>

Yes, this would work I think. And then the other changes in the
cleanup etc are not needed.

Ie it can be again:

    if (!(vm = testDomObjFromDomain(dom)))
        goto cleanup;

instead of "return -1" which is more consistent with the rest of the code.

However the code now returns errors for all disks. I thought we wanted
to report errors only for some of them?

In that case we would need to use the @n variable, as it was
initially. It is used in the same way in the qemu driver.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] test_driver: implement virDomainGetDiskErrors
Posted by Michal Privoznik 5 years, 6 months ago 
On 5/15/19 11:49 AM, Ilias Stamatis wrote:
> On Wed, May 15, 2019 at 10:14 AM Michal Privoznik <mprivozn@redhat.com> wrote:
>>
>> On 5/14/19 5:24 PM, Ilias Stamatis wrote:
>>> On Tue, May 14, 2019 at 5:04 PM Michal Privoznik <mprivozn@redhat.com> wrote:
>>>>
>>>> On 5/14/19 12:50 PM, Ilias Stamatis wrote:
>>>>> On Tue, May 14, 2019 at 12:40 PM John Ferlan <jferlan@redhat.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 5/13/19 9:04 AM, Ilias Stamatis wrote:
>>>>>>> On Mon, May 13, 2019 at 2:38 PM Michal Privoznik <mprivozn@redhat.com> wrote:
>>>>>>>>
>>>>>>>> On 5/13/19 1:26 AM, Ilias Stamatis wrote:
>>>>>>>>> Return the number of disks present in the configuration of the test
>>>>>>>>> domain when called with @errors as NULL and @maxerrors as 0.
>>>>>>>>>
>>>>>>>>> Otherwise report an error for every second disk, assigning available
>>>>>>>>> error codes in a cyclic order.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
>>>>>>>>> ---
>>>>>>>>>      src/test/test_driver.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>      1 file changed, 42 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
>>>>>>>>> index a06d1fc402..527c2f5d3b 100644
>>>>>>>>> --- a/src/test/test_driver.c
>>>>>>>>> +++ b/src/test/test_driver.c
>>>>>>>>> @@ -3046,6 +3046,47 @@ static int testDomainSetAutostart(virDomainPtr domain,
>>>>>>>>>          return 0;
>>>>>>>>>      }
>>>>>>>>>
>>>>>>>>> +static int testDomainGetDiskErrors(virDomainPtr dom,
>>>>>>>>> +                                   virDomainDiskErrorPtr errors,
>>>>>>>>> +                                   unsigned int maxerrors,
>>>>>>>>> +                                   unsigned int flags)
>>>>>>>>> +{
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>>>> +            n++;
>>>>>>>>> +        }
>>>>>>>>> +        ret = n;
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>> + cleanup:
>>>>>>>>> +    virDomainObjEndAPI(&vm);
>>>>>>>>> +    if (ret < 0) {
>>>>>>>>> +        for (i = 0; i < n; i++)
>>>>>>>>> +            VIR_FREE(errors[i].disk);
>>>>>>>>> +    }
>>>>>>
>>>>>> The above got changed to :
>>>>>>
>>>>>> + cleanup:
>>>>>> +    virDomainObjEndAPI(&vm);
>>>>>> +    if (ret < 0) {
>>>>>> +        for (i = 0; i < MIN(vm->def->ndisks, maxerrors); i++)
>>>>>> +            VIR_FREE(errors[i].disk);
>>>>>> +    }
>>>>>
>>>>> I think this change is incorrect and a bug lies in here.
>>>>>
>>>>> If VIR_STRDUP fails above, memory for less than MIN(vm->def->ndisks,
>>>>> maxerrors) will have been allocated, and then in the cleanup code
>>>>> we'll call VIR_FREE with pointers that haven't been previously
>>>>> allocated.
>>>>
>>>> That isn't a problem. User has to passed an array that we can touch. If
>>>> they store some data in it, well, their fault - how are we supposed to
>>>> return anything if we can't touch the array?
>>>
>>> I'm not sure I understand exactly what you mean.
>>>
>>> We can touch the array of course.
>>>
>>> What I'm saying is that we allocate memory with VIR_STRDUP for each
>>> errors[i].disk, but if the call fails we free this memory on our own.
>>>
>>> However how it is implemented now we might call VIR_FREE on pointers
>>> for which we have *not* allocated any memory.
>>>
>>> Because in the first loop, VIR_STRDUP might fail and send us to
>>> "cleanup". But then on cleanup we iterate over the whole errors array.
>>>
>>> Isn't this incorrect? Do I understand something wrong?
>>
>>
>> Ah, now I get it. If user passes an array that is not zeroed out then we
>> might end up passing a random pointer to free(). How about this then?
>>
>>       if (ret < 0) {
>>           while (i > 0)
>>               VIR_FREE(errors[i--].disk);
>>       }
>>
> 
> Yes, this would work I think. And then the other changes in the
> cleanup etc are not needed.
> 
> Ie it can be again:
> 
>      if (!(vm = testDomObjFromDomain(dom)))
>          goto cleanup;
> 
> instead of "return -1" which is more consistent with the rest of the code.

This is done in 1/2. Or what do you mean?

> 
> However the code now returns errors for all disks. I thought we wanted
> to report errors only for some of them?

Doesn't matter really.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] test_driver: implement virDomainGetDiskErrors
Posted by Ilias Stamatis 5 years, 6 months ago 
On Thu, May 16, 2019 at 9:44 AM Michal Privoznik <mprivozn@redhat.com> wrote:
>
> On 5/15/19 11:49 AM, Ilias Stamatis wrote:
> > On Wed, May 15, 2019 at 10:14 AM Michal Privoznik <mprivozn@redhat.com> wrote:
> >>
> >> On 5/14/19 5:24 PM, Ilias Stamatis wrote:
> >>> On Tue, May 14, 2019 at 5:04 PM Michal Privoznik <mprivozn@redhat.com> wrote:
> >>>>
> >>>> On 5/14/19 12:50 PM, Ilias Stamatis wrote:
> >>>>> On Tue, May 14, 2019 at 12:40 PM John Ferlan <jferlan@redhat.com> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 5/13/19 9:04 AM, Ilias Stamatis wrote:
> >>>>>>> On Mon, May 13, 2019 at 2:38 PM Michal Privoznik <mprivozn@redhat.com> wrote:
> >>>>>>>>
> >>>>>>>> On 5/13/19 1:26 AM, Ilias Stamatis wrote:
> >>>>>>>>> Return the number of disks present in the configuration of the test
> >>>>>>>>> domain when called with @errors as NULL and @maxerrors as 0.
> >>>>>>>>>
> >>>>>>>>> Otherwise report an error for every second disk, assigning available
> >>>>>>>>> error codes in a cyclic order.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
> >>>>>>>>> ---
> >>>>>>>>>      src/test/test_driver.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> >>>>>>>>>      1 file changed, 42 insertions(+)
> >>>>>>>>>
> >>>>>>>>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> >>>>>>>>> index a06d1fc402..527c2f5d3b 100644
> >>>>>>>>> --- a/src/test/test_driver.c
> >>>>>>>>> +++ b/src/test/test_driver.c
> >>>>>>>>> @@ -3046,6 +3046,47 @@ static int testDomainSetAutostart(virDomainPtr domain,
> >>>>>>>>>          return 0;
> >>>>>>>>>      }
> >>>>>>>>>
> >>>>>>>>> +static int testDomainGetDiskErrors(virDomainPtr dom,
> >>>>>>>>> +                                   virDomainDiskErrorPtr errors,
> >>>>>>>>> +                                   unsigned int maxerrors,
> >>>>>>>>> +                                   unsigned int flags)
> >>>>>>>>> +{
> >>>>>>
> >>>>>> [...]
> >>>>>>
> >>>>>>>>> +            n++;
> >>>>>>>>> +        }
> >>>>>>>>> +        ret = n;
> >>>>>>>>> +    }
> >>>>>>>>> +
> >>>>>>>>> + cleanup:
> >>>>>>>>> +    virDomainObjEndAPI(&vm);
> >>>>>>>>> +    if (ret < 0) {
> >>>>>>>>> +        for (i = 0; i < n; i++)
> >>>>>>>>> +            VIR_FREE(errors[i].disk);
> >>>>>>>>> +    }
> >>>>>>
> >>>>>> The above got changed to :
> >>>>>>
> >>>>>> + cleanup:
> >>>>>> +    virDomainObjEndAPI(&vm);
> >>>>>> +    if (ret < 0) {
> >>>>>> +        for (i = 0; i < MIN(vm->def->ndisks, maxerrors); i++)
> >>>>>> +            VIR_FREE(errors[i].disk);
> >>>>>> +    }
> >>>>>
> >>>>> I think this change is incorrect and a bug lies in here.
> >>>>>
> >>>>> If VIR_STRDUP fails above, memory for less than MIN(vm->def->ndisks,
> >>>>> maxerrors) will have been allocated, and then in the cleanup code
> >>>>> we'll call VIR_FREE with pointers that haven't been previously
> >>>>> allocated.
> >>>>
> >>>> That isn't a problem. User has to passed an array that we can touch. If
> >>>> they store some data in it, well, their fault - how are we supposed to
> >>>> return anything if we can't touch the array?
> >>>
> >>> I'm not sure I understand exactly what you mean.
> >>>
> >>> We can touch the array of course.
> >>>
> >>> What I'm saying is that we allocate memory with VIR_STRDUP for each
> >>> errors[i].disk, but if the call fails we free this memory on our own.
> >>>
> >>> However how it is implemented now we might call VIR_FREE on pointers
> >>> for which we have *not* allocated any memory.
> >>>
> >>> Because in the first loop, VIR_STRDUP might fail and send us to
> >>> "cleanup". But then on cleanup we iterate over the whole errors array.
> >>>
> >>> Isn't this incorrect? Do I understand something wrong?
> >>
> >>
> >> Ah, now I get it. If user passes an array that is not zeroed out then we
> >> might end up passing a random pointer to free(). How about this then?
> >>
> >>       if (ret < 0) {
> >>           while (i > 0)
> >>               VIR_FREE(errors[i--].disk);
> >>       }
> >>
> >
> > Yes, this would work I think. And then the other changes in the
> > cleanup etc are not needed.
> >
> > Ie it can be again:
> >
> >      if (!(vm = testDomObjFromDomain(dom)))
> >          goto cleanup;
> >
> > instead of "return -1" which is more consistent with the rest of the code.
>
> This is done in 1/2. Or what do you mean?

I meant that the previous change of returning -1 directly instead of
doing "goto cleanup" is not needed anymore. But of course it's fine
either way. Just with the goto, there will be only a single point of
exit.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] test_driver: implement virDomainGetDiskErrors
Posted by Peter Krempa 5 years, 6 months ago 
On Wed, May 15, 2019 at 10:14:35 +0200, Michal Privoznik wrote:
> On 5/14/19 5:24 PM, Ilias Stamatis wrote:
> > On Tue, May 14, 2019 at 5:04 PM Michal Privoznik <mprivozn@redhat.com> wrote:

[...]

> > Because in the first loop, VIR_STRDUP might fail and send us to
> > "cleanup". But then on cleanup we iterate over the whole errors array.
> > 
> > Isn't this incorrect? Do I understand something wrong?
> 
> 
> Ah, now I get it. If user passes an array that is not zeroed out then we
> might end up passing a random pointer to free(). How about this then?

Why don't you just sanitize the user-passed memory first then?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.