Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

network: add network_ref/_unref() and make Link object take a reference of Network object #12475

Merged
merged 10 commits into from
May 7, 2019

Conversation

yuwata
Copy link
Member

@yuwata yuwata commented May 4, 2019

Then, NetDev, Network, and Link objects can be unrefed in arbitrary orders, and now the first commit of #12463 (EDIT: which was already dropped from #12464) can be safely applied. So, it is cherry-picked on top of that.

Fixes the second issue of #12452.

@ssahani PTAL.

@ssahani
Copy link
Contributor

ssahani commented May 4, 2019

@yuwata networkd is crashing. Could you please fix it . thanks !

Assertion 'p->n_ref > 0' failed at ../src/network/networkd-network.c:587, function network_unref(). Aborting.
--- test-networkd-conf end ---
make: *** [run] Error 1
make: Leaving directory `/root/systemd/test/TEST-24-UNIT-TESTS'
[TASK END] Sat  4 May 07:27:07 BST 2019

@yuwata
Copy link
Member Author

yuwata commented May 4, 2019

Ouch! I hope the failure is fixed now.

@yuwata yuwata force-pushed the network-fix-12452 branch 5 times, most recently from 5d0c682 to 9aee0ba Compare May 5, 2019 14:01
@yuwata
Copy link
Member Author

yuwata commented May 5, 2019

The CentOS CI failure is #12344. Should be unrelated.

======================================================================
FAIL: test_bond_operstate (__main__.NetworkdNetWorkBondTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./test/test-network/systemd-networkd-tests.py", line 1581, in test_bond_operstate
    self.assertRegex(output, 'State: no-carrier \(configured\)')
AssertionError: Regex didn't match: 'State: no-carrier \\(configured\\)' not found in '● 104: bond99\n       Link File: /usr/lib/systemd/network/99-default.link\n    Network File: /run/systemd/network/bond99.network\n            Type: bond\n           State: degraded-carrier (configured)\n          Driver: bonding\n      HW Address: 4a:45:fe:d2:fd:65\n         Address: 192.168.123.45\n                  fe80::4845:feff:fed2:fd65'

----------------------------------------------------------------------

@yuwata
Copy link
Member Author

yuwata commented May 7, 2019

In #12452, @Just4GH confirmed now all memleaks are fixed by this. PTAL.

Copy link
Member

@keszybz keszybz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow all the refcounting details, but look OK. Some minor suggestions below.

src/network/networkd-manager.c Show resolved Hide resolved
return 0;
}

a = new(struct in6_addr, 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newdup() ?

yuwata and others added 10 commits May 7, 2019 16:55
Because of this the fd is getting closed and we getting errors
like
```
^Ceno1: Could not send rtnetlink message: Bad file descriptor
enp7s0f0: Could not send rtnetlink message: Bad file descriptor
enp7s0f0: Cannot delete unreachable route for DHCPv6 delegated subnet 2a0a:...:fc::/62: Bad file descriptor
Assertion '*_head == _item' failed at ../systemd/src/network/networkd-route.c:126, function route_free(). Aborting.
Aborted
```

Closes one of systemd#12452
The function sd_radv_add_prefix() in dhcp6_pd_prefix_assign() may
return -EEXIST, and in that case the sd_radv_prefix object allocated
in dhcp6_pd_prefix_assign() will be freed when the function returns.
Hence, the key value in Manager::dhcp6_prefixes hashmap is lost.
Fixes one memleak found in systemd#12452.
When address is in IPv4, the remaining buffer in in_addr_union may
not be initialized.

Fixes the following valgrind warning:
```
==13169== Conditional jump or move depends on uninitialised value(s)
==13169==    at 0x137FF6: UnknownInlinedFun (networkd-ndisc.c:77)
==13169==    by 0x137FF6: UnknownInlinedFun (networkd-ndisc.c:580)
==13169==    by 0x137FF6: ndisc_handler.lto_priv.83 (networkd-ndisc.c:597)
==13169==    by 0x11BE23: UnknownInlinedFun (sd-ndisc.c:201)
==13169==    by 0x11BE23: ndisc_recv.lto_priv.174 (sd-ndisc.c:254)
==13169==    by 0x4AA18CF: source_dispatch (sd-event.c:2821)
==13169==    by 0x4AA1BC2: sd_event_dispatch (sd-event.c:3234)
==13169==    by 0x4AA1D88: sd_event_run (sd-event.c:3291)
==13169==    by 0x4AA1FAB: sd_event_loop (sd-event.c:3313)
==13169==    by 0x117401: UnknownInlinedFun (networkd.c:113)
==13169==    by 0x117401: main (networkd.c:120)
==13169==  Uninitialised value was created by a stack allocation
==13169==    at 0x1753C8: manager_rtnl_process_address (networkd-manager.c:479)
```
The change in manager_rtnl_process_address() may not be necessary,
but for safety, let's initialize the value.
@yuwata
Copy link
Member Author

yuwata commented May 7, 2019

@keszybz Thank you for the review. The two suggestions are addressed.

@keszybz
Copy link
Member

keszybz commented May 7, 2019

bionic-amd64 fails in udev test:

TEST 30: program with subshell
device '/devices/pci0000:00/0000:00:1f.2/host0/target0:0:0/0:0:0:0/block/sda/sda5' expecting node/link 'bar9'
add:         error
test/tmpfs/dev
├── block
│   └── 8:5 -> ../sda5
├── char
├── null
└── sda5

Not sure what to make of this, but it looks unrelated.

@keszybz keszybz merged commit 717e8ed into systemd:master May 7, 2019
@yuwata yuwata deleted the network-fix-12452 branch May 7, 2019 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants