netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Johannes Berg <johannes@sipsolutions.net>
To: Jiri Benc <jbenc@redhat.com>
Cc: netdev@vger.kernel.org
Subject: Re: [RFC 4/5] netlink: prepare validate extack setting for recursion
Date: Wed, 19 Sep 2018 11:44:59 +0200	[thread overview]
Message-ID: <1537350299.10305.33.camel@sipsolutions.net> (raw)
In-Reply-To: <20180919112829.647896cf@redhat.com>

On Wed, 2018-09-19 at 11:28 +0200, Jiri Benc wrote:

> > It might be possible to do this differently, in theory, but all the ways
> > I've tried to come up with so far made the code vastly more complex.
> 
> Wouldn't still make sense to store the flag in the struct
> netlink_ext_ack, though?

Does it make sense to store a flag there that only has a single,
localized, user? I'd say no.

> The way the parameters are passed around in
> this patch looks ugly. 

Yeah, it's not the best in some ways.

I considered having a cleared new extack on the stack in the outer-most
call (nla_parse/nla_validate), and then we can "set if not already set",
and at the end unconditionally copy/set to the real one... But then I
either need to duplicate that code in both, or pass another argument to
nla_validate_parse() anyway to indicate whether to do this or not (since
the inner calls to it shouldn't, since that would defeat the purpose),
or add another level of function call indirection?

I'm not really sure any of these are better ... and more complex, in
some ways, since we have to copy all the data around.

> And adding more users of the flag later (there
> may be other cases when we want the earlier messages to be preserved)
> would mean adding parameters all around, while the flag in the struct
> would be readily available.

I can't really think of any other users of such a thing?

johannes

  reply	other threads:[~2018-09-19 15:22 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-18 13:12 [RFC 1/5] netlink: remove NLA_NESTED_COMPAT Johannes Berg
2018-09-18 13:12 ` [RFC 2/5] netlink: set extack error message in nla_validate() Johannes Berg
2018-09-18 17:18   ` David Ahern
2018-09-18 17:36     ` Johannes Berg
2018-09-18 13:12 ` [RFC 3/5] netlink: combine validate/parse functions Johannes Berg
2018-09-18 13:12 ` [RFC 4/5] netlink: prepare validate extack setting for recursion Johannes Berg
2018-09-19  3:37   ` Marcelo Ricardo Leitner
2018-09-19  9:25     ` Johannes Berg
2018-09-19  9:44       ` Jiri Benc
2018-09-19 18:46       ` Marcelo Ricardo Leitner
2018-09-19 19:19         ` Johannes Berg
2018-09-19 21:10           ` Marcelo Ricardo Leitner
2018-09-20  8:14             ` Johannes Berg
2018-09-20 17:48               ` Marcelo Ricardo Leitner
2018-09-19  9:10   ` Jiri Benc
2018-09-19  9:15     ` Johannes Berg
2018-09-19  9:28       ` Jiri Benc
2018-09-19  9:44         ` Johannes Berg [this message]
2018-09-18 13:12 ` [RFC 5/5] netlink: allow NLA_NESTED to specify nested policy to validate Johannes Berg
2018-09-18 17:18 ` [RFC 1/5] netlink: remove NLA_NESTED_COMPAT David Ahern

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1537350299.10305.33.camel@sipsolutions.net \
    --to=johannes@sipsolutions.net \
    --cc=jbenc@redhat.com \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).