Skip to content

Conversation

@Nevexo
Copy link
Contributor

@Nevexo Nevexo commented Jan 3, 2025

This PR fixes #12 by placing the udhcpc DHCP client into an idle state when the network interface goes link down.

When the link transitions down, this PR will:

  • Send udhcpc the USR2 signal which puts it into an idle state 1
  • Remove all IPv4 addresses from the interface

Then once the interface comes back up, it'll send USR1 to udhcpc which activates it and forces it to get a new lease.

This not only fixes the issue reported in #12 by removing the active addresses, it ensures that if the JetKVM is moved to a new subnet, it'll get an IPv4 address from the DHCP server on the new network, rather than trying to hold on to a dead lease from the previous network.

Implementation

I'd like some feedback on the implementation of this, as it's just invoking the killall command with the signals, rather than using syscall.Kill with a signal, I avoided that method as that requires figuring out the PID of the udhcpc service, and I'm not sure if that's worth doing, since killall will always be present when jetkvm_app is running on the official buildroot image.

It may also be worth checking that the udhcpc process is actually running, and if it's not, is it the programs responsibility to start it?

Further work is needed on the IPv6 stack to see if #12 also effects that, I'll try this soon, but issue #11 is the more important IPv6 issue at this time.

Open to feedback, thank you!

1 - https://udhcp.busybox.net/README.udhcpc

@Nevexo Nevexo force-pushed the nevexo/fix-ipv4-display-isssue branch from f916cb5 to 4265b94 Compare January 3, 2025 09:39
@Nevexo Nevexo requested a review from adamshiervani January 3, 2025 09:40
@Nevexo Nevexo self-assigned this Jan 3, 2025
@Nevexo Nevexo force-pushed the nevexo/fix-ipv4-display-isssue branch 2 times, most recently from 7ca180b to ef1e3b7 Compare January 3, 2025 09:42
This commit fixes jetkvm#12 by disabling the udhcpc client when the
link goes down, it then removes all the active IPv4 addresses from the
deivce.

Once the link comes back up, it re-activates the udhcpc client so it can
fetch a new IPv4 address for the device.

This doesn't make any changes to the IPv6 side of things yet.
@Nevexo Nevexo force-pushed the nevexo/fix-ipv4-display-isssue branch from ef1e3b7 to 1e589ed Compare January 3, 2025 11:44
@eric
Copy link

eric commented Jan 3, 2025

What is the benefit of telling the client to release the IP when the interface is down? The preference would be for it to always try to hold on to the same IP.

Additionally, what is the value to actually remove the IP from the interface instead of just not displaying it in the UI when it is down? It's only going to make it take longer for the device to come back after link is restored.

@Nevexo
Copy link
Contributor Author

Nevexo commented Jan 3, 2025

What is the benefit of telling the client to release the IP when the interface is down? The preference would be for it to always try to hold on to the same IP.

The way I imagined it, is if the link is going down, the user is probably moving it to a different network, so it should drop that IP address entirely and let the DHCP client find a completely new one.

It's only going to make it take longer for the device to come back after link is restored.
If the device is switching networks, having it hold onto the IP is going to make it take much longer as the client attempts to renew it's existing IP address, until it gives up and requests a completely new one.

In my testing, it was still less than a few seconds from link-up, to pingable, but I guess if you have a slow DHCP server, or a noisy network, it could be quite a bit slower.

However, if the general consensus is that the device will probably be connected back to the same network when the link restores, then we can drop the IP removal stage and just blank out the displayed address.

@eric
Copy link

eric commented Jan 3, 2025

100% of the time that my JetKVM loses link it will be because the switch was rebooted or I temporarily pulled the cable to do something. I never renumber my network, but I do sometimes have reasons why a device would lose link for a moment.

@Nevexo
Copy link
Contributor Author

Nevexo commented Jan 3, 2025

Fair enough, I guess it all comes down to use-case, if the device spends most of it's time sat in a rack it'll almost always come back up on the same network.

My use usually ends up with the KVM running on a USB battery bank and jumping between devices, and potentially between different management networks.

I'll give it a test without removing the IP addresses and swap it between networks to make sure that still works, and we'll go from there.

Thanks for your feedback!

@apalrd
Copy link
Contributor

apalrd commented Jan 3, 2025

I think at a minimum you must try to renew the lease when the link goes back up. When link is lost, you have no idea if you are on the same network as you were before. A user in a room with a huge mess of cables could be plugging the device into each one and looking at the DHCP IPs to see if they have 'got' the right one, for example. Going from the 'network is down' screen to a screen which still shows the old IP (even if it is renewing in the background, and will eventually get the right IP) would be misleading to the user in this case.

You could skip the USR2 to release and only do USR1 on link up, which will renew the lease if it already exists, but then you don't get great feedback (since afaik you are watching Netlink and not udhcp) on when the renewal is a success to display on the screen.

@Nevexo
Copy link
Contributor Author

Nevexo commented Jan 4, 2025

since afaik you are watching Netlink and not udhcp

Yeah, it currently watches the address with netlink, so if it's still "installed" the IP will just reappear on the LCD, even if udhcpc is in the process of trying to get a new lease, hence why my current version deletes all IPv4 addresses on link-down.

You could skip the USR2 to release and only do USR1 on link up

I could probably do this anyway really, there's no real need to put udhcpc into idle mode now I think of it, just need to tell it to renew as soon as the link comes up.

@CLAassistant
Copy link

CLAassistant commented Feb 13, 2025

CLA assistant check
All committers have signed the CLA.

@chemhack chemhack merged commit 2a99c2d into jetkvm:dev Feb 13, 2025
1 check passed
@Nevexo Nevexo deleted the nevexo/fix-ipv4-display-isssue branch February 13, 2025 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Display will show a previous IPv4 address if an address was assigned from a previous connection

5 participants