> dhcp: refactor state to string conversion

DHC_DEPREF6 is in the middle of NMDHCPState enum in nm-dhcp-client.h, but as last item in nm-dhcp-client.c. And now the order is important.
You should also add a comment to both header and the code that the states have to be in sync.

Else, the branch looks good to me.

(In reply to comment #1)
> > dhcp: refactor state to string conversion
> 
> DHC_DEPREF6 is in the middle of NMDHCPState enum in nm-dhcp-client.h, but as
> last item in nm-dhcp-client.c. And now the order is important.
> You should also add a comment to both header and the code that the states have
> to be in sync.

I think this is ok, because the array is set by index...

+static const char *state_table[] = {
+    [DHC_NBI]             = "nbi",

means to set the index at position [DHC_NBI], so the order does not matter... (as long as the indices are distinct....

Anyway, I adjusted the order in nm-dhcp-client.c.


Repushed (also rebased on current master + fixed spelling error in commit message + white_space error in patch).

> dhcp: refactor dhcp code to use @dhcp_anycast_addr as #GByteArray type

>+	} else
>+		g_return_val_if_fail (!anycast_addr, g_string_free (new_contents, FALSE));
> 
> 	return g_string_free (new_contents, FALSE);

That's weird. You should just

  g_return_val_if_fail (!anycast_addr || anycast_addr->len == 6, NULL);

at the top of the function.


> dhcp: refactor not to pass on NMSettingIP4Config objects

This seems fine to me, but dcbw would know if there's some reason why we might want the full configs in the future.



The branch seems weird in that it's preparing for future changes, but those changes aren't here... I guess they're somewhere else in the explosion of patches from 726177?

(In reply to comment #3)
> > dhcp: refactor dhcp code to use @dhcp_anycast_addr as #GByteArray type
> 
> >+	} else
> >+		g_return_val_if_fail (!anycast_addr, g_string_free (new_contents, FALSE));
> > 
> > 	return g_string_free (new_contents, FALSE);
> 
> That's weird. You should just
> 
>   g_return_val_if_fail (!anycast_addr || anycast_addr->len == 6, NULL);
> 
> at the top of the function.

Of course, this case should never arise, but *if* it happens to fail, g_return-ing at the top breaks the functionality, while doing it at the end simply warns critically. I prefer that for now.


> > dhcp: refactor not to pass on NMSettingIP4Config objects
> 
> This seems fine to me, but dcbw would know if there's some reason why we might
> want the full configs in the future.
> 
> 
> 
> The branch seems weird in that it's preparing for future changes, but those
> changes aren't here... I guess they're somewhere else in the explosion of
> patches from 726177?

Yes, I did these patches in the course of something else, where the starting of the client was delayed. The reason to do that is to kill the dhcp client process asynchronously. But the manager must not start another process for the same interface until the old one is reaped.

Therefore it would need to clone the setting to have the arguments around for later. Hence the change. If we later need additional parameters, it will be simple to add them on a one-by-one basis.

The change also makes it clear, which properties of the settings are actually interesting for starting dhcp.

(In reply to comment #4)
> Of course, this case should never arise, but *if* it happens to fail,
> g_return-ing at the top breaks the functionality, while doing it at the end
> simply warns critically. I prefer that for now.

If you want to return a valid result, then don't use g_return_if_fail(). Do "g_warn_if_fail (!anycast_addr || anycast_addr->len == 6)" and then keep going. Besides being semantically wrong, using g_return_if_fail() there only works because it just happens that anycast_addr is the last field to be processed. If someone adds another config field later, after your g_return_if_fail(), then that field would only get written to the config if anycast_addr was valid, which isn't what you want.


Otherwise looks good, other than a commit message nitpick:

> dhcp: refactor using a unique named define for signal names

should be "refactor using named defines for signal names". Saying "unique" makes it sound like you're using the same define for all three signals.

(In reply to comment #5)
> (In reply to comment #4)
> > Of course, this case should never arise, but *if* it happens to fail,
> > g_return-ing at the top breaks the functionality, while doing it at the end
> > simply warns critically. I prefer that for now.
> 
> If you want to return a valid result, then don't use g_return_if_fail(). Do
> "g_warn_if_fail (!anycast_addr || anycast_addr->len == 6)" and then keep going.
> Besides being semantically wrong, using g_return_if_fail() there only works
> because it just happens that anycast_addr is the last field to be processed. If
> someone adds another config field later, after your g_return_if_fail(), then
> that field would only get written to the config if anycast_addr was valid,
> which isn't what you want.


I don't want to discuss this to dead, but just to explain my view on this:

IMO g_warn_* is not very inside NM, because when under regular, non-bug conditions something happens that is worth a warning, I would prefer nm_log_warn() instead.

g_return_* on the other hand is a true assert that can be disabled. By moving it to the end of the function, the code will still work correct in case of assert-failure. Would such a bug hit the users in the wild, this particular functionality might still work -- making a bug less grave.


I rebased and added a fixup! to move the g_return_* at the end of the function. This way it is clearer, if you add another config field later.


Is that acceptable?

Merged to master as: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=542832eeb3b5fd531a102d36770ebfe963296783