Re: [PATCH 10/14] (RFC-only) config: finish config_fn_t refactor

From: Jonathan Tan <jonathantanmy@google.com>
Date: May 11, 2023, 16:21 UTC
Message-ID: <20230511162149.897554-1-jonathantanmy@google.com>
In-Reply-To: <kl6ly1lxyvgr.fsf@chooglen-macbookpro.roam.corp.google.com>

Glen Choo <chooglen@google.com> writes:

> I've covered most your response to Ævar upthread, so I'll omit that.

Thanks. Indeed, I missed the situation in which a caller used kvi not by accessing its fields directly but by passing kvi to a function in config.c.

Show 9 quoted lines

> In an off-list discussion, we described some plausible ways to organize
> the refactor that would make it easier for a reviewer to confirm safety.
> 
> I haven't tried that yet because it sounds like you'd prefer the
> sidestepping approach. Do you prefer that primarily for safety reasons,
> or is it largely motivated by other concerns too (e.g. reducing churn or
> sidestepping produces a better API)? If the primary concern is just
> safety, I'm somewhat confident that we can find some way to organize
> this that makes it easier to review and I should just do it.

My preference for the sidestepping approach was to reduce churn, but as you have pointed out, it doesn't actually reduce churn. So now I think that the patches should be reordered (but am open to being convinced otherwise, of course).

As for safety and better API, I think both approaches (bulk modification of all functions to take the new config_fn_t and two sets of functions each taking a different function type) are equally safe, and it is bulk modification that results in a better API (as you've demonstrated, having kvi information is needed for good error messages, and I expect that to be more and more needed).