>> build: move systemd DHCP files to a common directory

You said you moved them in branch "systemd-dhcp" too. It makes sense to do that first, and merge that into master so that git tracks this change properly (just a reminder not to forget before merging).


   src/{dhcp-manager/systemd-dhcp/src => systemd}/

I would rename differently and keep the files under "src":

   src/{dhcp-manager/systemd-dhcp => systemd}/src



>> autoip4: import systemd IPv4LL code

I think this import should happen on systemd-dhcp branch (we should now rename systemd-dhcp branch).
Also, only re-importing parts of the files seems wrong to me. I'd rather take all files from the same sd-commit.


I mean:

  git checkout systemd-dhcp
  do-stuff-4488288 (build: move systemd DHCP files to a common directory)
  do-stuff-0696eca (autoip4: import systemd IPv4LL code)
  merge systemd-dhcp into master


>> autoip4: use internal implementation

I wouldn't include systemd-headers in nm-device.c. There should be a (simple) nm-sd-avahi.h header that exposes the systemd functionality. Or maybe it should be called "nm-aipd.h", the fact that it reuses systemd code should be hidden.



>> device: replace occurrences of 'aipd' with 'autoip4'

is this a trivial renaming? Then the commit message should mention that. And possibly that simple commit should be the very first.

(In reply to Thomas Haller from comment #1)
> >> build: move systemd DHCP files to a common directory
> 
> You said you moved them in branch "systemd-dhcp" too. It makes sense to do
> that first, and merge that into master so that git tracks this change
> properly (just a reminder not to forget before merging).
> 
> 
>    src/{dhcp-manager/systemd-dhcp/src => systemd}/
> 
> I would rename differently and keep the files under "src":
> 
>    src/{dhcp-manager/systemd-dhcp => systemd}/src

Ok. BTW, I forgot to mention the branch, it's bg/wip/ipv4ll-systemd.

> 
> >> autoip4: import systemd IPv4LL code
> 
> I think this import should happen on systemd-dhcp branch (we should now
> rename systemd-dhcp branch).
> Also, only re-importing parts of the files seems wrong to me. I'd rather
> take all files from the same sd-commit.
> 
> 
> I mean:
> 
>   git checkout systemd-dhcp
>   do-stuff-4488288 (build: move systemd DHCP files to a common directory)
>   do-stuff-0696eca (autoip4: import systemd IPv4LL code)
>   merge systemd-dhcp into master

At this point we need a commit on master that updates src/Makefile.am with
the new paths, but this implies that the merge commit would not build.
Isn't it?

> 
> >> autoip4: use internal implementation
> 
> I wouldn't include systemd-headers in nm-device.c. There should be a
> (simple) nm-sd-avahi.h header that exposes the systemd functionality. Or
> maybe it should be called "nm-aipd.h", the fact that it reuses systemd code
> should be hidden.

My only doubt is that this new header file would be an exact copy of sd-ipv4ll.h,
since this file already exposes only "public" APIs. So I'm wondering if it's
worth duplicating it.
 
> 
> >> device: replace occurrences of 'aipd' with 'autoip4'
> 
> is this a trivial renaming? Then the commit message should mention that. And
> possibly that simple commit should be the very first.

Yes, it's only a renaming; I will move it to the top.

(In reply to Beniamino Galvani from comment #2)
> (In reply to Thomas Haller from comment #1)
> > >> autoip4: import systemd IPv4LL code
> > 
> > I think this import should happen on systemd-dhcp branch (we should now
> > rename systemd-dhcp branch).
> > Also, only re-importing parts of the files seems wrong to me. I'd rather
> > take all files from the same sd-commit.
> > 
> > 
> > I mean:
> > 
> >   git checkout systemd-dhcp
> >   do-stuff-4488288 (build: move systemd DHCP files to a common directory)
> >   do-stuff-0696eca (autoip4: import systemd IPv4LL code)
> >   merge systemd-dhcp into master
> 
> At this point we need a commit on master that updates src/Makefile.am with
> the new paths, but this implies that the merge commit would not build.
> Isn't it?

hm, yes. Either:

 (a) fix it in the merge commit (downside: the merge commit does more work then 
     plain merging)
 (b) fix it in a follow-up (downside: merge commit doesn't compile)
 (c) merge master into systemd-integration branch, and merge that back to
     master. Like `gitk 4e07f6117391ee4d112f8035039ef237c160e828`
     (downside: more work, upside: all commits on `git log --first-parent` 
      build)

I'd just go with (a) because you only need to fix src/Makefile.am which is not part of the modified files.

(In reply to Thomas Haller from comment #3)
> (In reply to Beniamino Galvani from comment #2)
> > At this point we need a commit on master that updates src/Makefile.am with
> > the new paths, but this implies that the merge commit would not build.
> > Isn't it?
> 
> hm, yes. Either:
> 
>  (a) fix it in the merge commit (downside: the merge commit does more work
> then 
>      plain merging)
>  (b) fix it in a follow-up (downside: merge commit doesn't compile)
>  (c) merge master into systemd-integration branch, and merge that back to
>      master. Like `gitk 4e07f6117391ee4d112f8035039ef237c160e828`
>      (downside: more work, upside: all commits on `git log --first-parent` 
>       build)
> 
> I'd just go with (a) because you only need to fix src/Makefile.am which is
> not part of the modified files.

I fixed the Makefile in the merge commit and pushed to branch bg/ipv4ll-bgo747463

Urg, the merge history is starting to get a lot more complicated, so it's harder to see what's happening in tools like gitk :(  Anything we can do to make the gitk view simpler would be great...  But anyway:

> device/trivial: rename 'aipd' to 'autoip4'

I would use "ipv4ll" to match the IPv6 stuff.

> autoip4: use internal implementation

Some devices won't have an L2 address, and some might have an L2 address that's larger than ETH_ALEN.  So we have to handle both those cases in autoip4_start() and goto fail.

I'd also put some specific debugging on each of the systemd calls that fails, so we can easily tell which one failed instead of a generic message.

Next up, we should use the "ip_ifindex" instead of the ifindex of the device, so that layered devices (ppp0, ADSL NAS, WWAN, etc) would work.  Ok yeah, they shouldn't be doing IPv4LL, but we might as well make the attempt to try.  Calling nm_device_get_ip_ifindex() will do the right thing and return either the ifindex or the ip_ifindex when needed.  It seems we also don't have a way to get the MAC address of the IP interface though, so I guess we should call nm_platform_link_get_address(nm_device_get_ip_ifindex()) to get the MAC perhaps?  As a bonus that doesn't need conversion from string -> binary either :)

Also, and I think the old implementation was incorrect here too, but in nm_device_handle_autoip4_event() I think instead of failing the entire activation, we should just do something like this:

	priv->ip4_state = IP_FAIL;
	nm_device_check_ip_failed (self, FALSE);

so that if IPv6 is still allowed to work, we don't kill the entire connection.  We should eventually do that in the DHCP code too, but not quite yet.

Looks good overall, and a great cleanup.  Thanks!

(In reply to Dan Williams from comment #5)
> Urg, the merge history is starting to get a lot more complicated, so it's
> harder to see what's happening in tools like gitk :(  Anything we can do to
> make the gitk view simpler would be great...  But anyway:
> 
We prefer to rebase branches to make history as clean/linear as possible. When merging some topic branch to master it should be first rebased to master and then merged to master with
git merge --no-ff topic-branch

Beniamino, I think you should put your changes on top of systemd-dhcp (if you want to use the changes there). This could be done like this:
git checkout systemd-dhcp
git checkout -b new-branch
git cherry-pick <commits to put on the branch>

(In reply to Jiri Klimes from comment #6)
> (In reply to Dan Williams from comment #5)
> > Urg, the merge history is starting to get a lot more complicated, so it's
> > harder to see what's happening in tools like gitk :(  Anything we can do to
> > make the gitk view simpler would be great...  But anyway:
> > 
> We prefer to rebase branches to make history as clean/linear as possible.
> When merging some topic branch to master it should be first rebased to
> master and then merged to master with
> git merge --no-ff topic-branch
> 
> Beniamino, I think you should put your changes on top of systemd-dhcp (if
> you want to use the changes there). This could be done like this:
> git checkout systemd-dhcp
> git checkout -b new-branch
> git cherry-pick <commits to put on the branch>

We always rebase and merge --no-ff for our new branches to keep the history in master linear.

But doing a real git-merge for systemd-code makes actually sense, because there are the differences between
  git diffs HEAD ddb5112 -- src/dhcp-manager/systemd-dhcp/src/
by repeatadly merging systemd-dhcp into master, these conflicts are resolved automatically.


IMO having our last-import from systemd without modifications (on systemd-dhcp branch) has advantages. Namely, that git helps you merging updates, and that you see what are our differences (above).



I would split the "move files" and "update to new upstream snapshot" two separate parts, but anyway.

(In reply to Dan Williams from comment #5)
> 
> I would use "ipv4ll" to match the IPv6 stuff.

Ok.

> 
> > autoip4: use internal implementation
> 
> Some devices won't have an L2 address, and some might have an L2 address
> that's larger than ETH_ALEN.  So we have to handle both those cases in
> autoip4_start() and goto fail.
> 
> I'd also put some specific debugging on each of the systemd calls that
> fails, so we can easily tell which one failed instead of a generic message.
> 
> Next up, we should use the "ip_ifindex" instead of the ifindex of the
> device, so that layered devices (ppp0, ADSL NAS, WWAN, etc) would work.  Ok
> yeah, they shouldn't be doing IPv4LL, but we might as well make the attempt
> to try.  Calling nm_device_get_ip_ifindex() will do the right thing and
> return either the ifindex or the ip_ifindex when needed.  It seems we also
> don't have a way to get the MAC address of the IP interface though, so I
> guess we should call
> nm_platform_link_get_address(nm_device_get_ip_ifindex()) to get the MAC
> perhaps?  As a bonus that doesn't need conversion from string -> binary
> either :)

Yes, this seems the correct way.

> Also, and I think the old implementation was incorrect here too, but in
> nm_device_handle_autoip4_event() I think instead of failing the entire
> activation, we should just do something like this:
> 
> 	priv->ip4_state = IP_FAIL;
> 	nm_device_check_ip_failed (self, FALSE);
> 
> so that if IPv6 is still allowed to work, we don't kill the entire
> connection.  We should eventually do that in the DHCP code too, but not
> quite yet.

Right, I added commit "device: fix device state transition after IPv4LL failure" to address this.

(In reply to Thomas Haller from comment #7)
> (In reply to Jiri Klimes from comment #6)
> > Beniamino, I think you should put your changes on top of systemd-dhcp (if
> > you want to use the changes there). This could be done like this:
> > git checkout systemd-dhcp
> > git checkout -b new-branch
> > git cherry-pick <commits to put on the branch>
> 
> We always rebase and merge --no-ff for our new branches to keep the history
> in master linear.
> 
> But doing a real git-merge for systemd-code makes actually sense, because
> there are the differences between
>   git diffs HEAD ddb5112 -- src/dhcp-manager/systemd-dhcp/src/
> by repeatadly merging systemd-dhcp into master, these conflicts are resolved
> automatically.
> 
> IMO having our last-import from systemd without modifications (on
> systemd-dhcp branch) has advantages. Namely, that git helps you merging
> updates, and that you see what are our differences (above).
> 
> I would split the "move files" and "update to new upstream snapshot" two
> separate parts, but anyway.

So the integration of IPv4LL files from systemd would consist in:

 - systemd-dhcp branch: move systemd files to src/systemd
 - merge systemd-dhcp branch into master and update Makefile in the merge commit

 - systemd-dhcp branch: update systemd code and import ipv4ll files
 - merge systemd-dhcp branch into master

Then the IP4LL integration can be implemented in a topic branch and merged into
master. Is this ok?

(In reply to Beniamino Galvani from comment #9)
> (In reply to Thomas Haller from comment #7)
> > (In reply to Jiri Klimes from comment #6)
> > > Beniamino, I think you should put your changes on top of systemd-dhcp (if
> > > you want to use the changes there). This could be done like this:
> > > git checkout systemd-dhcp
> > > git checkout -b new-branch
> > > git cherry-pick <commits to put on the branch>
> > 
> > We always rebase and merge --no-ff for our new branches to keep the history
> > in master linear.
> > 
> > But doing a real git-merge for systemd-code makes actually sense, because
> > there are the differences between
> >   git diffs HEAD ddb5112 -- src/dhcp-manager/systemd-dhcp/src/
> > by repeatadly merging systemd-dhcp into master, these conflicts are resolved
> > automatically.
> > 
> > IMO having our last-import from systemd without modifications (on
> > systemd-dhcp branch) has advantages. Namely, that git helps you merging
> > updates, and that you see what are our differences (above).
> > 
> > I would split the "move files" and "update to new upstream snapshot" two
> > separate parts, but anyway.
> 
> So the integration of IPv4LL files from systemd would consist in:
> 
>  - systemd-dhcp branch: move systemd files to src/systemd
>  - merge systemd-dhcp branch into master and update Makefile in the merge
> commit
> 
>  - systemd-dhcp branch: update systemd code and import ipv4ll files
>  - merge systemd-dhcp branch into master
> 
> Then the IP4LL integration can be implemented in a topic branch and merged
> into
> master. Is this ok?

Exactly.

The branch can from now be named "systemd" (or similar), no longer "-dhcp".
The only actual difference is the commit message that should not read:
 >> dhcp: merge branch 'systemd-dhcp' into master
but something like:
 >> systemd: merge branch 'systemd' into master

> ipv4ll: use internal implementation

+	ifindex = nm_device_get_ip_ifindex (self);
+	mac = nm_platform_link_get_address (ifindex, &mac_len);
+	if (!mac || mac_len != ETH_ALEN) {
+		_LOGE (LOGD_AUTOIP4, "IPv4LL: can't retrieve hardware address");
+		goto fail;
+	}
 
+	memcpy (&addr, mac, ETH_ALEN);

^^^ doesn't this need to be nm_utils_hwaddr_aton()?  'mac' is a string but sd_ipv4ll wants a u8[6].

+	r = sd_ipv4ll_set_mac (priv->ipv4ll, &addr);
+	if (r < 0) {

The rest looks good to me!

(In reply to Dan Williams from comment #11)
> +	mac = nm_platform_link_get_address (ifindex, &mac_len);
> +	if (!mac || mac_len != ETH_ALEN) {
> +		_LOGE (LOGD_AUTOIP4, "IPv4LL: can't retrieve hardware address");
> +		goto fail;
> +	}
>  
> +	memcpy (&addr, mac, ETH_ALEN);
> 
> ^^^ doesn't this need to be nm_utils_hwaddr_aton()?  'mac' is a string but
> sd_ipv4ll wants a u8[6].

The output of nm_platform_link_get_address() is already in binary form.

(In reply to Beniamino Galvani from comment #12)
> (In reply to Dan Williams from comment #11)
> > +	mac = nm_platform_link_get_address (ifindex, &mac_len);
> > +	if (!mac || mac_len != ETH_ALEN) {
> > +		_LOGE (LOGD_AUTOIP4, "IPv4LL: can't retrieve hardware address");
> > +		goto fail;
> > +	}
> >  
> > +	memcpy (&addr, mac, ETH_ALEN);
> > 
> > ^^^ doesn't this need to be nm_utils_hwaddr_aton()?  'mac' is a string but
> > sd_ipv4ll wants a u8[6].
> 
> The output of nm_platform_link_get_address() is already in binary form.

Ah, I think I was thrown off by the 'const char *mac'.  I think 'guint8*' would be better there, since that has less possibility to be confused with 'printable string' like char* is often used.  Also reduces confusion around whether 'char' is signed or unsigned, which varies by platform.

But I think even simpler would be:

	const struct ether_addr *addr;

	addr = nm_platform_link_get_address (NM_PLATFORM_GET, ifindex, &mac_len);
	if (!addr || mac_len != ETH_ALEN) {
		_LOGE (LOGD_AUTOIP4, "IPv4LL: can't retrieve hardware address");
		goto fail;
	}

	r = sd_ipv4ll_set_mac (priv->ipv4ll, addr);
	if (r < 0) {
		_LOGE (LOGD_AUTOIP4, "IPv4LL: set_mac() failed with error %d", r);
		goto fail;
	}

everything else looks good!

(In reply to Dan Williams from comment #13)

> But I think even simpler would be:
> 
> 	const struct ether_addr *addr;
> 
> 	addr = nm_platform_link_get_address (NM_PLATFORM_GET, ifindex, &mac_len);
> 	if (!addr || mac_len != ETH_ALEN) {
> 		_LOGE (LOGD_AUTOIP4, "IPv4LL: can't retrieve hardware address");
> 		goto fail;
> 	}
> 
> 	r = sd_ipv4ll_set_mac (priv->ipv4ll, addr);
> 	if (r < 0) {
> 		_LOGE (LOGD_AUTOIP4, "IPv4LL: set_mac() failed with error %d", r);
> 		goto fail;
> 	}

Yeah, this is better; pushed a fixup to the branch.

LGTM

LGTM (nice!)

Squashed and merged to master.

b341431 ipv4ll: merge branch 'bg/ipv4ll-bgo747463'