Re: [PATCH 2/2] tootls/tests: introduce unit tests for rangesets

[Jan Beulich <jbeulich@xxxxxxxx>]

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.pau@xxxxxxxxxx>

It's okay as is, so:
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
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