Re: [PATCH 1/9] netconfig: Refactor setting new values to system

[Denis Kenzior <denkenz at gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 9804 bytes --]

Hi Andrew,

On 11/11/21 1:29 PM, Andrew Zaborowski wrote:
> On Thu, 11 Nov 2021 at 19:35, Denis Kenzior <denkenz(a)gmail.com> wrote:
>>>> I really don't think these belong here.  If we're going in the direction of
>>>> netconfig delegating the management of applying IP configuration settings, then
>>>> this is up to the applying entity to store this information.  netconfig would
>>>> only be responsible for obtaining the settings and notifying the new management
>>>> entity.
>>>>
>>>> The new management entity would either:
>>>> A) invoke the agent, or
>>>> B) apply the settings itself.
>>>>
>>>> Preferably, A & B would use the same API and be subclasses of some generic
>>>> abstract interface.  Similarly to how we handle auth_protos, or perhaps modeled
>>>> after resolv_ops.
>>>
>>> Ok, having these RTNL calls etc., in a separate file from where we
>>> handle the config methods and policies will make the code a little
>>> clearer but the API will probably need to include the optional details
>>> like the MAC addresses.
>>>
>>
>> That is fine.  Every time we logically separate pieces behind a clear API makes
>> the code more maintainable and easier to understand.  We pay a bit in code size,
>> but it is worth it in my opinion.
> 
> Every time we say "every time" it's probably wrong ;-)
> 
> I mean let's do this, but there's obviously a threshold of complexity
> where this starts to make sense.

Oh I agree.  And so far I would say that we only did such restructuring when it 
made sense.  Of course you can disagree, but I don't recall any situation where 
we did something like this and said: "that was a terrible idea."

> 
> And in general clear APIs are not generally ones that use function
> pointers, or wrap simple things like a memcpy or integer operations in
> functions -- in terms of readability you simply can't beat a language
> feature (I know you disagree, not proposing we spend any more time on
> this topic..).

This is a classic example of polymorphic behavior.  Hiding this behind a virtual 
interface + backend is about as clear as things can get ;)

> 
>>
>>>> Note that netconfig behaves differently with dhcp6 client vs dhcp.  With dhcp,
>>>> netconfig is the one setting routes and addresses.  But with dhcp6, we let the
>>>> dhcp6_client itself set the route and the address.  So this may need to be
>>>> refactored to act similarly to how dhcp client is managed.  Otherwise the
>>>> settings would be applied despite the agent being registered.
>>>
>>> I don't think that's much of an issue but let's do it, one difficulty
>>> is going to be synchronising the ell and iwd changes though.
>>
>> So I'm not sure how to interpret the statement that it isn't 'much of an issue'?
>>    The contract we have is that if the agent is registered, then iwd doesn't
>> apply any settings to the netdevs.
> 
> Well that's the proposal, not a contract that we have or one that has
> to be implemented fully from the start (there's hardly any protocol we

Eh, why not?  Especially if you want NM to use it?  Not honoring contracts is 
amateur hour.  I mean why would they even bother if we can't keep our contracts?

> implement 100% and especially in netconfig -- yet it got merged).  In
> any case there's not much that that agent can do other than the same
> thing l_dhcp6_client does.

Sure, but it has to be done, not hand-waved away.

> 
>>   But what I'm pointing out above is that: the
>> way l_dhcp6_client is being driven today, this contract is broken.  netconfig
>> delegates setting of the address and routes to l_dhcp6_client/l_icmp6_client and
>> these settings are always applied if IPv6 support is enabled / detected.
> 
> Right, the difference between l_dhcp_client and l_dhcp6_client is
> strange and didn't seem to be mentioned anywhere in the netconfig.c
> until the comment I add in this patch.

Well, we tried it one way with dhcpv4.  We tried another approach with dhcpv6. 
Arguably, the dhcpv6 approach made a whole lot more sense until this agent 
proposal came about.  Having ell / dhcp{4|6}_client be able to set settings 
directly still might make sense for certain applications...

