Looks like a very nice improvement in general.

> d040aebf rdisc: emit config-changed signal by ID and not by name

The move of the (changed) conditionals seems to harm readability; I'd prefer if 
you didn't do that (fine with using the signal names though).

+static void
+_emit_config_change (NMRDisc *self, NMRDiscConfigMap changed)
+{
+       if (changed)
+               g_signal_emit (self, signals[CONFIG_CHANGED], 0, (int) changed);
+}

-       if (changed)
-               g_signal_emit_by_name (rdisc, NM_RDISC_CONFIG_CHANGED, NM_RDISC_CONFIG_ADDRESSES);
+       _emit_config_change (rdisc, NM_RDISC_CONFIG_ADDRESSES);

Here it even changes behavior; and using a numeric value of a NMRDiscConfigMap 
in a conditional looks like a poor style to me.

> 69eedf44 rdisc: hide internal fields from NMRDisc API

Honestly, this looks worse to me with the NMRDiscData split off :(

(In reply to Lubomir Rintel from comment #1)
> Looks like a very nice improvement in general.
> 
> > d040aebf rdisc: emit config-changed signal by ID and not by name
> 
> The move of the (changed) conditionals seems to harm readability; I'd prefer
> if 
> you didn't do that (fine with using the signal names though).
> 
> +static void
> +_emit_config_change (NMRDisc *self, NMRDiscConfigMap changed)
> +{
> +       if (changed)
> +               g_signal_emit (self, signals[CONFIG_CHANGED], 0, (int)
> changed);
> +}
> 
> -       if (changed)
> -               g_signal_emit_by_name (rdisc, NM_RDISC_CONFIG_CHANGED,
> NM_RDISC_CONFIG_ADDRESSES);
> +       _emit_config_change (rdisc, NM_RDISC_CONFIG_ADDRESSES);
> 
> Here it even changes behavior; and using a numeric value of a
> NMRDiscConfigMap 
> in a conditional looks like a poor style to me.
> 
> > 69eedf44 rdisc: hide internal fields from NMRDisc API

Yeah, this was wrong. Fixed.


> Honestly, this looks worse to me with the NMRDiscData split off :(

The idea is, that the RA data is not part of NMRDisc instead, instead it is separate data: the NMRDiscData argument in the "config-changed" signal.

Also, NMRDiscData is immuable and presented to the user of the API in a cleaner way (with proper C types).

Also, subclass NMLndpRDisc can access NMRDiscDataInternal (defined in nm-rdisc-private.h). Thus, the mutable data is hidden and only accessible from within rdisc implementation.


The overhead of this is:
 - 130 LOC
   - note that test-rdisc-fake.c alone grew by 30 lines, because it has
     more assertions.
   - I gained some line by the "guint changed_int" change.
 - one additional call to _data_complete().

> disc: move public fields from NMRDisc to NMRDiscPrivate

+const char *
+nm_rdisc_get_ifname (NMRDisc *self)
+{
+       g_return_val_if_fail (NM_IS_RDISC (self), 0);

Nitpick:
g_return_val_if_fail (NM_IS_RDISC (self), NULL);


LGTM

Thanks!

merged: 
https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=79c48a559f6e24820bb51b1328b9d2ac85214637