[libvirt] [PATCH] Add test suite for viralloc APIs
In debugging a crash on OOM, I thought that the virInsert APIs might be at fault, but couldn't isolate them as a cause. While the viralloc APIs are used in many test suites, this is as a side-effect, they are not directly tested :-) Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tests/Makefile.am | 5 + tests/viralloctest.c | 387 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 392 insertions(+) create mode 100644 tests/viralloctest.c diff --git a/tests/Makefile.am b/tests/Makefile.am index 122bf6c..c13cc15 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -140,6 +140,7 @@ test_programs = virshtest sockettest \ viratomictest \ utiltest shunloadtest \ virtimetest viruritest virkeyfiletest \ + viralloctest \ virauthconfigtest \ virbitmaptest \ vircgrouptest \ @@ -929,6 +930,10 @@ virkeyfiletest_SOURCES = \ virkeyfiletest.c testutils.h testutils.c virkeyfiletest_LDADD = $(LDADDS) +viralloctest_SOURCES = \ + viralloctest.c testutils.h testutils.c +viralloctest_LDADD = $(LDADDS) + virauthconfigtest_SOURCES = \ virauthconfigtest.c testutils.h testutils.c virauthconfigtest_LDADD = $(LDADDS) diff --git a/tests/viralloctest.c b/tests/viralloctest.c new file mode 100644 index 0000000..abdd871 --- /dev/null +++ b/tests/viralloctest.c @@ -0,0 +1,387 @@ +/* + * viralloctest.c: Test memory allocation APIs + * + * Copyright (C) 2014 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#include <config.h> + +#include <viralloc.h> + +#include "testutils.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +typedef struct testDummyStruct { + int a; + int b; +} testDummyStruct; + +static int +testAllocScalar(const void *opaque ATTRIBUTE_UNUSED) +{ + testDummyStruct *t; + int ret = -1; + + if (VIR_ALLOC(t) < 0) + return -1; + + if (t == NULL) { + fprintf(stderr, "Allocation succeeded by pointer is NULL\n"); + goto cleanup; + } + + if (t->a != 0 || + t->b != 0) { + fprintf(stderr, "Allocated ram was not zerod\n"); + goto cleanup; + } + + VIR_FREE(t); + + if (t != NULL) { + fprintf(stderr, "Pointer is still set after free\n"); + goto cleanup; + } + + ret = 0; + cleanup: + VIR_FREE(t); + return ret; +} + + +static int +testAllocArray(const void *opaque ATTRIBUTE_UNUSED) +{ + testDummyStruct *t; + size_t nt = 10, i; + int ret = -1; + + if (VIR_ALLOC_N(t, nt) < 0) + return -1; + + if (t == NULL) { + fprintf(stderr, "Allocation succeeded by pointer is NULL\n"); + goto cleanup; + } + + for (i = 0; i < nt; i++) { + if (t[i].a != 0 || + t[i].b != 0) { + fprintf(stderr, "Allocated ram block %zu was not zerod\n", i); + goto cleanup; + } + } + + VIR_FREE(t); + + if (t != NULL) { + fprintf(stderr, "Pointer is still set after free\n"); + goto cleanup; + } + + ret = 0; + cleanup: + VIR_FREE(t); + return ret; +} + + +static int +testReallocArray(const void *opaque ATTRIBUTE_UNUSED) +{ + testDummyStruct *t; + size_t nt = 10, i; + int ret = -1; + + if (VIR_ALLOC_N(t, nt) < 0) + return -1; + + if (t == NULL) { + fprintf(stderr, "Allocation succeeded by pointer is NULL\n"); + goto cleanup; + } + + for (i = 0; i < nt; i++) { + t[i].a = 10; + t[i].b = 20; + } + + if (VIR_REALLOC_N(t, nt + 5) < 0) + goto cleanup; + + for (i = 0; i < nt; i++) { + if (t[i].a != 10 || + t[i].b != 20) { + fprintf(stderr, "Reallocated ram block %zu lost data\n", i); + goto cleanup; + } + } + + if (VIR_REALLOC_N(t, nt) < 0) + goto cleanup; + + for (i = 0; i < nt; i++) { + if (t[i].a != 10 || + t[i].b != 20) { + fprintf(stderr, "Reallocated ram block %zu lost data\n", i); + goto cleanup; + } + } + + if (VIR_REALLOC_N(t, nt - 5) < 0) + goto cleanup; + + for (i = 0; i < (nt - 5); i++) { + if (t[i].a != 10 || + t[i].b != 20) { + fprintf(stderr, "Reallocated ram block %zu lost data\n", i); + goto cleanup; + } + } + + VIR_FREE(t); + + if (t != NULL) { + fprintf(stderr, "Pointer is still set after free\n"); + goto cleanup; + } + + ret = 0; + cleanup: + VIR_FREE(t); + return ret; +} + + +static int +testExpandArray(const void *opaque ATTRIBUTE_UNUSED) +{ + testDummyStruct *t; + size_t nt = 10, i; + int ret = -1; + + if (VIR_ALLOC_N(t, nt) < 0) + return -1; + + if (t == NULL) { + fprintf(stderr, "Allocation succeeded by pointer is NULL\n"); + goto cleanup; + } + + for (i = 0; i < nt; i++) { + t[i].a = 10; + t[i].b = 20; + } + + if (VIR_EXPAND_N(t, nt, 5) < 0) + goto cleanup; + + for (i = 0; i < (nt - 5); i++) { + if (t[i].a != 10 || + t[i].b != 20) { + fprintf(stderr, "Reallocated ram block %zu lost data\n", i); + goto cleanup; + } + } + + for (i = (nt - 5); i < nt; i++) { + if (t[i].a != 0 || + t[i].b != 0) { + fprintf(stderr, "New ram block %zu was not zerod\n", i); + goto cleanup; + } + } + + VIR_SHRINK_N(t, nt, 5); + + for (i = 0; i < nt; i++) { + if (t[i].a != 10 || + t[i].b != 20) { + fprintf(stderr, "Reallocated ram block %zu lost data\n", i); + goto cleanup; + } + } + + VIR_SHRINK_N(t, nt, 5); + + for (i = 0; i < nt; i++) { + if (t[i].a != 10 || + t[i].b != 20) { + fprintf(stderr, "Reallocated ram block %zu lost data\n", i); + goto cleanup; + } + } + + VIR_FREE(t); + + if (t != NULL) { + fprintf(stderr, "Pointer is still set after free\n"); + goto cleanup; + } + + ret = 0; + cleanup: + VIR_FREE(t); + return ret; +} + + +static int +testResizeArray(const void *opaque ATTRIBUTE_UNUSED) +{ + testDummyStruct *t; + size_t nt = 10, at, i; + int ret = -1; + + if (VIR_ALLOC_N(t, nt) < 0) + return -1; + + at = nt; + + if (t == NULL) { + fprintf(stderr, "Allocation succeeded by pointer is NULL\n"); + goto cleanup; + } + + for (i = 0; i < nt; i++) { + t[i].a = 10; + t[i].b = 20; + } + + if (VIR_RESIZE_N(t, at, nt, 8) < 0) + goto cleanup; + + if (at != 18) { + fprintf(stderr, "Expected allocation of 16 not %zu\n", at); + goto cleanup; + } + + for (i = 0; i < at; i++) { + if (i >= nt) { + if (t[i].a != 0 || + t[i].b != 0) { + fprintf(stderr, "New ram block %zu was not zerod\n", i); + goto cleanup; + } + } else { + if (t[i].a != 10 || + t[i].b != 20) { + fprintf(stderr, "Reallocated ram block %zu lost data\n", i); + goto cleanup; + } + } + } + + VIR_FREE(t); + + if (t != NULL) { + fprintf(stderr, "Pointer is still set after free\n"); + goto cleanup; + } + + ret = 0; + cleanup: + VIR_FREE(t); + return ret; +} + + +static int +testInsertArray(const void *opaque ATTRIBUTE_UNUSED) +{ + testDummyStruct **t; + size_t nt = 10, i; + int ret = -1; + testDummyStruct *n = (void *)0xff; + + if (VIR_ALLOC_N(t, nt) < 0) + return -1; + + if (t == NULL) { + fprintf(stderr, "Allocation succeeded by pointer is NULL\n"); + goto cleanup; + } + + for (i = 0; i < nt; i++) + t[i] = (void*)0x50; + + if (VIR_INSERT_ELEMENT(t, 3, nt, n) < 0) { + if (nt != 10) { + fprintf(stderr, "Expecting array size 10 after OOM not %zu\n", nt); + goto cleanup; + } + goto cleanup; + } + + if (nt != 11) { + fprintf(stderr, "Expecting array size 11 not %zu\n", nt); + goto cleanup; + } + + if (n != NULL) { + fprintf(stderr, "Expecting element to be set to NULL\n"); + goto cleanup; + } + + for (i = 0; i < nt; i++) { + void *expect = i == 3 ? (void *)0xff : (void*)0x50; + if (t[i] != expect) { + fprintf(stderr, "Expecting %p at offset %zu not %p\n", + expect, i, t[i]); + goto cleanup; + } + } + + VIR_FREE(t); + + if (t != NULL) { + fprintf(stderr, "Pointer is still set after free\n"); + goto cleanup; + } + + ret = 0; + cleanup: + VIR_FREE(t); + return ret; +} + + +static int +mymain(void) +{ + int ret = 0; + + if (virtTestRun("alloc scalar", testAllocScalar, NULL) < 0) + ret = -1; + if (virtTestRun("alloc array", testAllocArray, NULL) < 0) + ret = -1; + if (virtTestRun("realloc array", testReallocArray, NULL) < 0) + ret = -1; + if (virtTestRun("expand array", testExpandArray, NULL) < 0) + ret = -1; + if (virtTestRun("resize array", testResizeArray, NULL) < 0) + ret = -1; + if (virtTestRun("insert array", testInsertArray, NULL) < 0) + ret = -1; + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIRT_TEST_MAIN(mymain) -- 1.9.0
On Tue, Apr 8, 2014 at 8:18 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
In debugging a crash on OOM, I thought that the virInsert APIs might be at fault, but couldn't isolate them as a cause. While the viralloc APIs are used in many test suites, this is as a side-effect, they are not directly tested :-)
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tests/Makefile.am | 5 + tests/viralloctest.c | 387 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 392 insertions(+) create mode 100644 tests/viralloctest.c
diff --git a/tests/Makefile.am b/tests/Makefile.am index 122bf6c..c13cc15 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -140,6 +140,7 @@ test_programs = virshtest sockettest \ viratomictest \ utiltest shunloadtest \ virtimetest viruritest virkeyfiletest \ + viralloctest \ virauthconfigtest \ virbitmaptest \ vircgrouptest \ @@ -929,6 +930,10 @@ virkeyfiletest_SOURCES = \ virkeyfiletest.c testutils.h testutils.c virkeyfiletest_LDADD = $(LDADDS)
+viralloctest_SOURCES = \ + viralloctest.c testutils.h testutils.c +viralloctest_LDADD = $(LDADDS) + virauthconfigtest_SOURCES = \ virauthconfigtest.c testutils.h testutils.c virauthconfigtest_LDADD = $(LDADDS) diff --git a/tests/viralloctest.c b/tests/viralloctest.c new file mode 100644 index 0000000..abdd871 --- /dev/null +++ b/tests/viralloctest.c @@ -0,0 +1,387 @@ +/* + * viralloctest.c: Test memory allocation APIs + * + * Copyright (C) 2014 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#include <config.h> + +#include <viralloc.h> + +#include "testutils.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +typedef struct testDummyStruct { + int a; + int b; +} testDummyStruct; + +static int +testAllocScalar(const void *opaque ATTRIBUTE_UNUSED) +{ + testDummyStruct *t; + int ret = -1; + + if (VIR_ALLOC(t) < 0) + return -1; + + if (t == NULL) { + fprintf(stderr, "Allocation succeeded by pointer is NULL\n"); + goto cleanup; + }
Just out of curiosity, why don't we have this check after VIR_REALLOC_N, VIR_EXPAND_N, VIR_SHRINK_N and VIR_RESIZE_N ?
+ + if (t->a != 0 || + t->b != 0) { + fprintf(stderr, "Allocated ram was not zerod\n"); + goto cleanup; + } + + VIR_FREE(t); + + if (t != NULL) { + fprintf(stderr, "Pointer is still set after free\n"); + goto cleanup; + } + + ret = 0; + cleanup: + VIR_FREE(t); + return ret; +} + + +static int +testAllocArray(const void *opaque ATTRIBUTE_UNUSED) +{ + testDummyStruct *t; + size_t nt = 10, i; + int ret = -1; + + if (VIR_ALLOC_N(t, nt) < 0) + return -1; + + if (t == NULL) { + fprintf(stderr, "Allocation succeeded by pointer is NULL\n"); + goto cleanup; + } + + for (i = 0; i < nt; i++) { + if (t[i].a != 0 || + t[i].b != 0) { + fprintf(stderr, "Allocated ram block %zu was not zerod\n", i); + goto cleanup; + } + } + + VIR_FREE(t); + + if (t != NULL) { + fprintf(stderr, "Pointer is still set after free\n"); + goto cleanup; + } + + ret = 0; + cleanup: + VIR_FREE(t); + return ret; +} + + +static int +testReallocArray(const void *opaque ATTRIBUTE_UNUSED) +{ + testDummyStruct *t; + size_t nt = 10, i; + int ret = -1; + + if (VIR_ALLOC_N(t, nt) < 0) + return -1; + + if (t == NULL) { + fprintf(stderr, "Allocation succeeded by pointer is NULL\n"); + goto cleanup; + } + + for (i = 0; i < nt; i++) { + t[i].a = 10; + t[i].b = 20; + } + + if (VIR_REALLOC_N(t, nt + 5) < 0) + goto cleanup; + + for (i = 0; i < nt; i++) { + if (t[i].a != 10 || + t[i].b != 20) { + fprintf(stderr, "Reallocated ram block %zu lost data\n", i); + goto cleanup; + } + } + + if (VIR_REALLOC_N(t, nt) < 0) + goto cleanup; + + for (i = 0; i < nt; i++) { + if (t[i].a != 10 || + t[i].b != 20) { + fprintf(stderr, "Reallocated ram block %zu lost data\n", i); + goto cleanup; + } + } + + if (VIR_REALLOC_N(t, nt - 5) < 0) + goto cleanup; + + for (i = 0; i < (nt - 5); i++) { + if (t[i].a != 10 || + t[i].b != 20) { + fprintf(stderr, "Reallocated ram block %zu lost data\n", i); + goto cleanup; + } + } + + VIR_FREE(t); + + if (t != NULL) { + fprintf(stderr, "Pointer is still set after free\n"); + goto cleanup; + } + + ret = 0; + cleanup: + VIR_FREE(t); + return ret; +} + + +static int +testExpandArray(const void *opaque ATTRIBUTE_UNUSED) +{ + testDummyStruct *t; + size_t nt = 10, i; + int ret = -1; + + if (VIR_ALLOC_N(t, nt) < 0) + return -1; + + if (t == NULL) { + fprintf(stderr, "Allocation succeeded by pointer is NULL\n"); + goto cleanup; + } + + for (i = 0; i < nt; i++) { + t[i].a = 10; + t[i].b = 20; + } + + if (VIR_EXPAND_N(t, nt, 5) < 0) + goto cleanup; + + for (i = 0; i < (nt - 5); i++) { + if (t[i].a != 10 || + t[i].b != 20) { + fprintf(stderr, "Reallocated ram block %zu lost data\n", i); + goto cleanup; + } + } + + for (i = (nt - 5); i < nt; i++) { + if (t[i].a != 0 || + t[i].b != 0) { + fprintf(stderr, "New ram block %zu was not zerod\n", i); + goto cleanup; + } + } + + VIR_SHRINK_N(t, nt, 5); + + for (i = 0; i < nt; i++) { + if (t[i].a != 10 || + t[i].b != 20) { + fprintf(stderr, "Reallocated ram block %zu lost data\n", i); + goto cleanup; + } + } + + VIR_SHRINK_N(t, nt, 5); + + for (i = 0; i < nt; i++) { + if (t[i].a != 10 || + t[i].b != 20) { + fprintf(stderr, "Reallocated ram block %zu lost data\n", i); + goto cleanup; + } + } + + VIR_FREE(t); + + if (t != NULL) { + fprintf(stderr, "Pointer is still set after free\n"); + goto cleanup; + } + + ret = 0; + cleanup: + VIR_FREE(t); + return ret; +} + + +static int +testResizeArray(const void *opaque ATTRIBUTE_UNUSED) +{ + testDummyStruct *t; + size_t nt = 10, at, i; + int ret = -1; + + if (VIR_ALLOC_N(t, nt) < 0) + return -1; + + at = nt; + + if (t == NULL) { + fprintf(stderr, "Allocation succeeded by pointer is NULL\n"); + goto cleanup; + } + + for (i = 0; i < nt; i++) { + t[i].a = 10; + t[i].b = 20; + } + + if (VIR_RESIZE_N(t, at, nt, 8) < 0) + goto cleanup; + + if (at != 18) { + fprintf(stderr, "Expected allocation of 16 not %zu\n", at); + goto cleanup; + } + + for (i = 0; i < at; i++) { + if (i >= nt) { + if (t[i].a != 0 || + t[i].b != 0) { + fprintf(stderr, "New ram block %zu was not zerod\n", i); + goto cleanup; + } + } else { + if (t[i].a != 10 || + t[i].b != 20) { + fprintf(stderr, "Reallocated ram block %zu lost data\n", i); + goto cleanup; + } + } + } + + VIR_FREE(t); + + if (t != NULL) { + fprintf(stderr, "Pointer is still set after free\n"); + goto cleanup; + } + + ret = 0; + cleanup: + VIR_FREE(t); + return ret; +} + + +static int +testInsertArray(const void *opaque ATTRIBUTE_UNUSED) +{ + testDummyStruct **t; + size_t nt = 10, i; + int ret = -1; + testDummyStruct *n = (void *)0xff; + + if (VIR_ALLOC_N(t, nt) < 0) + return -1; + + if (t == NULL) { + fprintf(stderr, "Allocation succeeded by pointer is NULL\n"); + goto cleanup; + } + + for (i = 0; i < nt; i++) + t[i] = (void*)0x50; + + if (VIR_INSERT_ELEMENT(t, 3, nt, n) < 0) { + if (nt != 10) { + fprintf(stderr, "Expecting array size 10 after OOM not %zu\n", nt); + goto cleanup; + } + goto cleanup; + } +
Isn't the extra 'goto cleanup' in the inner block redundant, as we are already going to cleanup, because of OOM?
+ if (nt != 11) { + fprintf(stderr, "Expecting array size 11 not %zu\n", nt); + goto cleanup; + } + + if (n != NULL) { + fprintf(stderr, "Expecting element to be set to NULL\n"); + goto cleanup; + } + + for (i = 0; i < nt; i++) { + void *expect = i == 3 ? (void *)0xff : (void*)0x50; + if (t[i] != expect) { + fprintf(stderr, "Expecting %p at offset %zu not %p\n", + expect, i, t[i]); + goto cleanup; + } + } + + VIR_FREE(t); + + if (t != NULL) { + fprintf(stderr, "Pointer is still set after free\n"); + goto cleanup; + } + + ret = 0; + cleanup: + VIR_FREE(t); + return ret; +} + + +static int +mymain(void) +{ + int ret = 0; + + if (virtTestRun("alloc scalar", testAllocScalar, NULL) < 0) + ret = -1; + if (virtTestRun("alloc array", testAllocArray, NULL) < 0) + ret = -1; + if (virtTestRun("realloc array", testReallocArray, NULL) < 0) + ret = -1; + if (virtTestRun("expand array", testExpandArray, NULL) < 0) + ret = -1; + if (virtTestRun("resize array", testResizeArray, NULL) < 0) + ret = -1; + if (virtTestRun("insert array", testInsertArray, NULL) < 0) + ret = -1; + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIRT_TEST_MAIN(mymain) -- 1.9.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Nehal J Wani
On 04/09/2014 12:17 PM, Nehal J Wani wrote:
On Tue, Apr 8, 2014 at 8:18 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
In debugging a crash on OOM, I thought that the virInsert APIs might be at fault, but couldn't isolate them as a cause. While the viralloc APIs are used in many test suites, this is as a side-effect, they are not directly tested :-)
+static int +testAllocScalar(const void *opaque ATTRIBUTE_UNUSED) +{ + testDummyStruct *t; + int ret = -1; + + if (VIR_ALLOC(t) < 0) + return -1; + + if (t == NULL) { + fprintf(stderr, "Allocation succeeded by pointer is NULL\n"); + goto cleanup; + }
Just out of curiosity, why don't we have this check after VIR_REALLOC_N, VIR_EXPAND_N, VIR_SHRINK_N and VIR_RESIZE_N ?
Hmm, that would probably be a reasonable enhancement to the testsuite, if you'd like to contribute it as a followup patch.
+ + if (VIR_INSERT_ELEMENT(t, 3, nt, n) < 0) { + if (nt != 10) { + fprintf(stderr, "Expecting array size 10 after OOM not %zu\n", nt); + goto cleanup; + } + goto cleanup; + } +
Isn't the extra 'goto cleanup' in the inner block redundant, as we are already going to cleanup, because of OOM?
Yes, but it also doesn't hurt. By the way, thanks for jumping in and reviewing; and you will get better at it over time. I wish more new contributors would feel comfortable giving a review; even a review that admits being uncomfortable as the sole reviewer is still better than no comment at all. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
+static int +testAllocScalar(const void *opaque ATTRIBUTE_UNUSED) +{ + testDummyStruct *t; + int ret = -1; + + if (VIR_ALLOC(t) < 0) + return -1; + + if (t == NULL) { + fprintf(stderr, "Allocation succeeded by pointer is NULL\n"); + goto cleanup; + }
Just out of curiosity, why don't we have this check after VIR_REALLOC_N, VIR_EXPAND_N, VIR_SHRINK_N and VIR_RESIZE_N ?
As this patch hasn't been pushed yet, maybe the following can be squashed in? diff --git a/tests/viralloctest.c b/tests/viralloctest.c index abdd871..d5818c7 100644 --- a/tests/viralloctest.c +++ b/tests/viralloctest.c @@ -33,6 +33,17 @@ typedef struct testDummyStruct { } testDummyStruct; static int +testCheckNonNull(void *t) +{ + if (t == NULL) { + fprintf(stderr, "Allocation succeeded by pointer is NULL\n"); + return -1; + } + + return 0; +} + +static int testAllocScalar(const void *opaque ATTRIBUTE_UNUSED) { testDummyStruct *t; @@ -41,10 +52,8 @@ testAllocScalar(const void *opaque ATTRIBUTE_UNUSED) if (VIR_ALLOC(t) < 0) return -1; - if (t == NULL) { - fprintf(stderr, "Allocation succeeded by pointer is NULL\n"); + if (testCheckNonNull(t) < 0) goto cleanup; - } if (t->a != 0 || t->b != 0) { @@ -76,10 +85,8 @@ testAllocArray(const void *opaque ATTRIBUTE_UNUSED) if (VIR_ALLOC_N(t, nt) < 0) return -1; - if (t == NULL) { - fprintf(stderr, "Allocation succeeded by pointer is NULL\n"); + if (testCheckNonNull(t) < 0) goto cleanup; - } for (i = 0; i < nt; i++) { if (t[i].a != 0 || @@ -113,10 +120,8 @@ testReallocArray(const void *opaque ATTRIBUTE_UNUSED) if (VIR_ALLOC_N(t, nt) < 0) return -1; - if (t == NULL) { - fprintf(stderr, "Allocation succeeded by pointer is NULL\n"); + if (testCheckNonNull(t) < 0) goto cleanup; - } for (i = 0; i < nt; i++) { t[i].a = 10; @@ -126,6 +131,9 @@ testReallocArray(const void *opaque ATTRIBUTE_UNUSED) if (VIR_REALLOC_N(t, nt + 5) < 0) goto cleanup; + if (testCheckNonNull(t) < 0) + goto cleanup; + for (i = 0; i < nt; i++) { if (t[i].a != 10 || t[i].b != 20) { @@ -137,6 +145,9 @@ testReallocArray(const void *opaque ATTRIBUTE_UNUSED) if (VIR_REALLOC_N(t, nt) < 0) goto cleanup; + if (testCheckNonNull(t) < 0) + goto cleanup; + for (i = 0; i < nt; i++) { if (t[i].a != 10 || t[i].b != 20) { @@ -148,6 +159,9 @@ testReallocArray(const void *opaque ATTRIBUTE_UNUSED) if (VIR_REALLOC_N(t, nt - 5) < 0) goto cleanup; + if (testCheckNonNull(t) < 0) + goto cleanup; + for (i = 0; i < (nt - 5); i++) { if (t[i].a != 10 || t[i].b != 20) { @@ -180,10 +194,8 @@ testExpandArray(const void *opaque ATTRIBUTE_UNUSED) if (VIR_ALLOC_N(t, nt) < 0) return -1; - if (t == NULL) { - fprintf(stderr, "Allocation succeeded by pointer is NULL\n"); + if (testCheckNonNull(t) < 0) goto cleanup; - } for (i = 0; i < nt; i++) { t[i].a = 10; @@ -193,6 +205,9 @@ testExpandArray(const void *opaque ATTRIBUTE_UNUSED) if (VIR_EXPAND_N(t, nt, 5) < 0) goto cleanup; + if (testCheckNonNull(t) < 0) + goto cleanup; + for (i = 0; i < (nt - 5); i++) { if (t[i].a != 10 || t[i].b != 20) { @@ -211,6 +226,9 @@ testExpandArray(const void *opaque ATTRIBUTE_UNUSED) VIR_SHRINK_N(t, nt, 5); + if (testCheckNonNull(t) < 0) + goto cleanup; + for (i = 0; i < nt; i++) { if (t[i].a != 10 || t[i].b != 20) { @@ -221,6 +239,9 @@ testExpandArray(const void *opaque ATTRIBUTE_UNUSED) VIR_SHRINK_N(t, nt, 5); + if (testCheckNonNull(t) < 0) + goto cleanup; + for (i = 0; i < nt; i++) { if (t[i].a != 10 || t[i].b != 20) { @@ -255,10 +276,8 @@ testResizeArray(const void *opaque ATTRIBUTE_UNUSED) at = nt; - if (t == NULL) { - fprintf(stderr, "Allocation succeeded by pointer is NULL\n"); + if (testCheckNonNull(t) < 0) goto cleanup; - } for (i = 0; i < nt; i++) { t[i].a = 10; @@ -268,6 +287,9 @@ testResizeArray(const void *opaque ATTRIBUTE_UNUSED) if (VIR_RESIZE_N(t, at, nt, 8) < 0) goto cleanup; + if (testCheckNonNull(t) < 0) + goto cleanup; + if (at != 18) { fprintf(stderr, "Expected allocation of 16 not %zu\n", at); goto cleanup; @@ -314,10 +336,8 @@ testInsertArray(const void *opaque ATTRIBUTE_UNUSED) if (VIR_ALLOC_N(t, nt) < 0) return -1; - if (t == NULL) { - fprintf(stderr, "Allocation succeeded by pointer is NULL\n"); + if (testCheckNonNull(t) < 0) goto cleanup; - } for (i = 0; i < nt; i++) t[i] = (void*)0x50; -- Nehal J Wani
On 04/24/2014 02:25 PM, Nehal J Wani wrote:
+static int +testAllocScalar(const void *opaque ATTRIBUTE_UNUSED) +{ + testDummyStruct *t; + int ret = -1; + + if (VIR_ALLOC(t) < 0) + return -1; + + if (t == NULL) { + fprintf(stderr, "Allocation succeeded by pointer is NULL\n"); + goto cleanup; + }
Just out of curiosity, why don't we have this check after VIR_REALLOC_N, VIR_EXPAND_N, VIR_SHRINK_N and VIR_RESIZE_N ?
As this patch hasn't been pushed yet, maybe the following can be squashed in?
diff --git a/tests/viralloctest.c b/tests/viralloctest.c index abdd871..d5818c7 100644 --- a/tests/viralloctest.c +++ b/tests/viralloctest.c @@ -33,6 +33,17 @@ typedef struct testDummyStruct { } testDummyStruct;
static int +testCheckNonNull(void *t) +{ + if (t == NULL) { + fprintf(stderr, "Allocation succeeded by pointer is NULL\n");
s/by/but/ Looks reasonable to me. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Apr 24, 2014 at 04:00:35PM -0600, Eric Blake wrote:
On 04/24/2014 02:25 PM, Nehal J Wani wrote:
+static int +testAllocScalar(const void *opaque ATTRIBUTE_UNUSED) +{ + testDummyStruct *t; + int ret = -1; + + if (VIR_ALLOC(t) < 0) + return -1; + + if (t == NULL) { + fprintf(stderr, "Allocation succeeded by pointer is NULL\n"); + goto cleanup; + }
Just out of curiosity, why don't we have this check after VIR_REALLOC_N, VIR_EXPAND_N, VIR_SHRINK_N and VIR_RESIZE_N ?
As this patch hasn't been pushed yet, maybe the following can be squashed in?
diff --git a/tests/viralloctest.c b/tests/viralloctest.c index abdd871..d5818c7 100644 --- a/tests/viralloctest.c +++ b/tests/viralloctest.c @@ -33,6 +33,17 @@ typedef struct testDummyStruct { } testDummyStruct;
static int +testCheckNonNull(void *t) +{ + if (t == NULL) { + fprintf(stderr, "Allocation succeeded by pointer is NULL\n");
s/by/but/
Looks reasonable to me.
Yep, forgot I hadn't pushed this, so will squash the changes in. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 04/08/2014 04:48 PM, Daniel P. Berrange wrote:
In debugging a crash on OOM, I thought that the virInsert APIs might be at fault, but couldn't isolate them as a cause. While the viralloc APIs are used in many test suites, this is as a side-effect, they are not directly tested :-)
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tests/Makefile.am | 5 + tests/viralloctest.c | 387 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 392 insertions(+) create mode 100644 tests/viralloctest.c
+ + +static int +testAllocArray(const void *opaque ATTRIBUTE_UNUSED) +{ + testDummyStruct *t; + size_t nt = 10, i; + int ret = -1; + + if (VIR_ALLOC_N(t, nt) < 0) + return -1; + + if (t == NULL) { + fprintf(stderr, "Allocation succeeded by pointer is NULL\n");
s/by pointer/but pointer/ in the whole file
+ goto cleanup; + }
ACK Jan
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Ján Tomko -
Nehal J Wani