>>>>> +             if (netconfig->addr6_add_cmd_id) {
>>>>> +                     netconfig->pending_v4_set |= changed;
>>>>> +                     return;
>>>>> +             }
>>>>> +     }
>>>>> +
>>>>> +     if (af == AF_INET) {
>>>>> +             if (changed & NETCONFIG_CHANGED_GATEWAY) {
>>>>> +                     netconfig_gateway_to_arp(netconfig);
>>>>> +
>>>>> +                     if (netconfig_ipv4_gateway_route_install(netconfig))
>>>>> +                             changed &= ~NETCONFIG_CHANGED_GATEWAY;
>>>>> +             } else if (netconfig->notify) {
>>>>
>>>> This is really quite weird since netconfig_ipv4_gateway_route_install() checks
>>>> that the gateway is NULL.  What is this trying to take care of?
>>>
>>> You mean what is the if (netconfig_ipv4_gateway_route_install) trying
>>> to take care of?  Error handling, though errors are not really
>>> expected to happen there.
>>
>> You have a special case for gateway being NULL in
>> netconfig_ipv4_gateway_route_install().  When lease is lost or cleaning up, you
>> set NETCONFIG_CHANGED_GATEWAY flag and a debug message and other special
>> processing takes place.  That seems bogus.
>>
>>>
>>>>
>>>>> +                     /*
>>>>> +                      * The gateway route addition callback notifies the user
>>>>> +                      * of netconfig success if a gateway is provided, otherwise
>>>>> +                      * notify here.
>>>>> +                      */
>>>>> +                     netconfig->notify(NETCONFIG_EVENT_CONNECTED,
>>>>> +                                             netconfig->user_data);
>>>>> +                     netconfig->notify = NULL;
>>>>> +             }
>>
>> Also, it looks like even if a lease is obtained without a gateway, the changed
>> flag is still set.  The only time a changed flag is not set is if the lease is
>> renewed and the gateway is the same.  Which makes the block above pretty suspect?
> 
> If there was no gateway and the new lease has no gateway (or the lease
> is gone), then there's no change in the gateway, right?  We don't set
> NETCONFIG_CHANGED_GATEWAY in that case.

Sure, but you still try to call the notifier saying that we're 'connected'?  How 
does that make sense?  Yes, I know the notifier is likely NULL by now.  But I 
see no circumstances where this code path would even be hit.  And if it is hit, 
it is probably doing the wrong thing anyway.  So why have it in the first place?

> 
> When the value has changed then we set NETCONFIG_CHANGED_GATEWAY.  I'm
> not sure I understand what is bogus about this.  The wording CHANGED
> is because it's literally changing (new is different than the old --
> that's how you define a change), now if this is committed at the
> system level through an erase and re-add doesn't mean that the gateway
> address hasn't changed...
> 

What you say is absolutely correct.  Yet still wrong ;)  Just look at what 
happens on a 'change'.  You try to call the notifier with 'connected' event. 
You also print a debug message saying there's no gateway in the lease.  Neither 
make sense.

>>
>>>
>>> So basically we pass a set of new values to the new
>>> "netconfig-committer" entity, it checks what's changed, calls the
>>> agent method first, if no agent is registered it falls back to the
>>> local implementation.
>>>
>>> Honestly I see no place for an "ops" structure there, it can be forced
>>> between the "entity" and the agent code but it's adding more problems
>>> than solving.
>>
>> I disagree here.  It isn't forced at all.  The new entity just selects a backend
>> to be used based on some policy.  Similar to how resolve class works.
>>
>> Right now we would have 2 backends, the agent one and our native one.  But we
>> could add more in the future.
> 
> Hypothetically we could but in the end each backend basically does the
> same simple thing in this case.
> 
> What I suspect that someone will eventually have a usecase for is
> exposing those config properties on D-Bus, in fact I think there was
> talk about this since very early on when Tim added netconfig and then
> again when I was adding P2P (I eventually added the "ConnectedIP"
> property to start with the bare minimum).

Sure.  But why do you think this is somehow at odds with the proposed architecture?

> 
>>   The change tracking wouldn't need to be in the
>> entity itself, but the backend.  This is because the agent based backend has no
>> need to track changes at all.
> 
> Well we can send all of the parameters to the agent every time, that
> will work too.

We could.

> 
>>>> Perhaps lease expiration, and thus the clearing out of all settings should be a
>>>> separate operation?  There's not much point setting domains, dns, etc in this case.
>>>
>>> Special-casing it won't make the code clearer but ok.
>>
>> Again, I disagree completely.  There isn't much point to set empty domains or
>> DNS info and cause unnecessary D-Bus traffic.  We do have resolve_revert for a
>> reason.
> 
> We don't set them when they're empty, but we make the "committer" part
> aware that the value has changed (is different than it was).
> 

We have to see.  I think an explicit flag stating that v4/v6 info is invalidated 
would be a nice hint to the backend to simply invoke RevertLink or whatever. 
Given this, I'd also generate events and give both ipv4 and ipv6 change sets in 
one operation.  Even if only ipv4 or ipv6 changes were made.

Regards,
-Denis