The current implementation has the following bugs:

- the vm variable is accessed after calling virDomainObjEndAPI on it

- if VIR_STRDUP fails and we jump to the cleanup section, we're calling
VIR_FREE on pointers for which we haven't allocated memory

- the error type VIR_DOMAIN_DISK_ERROR_NONE is used which contradicts
the documentation of the API that says that disks with no errors are not
reported

This patch fixes all of them and additionally reports errors only for
every second disk (instead of reporting errors for all disks) which was
the initial intention.

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

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 2a0ffbc6c5..a7e40112d1 100755
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -3246,6 +3246,7 @@ static int testDomainGetDiskErrors(virDomainPtr dom,
                                    unsigned int flags)
 {
     virDomainObjPtr vm = NULL;
+    int n = 0;
     int ret = -1;
     size_t i;

@@ -3258,12 +3259,13 @@ static int testDomainGetDiskErrors(virDomainPtr dom,
         goto cleanup;

     if (errors) {
-        for (i = 0; i < MIN(vm->def->ndisks, maxerrors); i++) {
-            if (VIR_STRDUP(errors[i].disk, vm->def->disks[i]->dst) < 0)
+        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[i].error = i % VIR_DOMAIN_DISK_ERROR_LAST;
+            errors[n].error = (n % (VIR_DOMAIN_DISK_ERROR_LAST - 1)) + 1;
+            n++;
         }
-        ret = i;
+        ret = n;
     } else {
         ret = vm->def->ndisks;
     }
@@ -3271,7 +3273,7 @@ static int testDomainGetDiskErrors(virDomainPtr dom,
  cleanup:
     virDomainObjEndAPI(&vm);
     if (ret < 0) {
-        for (i = 0; i < MIN(vm->def->ndisks, maxerrors); i++)
+        for (i = 0; i < n; i++)
             VIR_FREE(errors[i].disk);
     }
     return ret;
--
2.22.0

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

On Thu, Jun 20, 2019 at 05:11:57PM +0200, Ilias Stamatis wrote:
> The current implementation has the following bugs:
>
> - the vm variable is accessed after calling virDomainObjEndAPI on it
>
> - if VIR_STRDUP fails and we jump to the cleanup section, we're calling
> VIR_FREE on pointers for which we haven't allocated memory
>
> - the error type VIR_DOMAIN_DISK_ERROR_NONE is used which contradicts
> the documentation of the API that says that disks with no errors are not
> reported
>
> This patch fixes all of them and additionally reports errors only for
> every second disk (instead of reporting errors for all disks) which was
> the initial intention.

I believe each patch should focus ideally on 1 thing only. So this patch should
IMO be split into 3 (doesn't matter in what order), each fixing a different bug.

Erik

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

On Tue, Jul 2, 2019 at 10:04 AM Erik Skultety <eskultet@redhat.com> wrote:
>
> On Thu, Jun 20, 2019 at 05:11:57PM +0200, Ilias Stamatis wrote:
> > The current implementation has the following bugs:
> >
> > - the vm variable is accessed after calling virDomainObjEndAPI on it
> >
> > - if VIR_STRDUP fails and we jump to the cleanup section, we're calling
> > VIR_FREE on pointers for which we haven't allocated memory
> >
> > - the error type VIR_DOMAIN_DISK_ERROR_NONE is used which contradicts
> > the documentation of the API that says that disks with no errors are not
> > reported
> >
> > This patch fixes all of them and additionally reports errors only for
> > every second disk (instead of reporting errors for all disks) which was
> > the initial intention.
>
> I believe each patch should focus ideally on 1 thing only. So this patch should
> IMO be split into 3 (doesn't matter in what order), each fixing a different bug.
>
> Erik

Hmm. I think it's impossible to solve the first 2 separately. Because
by solving the 2nd one the 1st is solved as well. The 3rd one is
really adding a "-1" and a "+1" in the VIR_DOMAIN_DISK_ERROR_LAST
line. But this line is touched anyway by the previous fix as well. So
I thought it doesn't worth it to introduce a separate patch for it.

I think the patch in general is quite small so it shouldn't bet a huge
problem, but if you insist I can separate the 1-line change for fix
no3. from the ones before.

Ilias

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

On Tue, Jul 02, 2019 at 10:27:50AM +0200, Ilias Stamatis wrote:
> On Tue, Jul 2, 2019 at 10:04 AM Erik Skultety <eskultet@redhat.com> wrote:
> >
> > On Thu, Jun 20, 2019 at 05:11:57PM +0200, Ilias Stamatis wrote:
> > > The current implementation has the following bugs:
> > >
> > > - the vm variable is accessed after calling virDomainObjEndAPI on it
> > >
> > > - if VIR_STRDUP fails and we jump to the cleanup section, we're calling
> > > VIR_FREE on pointers for which we haven't allocated memory
> > >
> > > - the error type VIR_DOMAIN_DISK_ERROR_NONE is used which contradicts
> > > the documentation of the API that says that disks with no errors are not
> > > reported
> > >
> > > This patch fixes all of them and additionally reports errors only for
> > > every second disk (instead of reporting errors for all disks) which was
> > > the initial intention.
> >
> > I believe each patch should focus ideally on 1 thing only. So this patch should
> > IMO be split into 3 (doesn't matter in what order), each fixing a different bug.
> >
> > Erik
>
> Hmm. I think it's impossible to solve the first 2 separately. Because
> by solving the 2nd one the 1st is solved as well. The 3rd one is
> really adding a "-1" and a "+1" in the VIR_DOMAIN_DISK_ERROR_LAST
> line. But this line is touched anyway by the previous fix as well. So
> I thought it doesn't worth it to introduce a separate patch for it.
>
> I think the patch in general is quite small so it shouldn't bet a huge
> problem, but if you insist I can separate the 1-line change for fix
> no3. from the ones before.

Since [1] already addressed almost all of the problems, I don't think there's
any need in further review of this series.

Erik

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