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

Adapted changes of 'Ignore carrier gainloss' #10187

Closed
wants to merge 233 commits into from
Closed

Adapted changes of 'Ignore carrier gainloss' #10187

wants to merge 233 commits into from

Conversation

cybin
Copy link

@cybin cybin commented Sep 27, 2018

Adapted changes of #7403 to merge with currect state.

keszybz and others added 30 commits September 15, 2018 19:43
When loading units, sometimes we'd first encounter a unit from .wants or
.requires directory. A typical case would be when multi-user.target.wants/
contains a symlink to some unit. We would prepare to load this unit using
/etc/systemd/system/multi-user.target.wants/foo.service as the fragment
path. This is always wrong. Instead, let's use NULL as the path and let
manager_load_unit() figure out the path on its own.

Fixes #9921.

    path=0x5625ed9b01a0 "/usr/lib/systemd/system/local-fs.target.wants/systemd-remount-fs.service", e=0x0,
    _ret=0x7ffe64645000) at ../src/core/manager.c:1887
    name=0x5625ed9b01ce "systemd-remount-fs.service",
    path=0x5625ed9b01a0 "/usr/lib/systemd/system/local-fs.target.wants/systemd-remount-fs.service", e=0x0,
    _ret=0x7ffe64645000) at ../src/core/manager.c:1961
    name=0x5625ed9b01ce "systemd-remount-fs.service",
    path=0x5625ed9b01a0 "/usr/lib/systemd/system/local-fs.target.wants/systemd-remount-fs.service",
    add_reference=true, mask=UNIT_DEPENDENCY_FILE) at ../src/core/unit.c:2946
    dir_suffix=0x5625ebb179ed ".wants") at ../src/core/load-dropin.c:95
    path=0x0, e=0x0, _ret=0x7ffe646452c0) at ../src/core/manager.c:1965
    name=0x5625ebb186f8 "local-fs.target", path=0x0, add_reference=true,
    mask=UNIT_DEPENDENCY_MOUNTINFO_IMPLICIT) at ../src/core/unit.c:2946
    where=0x5625ed9b3cc0 "/tmp", options=0x5625ed947110 "rw,nosuid,nodev,seclabel",
    fstype=0x5625ed95be90 "tmpfs", flags=0x7ffe64645395) at ../src/core/mount.c:1439
    where=0x5625ed9b3cc0 "/tmp", options=0x5625ed947110 "rw,nosuid,nodev,seclabel",
    fstype=0x5625ed95be90 "tmpfs", set_flags=false) at ../src/core/mount.c:1567
    at ../src/core/mount.c:1635
    ret_retval=0x7ffe64645660, ret_shutdown_verb=0x7ffe646456c0, ret_fds=0x7ffe646456d8,
    ret_switch_root_dir=0x7ffe646456b0, ret_switch_root_init=0x7ffe646456b8,
    ret_error_message=0x7ffe646456c8) at ../src/core/main.c:1669
This reverts commit 48d3e88.

I kept the follow-symlink=false → follow-symlink=true change instact, since
we're likely to have existing installations with a symlink now.
This reverts commit 0187368.
(systemd.conf.m4 part was already reverted in 5b5d826.)
This reverts commit d4e9e57.
(systemd.conf.m4 part was already reverted in 5b5d826.)

Together those reverts should "fix" #10025 and #10011. ("fix" is in quotes
because this doesn't really fix the underlying issue, which is combining
DynamicUser= with strict container sandbox, but it avoids the problem by not
using that feature in our default installation.)

Dynamic users don't work well if the service requires matching configuration in
other places, for example dbus policy. This is true for those three services.
In effect, distros create the user statically [1, 2]. Dynamic users make more
sense for "add-on" services where not creating the user, or more precisely,
creating the user lazily, can save resources. For "basic" services, if we are
going to create the user on package installation anyway, setting DynamicUser=
just creates unneeded confusion. The only case where it is actually used is
when somebody forgets to do system configuration. But it's better to have the
service fail cleanly in this case too. If we want to turn on some side-effect
of DynamicUser=yes for those services, we should just do that directly through
fine-grained options. By not using DynamicUser= we also avoid the need to
restart dbus.

[1] https://salsa.debian.org/systemd-team/systemd/commit/bd9bf307274faca24699c0c2d67cb86f18c0b2cb
[2] https://src.fedoraproject.org/rpms/systemd/blob/48ac1cebdedb055d9daf3dfe28c7bde80103f7a1/f/systemd.spec#_473
(Fedora does not create systemd-timesync user.)
"because blacklisted" is not grammatical, it should be something like
"becuase it is blacklisted", which in turn in too verbose. Let's use
a shorter form.
We have "installed tests", but don't provide an easy way to run them.

The protocol is very simple: each test must return 0 for success, 77 means
"skipped", anything else is an error. In addition, we want to print test
output only if the test failed.

