Re: [PATCH v6 01/15] lib: Add TLV parser

["Thomas Weißschuh" <linux@weissschuh.net>]

On 2025-01-21 15:55:28+0100, Roberto Sassu wrote:
> On Tue, 2025-01-21 at 14:29 +0100, Thomas Weißschuh wrote:
> > On 2024-11-19 11:49:08+0100, Roberto Sassu wrote:

[..]

> > > +typedef int (*callback)(void *callback_data, __u16 field,
> > > +			const __u8 *field_data, __u32 field_len);
> > 
> > No need for __underscore types in kernel-only signatures.
> 
> It is just for convenience. I'm reusing the same file for the userspace
> counterpart digest-cache-tools. In that case, the parser is used for
> example to show the content of the digest list.

This reuse leads to quite some ugly constructs.
Given that the single function will be really simple after removing the
unnecessary parts, maybe two clean copies are easier.
One copy is needed for Frama-C anyways.

> > > +
> > > +int tlv_parse(callback callback, void *callback_data, const __u8 *data,
> > > +	      size_t data_len, const char **fields, __u32 num_fields);
> > > +
> > > +#endif /* _LINUX_TLV_PARSER_H */
> > > diff --git a/include/uapi/linux/tlv_parser.h b/include/uapi/linux/tlv_parser.h
> > > new file mode 100644
> > > index 000000000000..171d0cfd2c4c
> > > --- /dev/null
> > > +++ b/include/uapi/linux/tlv_parser.h
> > > @@ -0,0 +1,41 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > > +/*
> > > + * Copyright (C) 2023-2024 Huawei Technologies Duesseldorf GmbH
> > > + *
> > > + * Author: Roberto Sassu <roberto.sassu@huawei.com>
> > > + *
> > > + * Implement the user space interface for the TLV parser.
> > > + */
> > 
> > Can you explain in the commit message where this will be exposed to
> > userspace as binary?
> 
> I see that my explanation is not ideal.
> 
> This is the format for data exchange between user space and kernel
> space, but it is still the kernel that reads and parses the TLV-
> formatted file for extracting the digests and adding them to the digest
> cache.

I figured that out :-)
It should be clear from the commit itself, though.

> > > +
> > > +#ifndef _UAPI_LINUX_TLV_PARSER_H
> > > +#define _UAPI_LINUX_TLV_PARSER_H
> > > +
> > > +#include <linux/types.h>
> > > +
> > > +/*
> > > + * TLV format:
> > > + *
> > > + * +--------------+--+---------+--------+---------+
> > > + * | field1 (u16) | len1 (u32) | value1 (u8 len1) |
> > > + * +--------------+------------+------------------+
> > > + * |     ...      |    ...     |        ...       |
> > > + * +--------------+------------+------------------+
> > > + * | fieldN (u16) | lenN (u32) | valueN (u8 lenN) |
> > > + * +--------------+------------+------------------+
> > > + */
> > > +
> > > +/**
> > > + * struct tlv_entry - Entry of TLV format
> > > + * @field: Field identifier
> > > + * @length: Data length
> > > + * @data: Data
> > > + *
> > > + * This structure represents an entry of the TLV format.
> > > + */
> > > +struct tlv_entry {
> > > +	__u16 field;
> > > +	__u32 length;

Looking at this again, the "length" field is unaligned by default.

Also FYI there is already a TLV implementation in
include/uapi/linux/tipc_config.

> > > +} __attribute__((packed));

[..]

> > Some kunit tests would be great.
> 
> I implemented kselftests also injecting errors (patch 13). If it is not
> enough, I implement kunit tests too.

These selftests are for the digest_cache.
If the TLV library is meant to be used alone, some dedicated tests would
be nice. kunit has the advantage that it can directly call kernel
functions with arbitrary parameters and does not require any userspace
setup.