On Wed, May 21, 2025 at 07:19:19PM +0200, Phil Sutter wrote:
> On Wed, May 21, 2025 at 04:17:49PM +0200, Pablo Neira Ayuso wrote:
> > On Wed, May 21, 2025 at 03:12:41PM +0200, Phil Sutter wrote:
> > > Print an error message and try to deserialize the remaining elements
> > > instead of calling BUG().
> > >
> > > Signed-off-by: Phil Sutter <phil@xxxxxx>
> > > ---
> > > src/netlink.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/netlink.c b/src/netlink.c
> > > index 1222919458bae..3221d9f8ffc93 100644
> > > --- a/src/netlink.c
> > > +++ b/src/netlink.c
> > > @@ -1475,7 +1475,9 @@ int netlink_delinearize_setelem(struct netlink_ctx *ctx,
> > > key->byteorder = set->key->byteorder;
> > > key->len = set->key->len;
> > > } else {
> > > - BUG("Unexpected set element with no key\n");
> > > + netlink_io_error(ctx, NULL,
> > > + "Unexpected set element with no key\n");
> > > + return 0;
> >
> > If set element has no key, then something is very wrong. There is
> > already one exception that is the catch-all element (which has no
> > key).
>
> Yes, in these cases the ruleset parser fails and the output is very
> likely broken or at least incomplete. This series merely aligns error
> handling: Take netlink_parse_cmp() for instance: If NFTNL_EXPR_CMP_SREG
> attribute is missing or bogus, netlink_error() is called and the
> function returns (void). No error status propagation happens (which we
> could change easily), but most importantly the parser continues to
> deserialize as much as possible.
>
> > This is enqueuing an error record, but 0 is returned, I am not sure if
> > this is ever going to be printed.
>
> It does: Forcing the code to enter that third branch, listing a set with three
> elements looks like this:
>
> % sudo ./src/nft list ruleset
> table ip t {
> set s {
> type inet_service
> }
> }
> netlink: Error: Unexpected set element with no key
>
> netlink: Error: Unexpected set element with no key
>
> netlink: Error: Unexpected set element with no key
>
> > I am not sure this patch works.
>
> Well, that extra newline is indeed a bug. :)
Go ahead push it out then.