I wrote this simple script. It is pretty basic, but implements the functions
listed above. Since it is written in python it should be easy to add option
parsing (like running only specific tests, or running unsafe tests, etc.)

I looked at the following alternatives:
- Ubuntu root-unittests: this works, but just dumps all output to the terminal,
  has no coloring.
- @ssahani's test runner [2]
  It uses the unittest library and the test suite was implented as a class, and
  doesn't implement any of the functions listed above.
- cram [3,4]
  cram runs our tests, but does not understand the "ignore the output" part,
  has not support for our magic skip code (it uses hardcoded 80 instead),
  and seems dead upstream.
- meson test
  Here the idea would be to provide an almost-empty meson.build file under
  /usr/lib/systemd/tests/ that would just define all the tests. This would
  allow us to reuse the test runner we use normally. Unfortunately meson requires
  a build directory and configuration to be done before running tests. This
  would be possible, but seems a lot of effort to just run a few binaries.

[1] https://salsa.debian.org/systemd-team/systemd/blob/242c96addb06480ec9cd75248a5660f37a17b4b9/debian/tests/root-unittests
[2] https://github.com/systemd/systemd-fedora-ci/blob/master/upstream/systemd-upstream-tests.py
[3] https://bitheap.org/cram/
[4] https://pypi.org/project/pytest-cram/

Fixes #10069.
we might as well filter these too since negative PIDs have special
semantics in kill(), and we should never trigger that...
If SPLIT_RELAX is specified, then it accepts unfinished quotes or
missing separator after right quote.
This intends to clarify the rule udev_build_argv() uses.
This also modernize udev_event_spawn() a bit.
poettering and others added 28 commits October 8, 2018 21:40
boot_loader_read_conf(), boot_entries_find(), boot_entries_load_config()
all log their errors internally, hence no need to log a second or third
time about the same error when they return.
Let's internalize these four calls as noone else calls them.
We shouldn't generate an error in that case, as the file is optional.
This is really confusing, let's try to clean this up a bit, in
particular as there are two very similar concepts:

1. The boot loaders, i.e. the category you find systemd-boot, the
   Windows and Apple boot loaders in. These may typically be listed in the
   firmware's EFI variables.

2. The boot loader entries, as defined by the Boot Loader Spec. In this
   category you find the various Linux kernels that are installed, i.e.
   the stuff systemd-boot shows on screen. To make things confusing, the
   Windows and Apple boot loaders can appear both as boot loaders and as
   boot loader entries.

This tries to establish the following nomenclature: "boot loaders" and
"boot loader entries" for these two concepts.
Also, let's move the --help and --version items to the end of the list.
let's add an env var for this, as this really shouldn't be a top-level
feature, as it turning off the validity checks certainly isn't
advisable.

Fixes: #4925
On EFI variables that aren't whitelisted in the kernel the
FS_IMMUTABLE_FL is set, as protection against accidental
removal/modification. Since our own variables do not appear in those
whielists, and we are not changing these variables, let's unset the flag
temporarily when needed. We restore the flag after all writes, just in
case.
…n $BOOT in the "list" output

Let's use the new LoaderEntries efi var for this, and show all entries
we couldn't find via the config snippets.
the bootctl changes from PR #9437 (the boot counting PR)
Some uses of n_netif already assumed it had time size_t. Others were a
bit sloppy. Let's clean this up.
…10314)

Let's make sure the integers we parse out are not larger than USHRT_MAX.
This is a good idea as the kernel's TIOCSWINSZ ioctl for sizing
terminals can't take larger values, and we shouldn't risk an overflow.
Until a core dump handler is installed by systemd-sysctl, the generation of
core dump for services is turned OFF which can make the debugging of the early
boot process harder especially since there's no easy way to restore the core
dump generation.

This patch introduces a new kernel command line option which specifies an
absolute path where the kernel should write the core dump file when an early
process crashes.

This will take effect until systemd-coredump (or any other handlers) takes
over.
…d $COLUMNS

This way users can directly influence the tty size if they like when
nspawn is invoked as a service and thus stdin/stdout/stderr are not
connected to a TTY.
Adapted changes of #7403 to merge with currect state.
@cybin cybin closed this Oct 9, 2018
@amishmm
Copy link

amishmm commented Oct 24, 2018

this seems to have explicit checks for dhcp, but what about ipv4ll/ipv6ll?

@poettering thats because @ssahani did not like DHCP being touched and I added checks. (may be to keep him happy and have some progress in PR).

I would have preferred code to not check anything at all because administrator is not supposed to set IgnoreCarrierGainLoss=true anyway for cases where it makes no sense.

See: #7403 (comment)

I kept asking other developers to consider my point of view because there were differences with opinions of @ssahani but there was almost no response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
network reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks
Development

Successfully merging this pull request may close these issues.

None yet