On 11.04.2025 09:55, Roger Pau Monne wrote:
> Introduce some basic infrastructure for doing rangeset unit tests, and add
> a few tests that ensure correctness of rangeset subtraction.
>
> Signed-off-by: Roger Pau Monné <roger....@citrix.com>
It's okay as is, so:
Reviewed-by: Jan Beulich <jbeul...@suse.com>
Nevertheless a couple of remarks.
> --- /dev/null
> +++ b/tools/tests/rangeset/Makefile
> @@ -0,0 +1,45 @@
> +XEN_ROOT=$(CURDIR)/../../..
> +include $(XEN_ROOT)/tools/Rules.mk
> +
> +TARGET := test-rangeset
> +
> +.PHONY: all
> +all: $(TARGET)
> +
> +.PHONY: run
> +run: $(TARGET)
> + ./$<
> +
> +.PHONY: clean
> +clean:
> + $(RM) -- *.o $(TARGET) $(DEPS_RM)
> +
> +.PHONY: distclean
> +distclean: clean
> + $(RM) -- *~
> +
> +.PHONY: install
> +install: all
> + $(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
> + $(INSTALL_PROG) $(TARGET) $(DESTDIR)$(LIBEXEC_BIN)
> +
> +.PHONY: uninstall
> +uninstall:
> + $(RM) -- $(addprefix $(DESTDIR)$(LIBEXEC_BIN)/,$(TARGET))
> +
> +list.h: $(XEN_ROOT)/xen/include/xen/list.h
> +rangeset.h: $(XEN_ROOT)/xen/include/xen/rangeset.h
> +list.h rangeset.h:
> + sed -e '/#include/d' <$< >$@
> +
> +rangeset.c: $(XEN_ROOT)/xen/common/rangeset.c list.h rangeset.h
> + # Remove includes and add the test harness header
> + sed -e '/#include/d' -e '1s/^/#include "harness.h"/' <$< >$@
> +
> +CFLAGS += -D__XEN_TOOLS__
> +CFLAGS += $(APPEND_CFLAGS)
> +CFLAGS += $(CFLAGS_xeninclude)
> +LDFLAGS += $(APPEND_LDFLAGS)
The mix of padding ahead of the += is a little odd here.
> --- /dev/null
> +++ b/tools/tests/rangeset/harness.h
> @@ -0,0 +1,71 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Unit tests for rangesets.
> + *
> + * Copyright (C) 2025 Cloud Software Group
> + */
> +
> +#ifndef _TEST_HARNESS_
> +#define _TEST_HARNESS_
> +
> +#include <assert.h>
> +#include <errno.h>
> +#include <stdbool.h>
> +#include <stddef.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#include <xen-tools/common-macros.h>
> +
> +#define smp_wmb()
> +#define __must_check __attribute__((__warn_unused_result__))
> +#define cf_check
> +
> +#define BUG_ON(x) assert(!(x))
> +#define ASSERT(x) assert(x)
> +
> +#include "list.h"
> +#include "rangeset.h"
> +
> +typedef bool rwlock_t;
> +typedef bool spinlock_t;
Are spinlocks really required for the rangeset code?
> --- /dev/null
> +++ b/tools/tests/rangeset/test-rangeset.c
> @@ -0,0 +1,228 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Unit tests for rangesets.
> + *
> + * Copyright (C) 2025 Cloud Software Group
> + */
> +
> +#include "harness.h"
> +
> +struct range {
> + unsigned long start, end;
> +};
> +
> +struct action {
> + enum {
> + ADD,
> + REMOVE,
> + } action;
> + struct range r;
> +};
> +
> +#define DECLARE_ACTIONS(nr) static const struct action actions ## nr []
> +#define DECLARE_RESULTS(nr) static const struct range results ## nr []
> +
> +/*
> + * Subtract range with tail overlap on existing range:
> + *
> + * { 0, 1, 4, 5 } - { 3, 4 } = { 0, 1, 5, 5 }
> + */
> +DECLARE_ACTIONS(0) = {
> + { ADD, { 0, 1 } },
> + { ADD, { 4, 5 } },
> + { REMOVE, { 3, 4 } },
> +};
> +DECLARE_RESULTS(0) = {
> + { 0, 1 }, { 5, 5 },
> +};
> +
> +/*
> + * Subtract range with complete and tail overlap on existing ranges:
> + *
> + * { 0, 1, 4, 5, 7, 8 } - { 3, 4, 5, 6, 7 } = { 0, 1, 8 }
> + */
> +DECLARE_ACTIONS(1) = {
> + { ADD, { 0, 1 } },
> + { ADD, { 4, 5 } },
> + { ADD, { 7, 8 } },
> + { REMOVE, { 3, 7 } },
> +};
> +DECLARE_RESULTS(1) = {
> + { 0, 1 }, { 8, 8 },
> +};
> +
> +/*
> + * Subtract range with no overlap:
> + *
> + * { 0, 1, 4, 5 } - { 2, 3 } = { 0, 1, 4, 5 }
> + */
> +DECLARE_ACTIONS(2) = {
> + { ADD, { 0, 1 } },
> + { ADD, { 4, 5 } },
> + { REMOVE, { 2, 3 } },
> +};
> +DECLARE_RESULTS(2) = {
> + { 0, 1 }, { 4, 5 },
> +};
> +
> +/*
> + * Subtract range with partial overlap on two existing ranges:
> + *
> + * { 0, 1, 4, 5 } - { 1, 4 } = { 0, 5 }
> + */
> +DECLARE_ACTIONS(3) = {
> + { ADD, { 0, 1 } },
> + { ADD, { 4, 5 } },
> + { REMOVE, { 1, 4 } },
> +};
> +DECLARE_RESULTS(3) = {
> + { 0, 0 }, { 5, 5 },
> +};
> +
> +static const struct test {
> + unsigned int nr_actions, nr_results;
> + const struct action *actions;
> + const struct range *result;
> +} tests[] = {
> +#define DECLARE_TEST(nr) \
> + { \
> + .actions = actions ## nr, \
> + .nr_actions = ARRAY_SIZE(actions ## nr), \
> + .result = results ## nr, \
> + .nr_results = ARRAY_SIZE(results ## nr), \
> + }
> +
> + DECLARE_TEST(0),
> + DECLARE_TEST(1),
> + DECLARE_TEST(2),
> + DECLARE_TEST(3),
> +
> +#undef DECLARE_TEST
> +};
> +
> +static int print_range(unsigned long s, unsigned long e, void *data)
> +{
> + printf("[%ld, %ld]\n", s, e);
> +
> + return 0;
> +}
> +
> +static int count_ranges(unsigned long s, unsigned long e, void *data)
> +{
> + unsigned int *nr = data;
> +
> + ++*nr;
> + return 0;
> +}
> +
> +const struct range *expected;
static?
Jan
