list: add a doubly linked list type.

[paulidale]

These list can be embedded into structures and structures can be members of
multiple lists. Moreover, this is done without dynamic memory allocation.
That is, this is legal:

    typedef struct item_st ITEM;

    struct item_st {
        ...
        OSSL_LIST_MEMBER(new_items, ITEM);
        OSSL_LIST_MEMBER(failed_items, ITEM);
        ...
    };

    DEFINE_LIST_OF(new_items, TESTL);
    DEFINE_LIST_OF(failed_items, TESTL);

    struct {
        ...
        OSSL_LIST(new_items) new;
        OSSL_LIST(failed_items) failed;
        ...
    } *st;

    ITEM *p;

    for (p = ossl_list_new_items_head(&st->new); p != NULL; p = ossl_list_new_items_next(p))
        /* do something */

• documentation is added or updated
• tests are added or updated

[paulidale]

If the size/complexity of the insertion and removal inline functions is deemed too big, they could be converted to wrappers around generic versions of the functions.

[levitte]

I find this silly. Even though we would use it primarly for QUIC, the implementation is so generic that it can be seen as such. No need to make it conditional like this.

[t8m]

Good work! Only a few copy&paste issues in the docs.

I agree with @levitte that this does not need to be disabled with quic. Especially because it is inlined so if not used by anything it won't raise the size of the libcrypto.

[paulidale]

I did ask the question about needing to inline the insert/remove functions. They could easily be made generic and out of line. The rest might as well stay inlined since they're trivial.

[t8m]

I'd keep them inline for now.

[tmshort]

Is this because we're assuming sys/queue.h isn't available everywhere?

[openssl-machine]

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

[slontis]

Doesnt this kind of break naming conventions?

[paulidale]

In what sense?

[slontis]

How badly does it fall over if the user does something wrong?
Like double removal, or addition of a element already in the list..
These conditions could be easily checked and returned as errors by checking the next/prev pointers.

[paulidale]

How badly does it fall over if the user does something wrong?

As it stands, it falls over really badly if bad stuff is done. As a low level internal data structure this is kind of acceptable. It would be possible to add extra checking in debug builds but not production ones.

How far should this go? E.g. it would be possible to automatically remove an item from a list when inserting it into another.

One programming error, which these kind of faults are, that can never be caught is uninitialised memory.

[slontis]

How badly does it fall over if the user does something wrong?

As it stands, it falls over really badly if bad stuff is done. As a low level internal data structure this is kind of acceptable. It would be possible to add extra checking in debug builds but not production ones.

How far should this go? E.g. it would be possible to automatically remove an item from a list when inserting it into another.

One programming error, which these kind of faults are, that can never be caught is uninitialised memory.

Checking if the next/prev are NULL is a fairly simple check that picks up most problems.. Yes if it is uninited then there is not much you can do.. Nice implementation by the way..

[paulidale]

next/prev NULL will SEGV and be easy to find however.

[paulidale]

The code can't check the element's next and free pointers: NULL is perfectly valid for either or both (single element list has both NULL). To do the kind of checks you're after requires keeping a current list pointer along with the next and prev pointers and checking that.

The only thing that can be checked is that the passed elements are not NULL.

[slontis]

If you insert a new node into a list - its next and prev should be NULL if it has been inited. Removing it should NULL the next and prev.

[paulidale]

If you insert a new node into a list - its next and prev should be NULL if it has been inited.

This check isn't reliable, a node in a single element list will have next and prev equal to NULL.
If this were a public facing API, I agree that more validation is required but it's internal & where we don't expect such checking.

E.g. it is perfect legal to remove a node with two NULL links from a list.

BTW: there is no init function for nodes, just for the container.

[paulidale]

I'd like this one to go in as it is.

We can figure out what manner of checking etc we want and I'll create a new PR handling that. I've raised an issue for this: #19129

[paulidale]

Is this because we're assuming sys/queue.h isn't available everywhere?

Yes, not in C89 😞

[paulidale]

Merged, thanks for the reviews and feedback.

[levitte]

Is this because we're assuming sys/queue.h isn't available everywhere?

Yes, not in C89 disappointed

Not any C language level as far as I know, and not even POSIX. It's a BSD thing.

[hlandau]

I wasn't around to review this, but this looks good to me. I actually had the same approach in mind. 👍