network: improve debug logs and add tests for IPv4LL#17798
network: improve debug logs and add tests for IPv4LL#17798keszybz merged 6 commits intosystemd:masterfrom
Conversation
I cannot find that behavior. The IPv4LL address is dropped soon after DHCPv4 address becomes ready. I've added a test to confirm that. PTAL. |
f57d874 to
0430b6f
Compare
keszybz
left a comment
There was a problem hiding this comment.
LGTM, except for the double-eval thing. It doesn't matter for the current usage, but I think it'd be nicer to avoid the issue to avoid bugs in the future.
I'm not sure why I saw different behaviour in my local test. I'll retest after all outstanding PRs have been merged.
src/libsystemd-network/sd-ipv4acd.c
Outdated
| } | ||
|
|
||
| int sd_ipv4acd_get_ifindex(sd_ipv4acd *acd) { | ||
| assert_return(acd, -EINVAL); |
There was a problem hiding this comment.
EBADF?
Maybe make this not assert_return but silent? See comments below...
There was a problem hiding this comment.
EBADF?? At least under libsystemd-network, we do not use EBADF...
src/libsystemd-network/sd-ipv4acd.c
Outdated
| #define log_ipv4acd_errno(acd, error, fmt, ...) log_internal(LOG_DEBUG, error, PROJECT_FILE, __LINE__, __func__, "IPV4ACD: " fmt, ##__VA_ARGS__) | ||
| #define log_ipv4acd(acd, fmt, ...) log_ipv4acd_errno(acd, 0, fmt, ##__VA_ARGS__) | ||
| #define log_ipv4acd_errno(acd, error, fmt, ...) \ | ||
| log_interface_full_errno(acd ? sd_ipv4acd_get_ifname(acd) : NULL, LOG_DEBUG, error, "IPV4ACD: " fmt, ##__VA_ARGS__) |
There was a problem hiding this comment.
Unfortunately this does double evaluation of acd. So maybe make sd_ipv4acd_get_ifname() just return NULL if the argument is NULL without complaining?
src/libsystemd-network/sd-ipv4ll.c
Outdated
| #define log_ipv4ll_errno(ll, error, fmt, ...) log_internal(LOG_DEBUG, error, PROJECT_FILE, __LINE__, __func__, "IPV4LL: " fmt, ##__VA_ARGS__) | ||
| #define log_ipv4ll(ll, fmt, ...) log_ipv4ll_errno(ll, 0, fmt, ##__VA_ARGS__) | ||
| #define log_ipv4ll_errno(ll, error, fmt, ...) \ | ||
| log_interface_full_errno((ll && ll->acd) ? sd_ipv4acd_get_ifname(ll->acd) : NULL, LOG_DEBUG, error, "IPV4LL: " fmt, ##__VA_ARGS__) |
| self.assertRegex(output, 'inet 192.168.5.[0-9]*/24 brd 192.168.5.255 scope global dynamic veth99') | ||
| output = check_output('ip -4 address show dev veth99 scope link') | ||
| self.assertNotRegex(output, 'inet .* scope link') | ||
| self.assertNotRegex(output, 'inet 169.254.[0-9]*.[0-9]*/16 brd 169.254.255.255 scope link') |
There was a problem hiding this comment.
Not that it matters, but maybe:
r'inet 169\.254\.\d+\.\d+/16 brd 169.254.255.255 scope link'
src/network/networkd-dhcp4.c
Outdated
| if (!a) | ||
| return 0; | ||
|
|
||
|
|
They will be used in later commits. This also makes sd_ipv4acd_set_ifindex() check the existence of the interface.
0430b6f to
5f016e3
Compare
|
@keszybz Thank you for the review. All points except |
|
LGTM. |
|
bionic-s390x failed in timedated test, looks unrelated. |
|
bionic-* is failing in timedated test (same as other PRs). |
No description provided.