Re: [systemd-devel] [PATCH] systemd-networkd man repair
On 25.04.2014 19:44, Tom Gundersen wrote: > Hi poma, > > The patch looks correct, thanks. However, Lennart requested that I > drop the whole config file (and hence the man page), so the patch I'm > working on now makes this one redundant. Sorry about that. > > Cheers, > > Tom > Slip the jab! :) poma ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-dhcp-client: log positive error number
On Sat, Apr 26, 2014 at 12:36 AM, Tom Gundersen wrote:
> On Sun, Apr 13, 2014 at 12:52 PM, Umut Tezduyar Lindskog
> wrote:
>> Log error no for such client_stop(client, DHCP_EVENT_STOP)
>> ---
>> src/libsystemd-network/sd-dhcp-client.c |5 -
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/libsystemd-network/sd-dhcp-client.c
>> b/src/libsystemd-network/sd-dhcp-client.c
>> index 4892203..9715953 100644
>> --- a/src/libsystemd-network/sd-dhcp-client.c
>> +++ b/src/libsystemd-network/sd-dhcp-client.c
>> @@ -230,7 +230,10 @@ static int client_initialize(sd_dhcp_client *client) {
>> static sd_dhcp_client *client_stop(sd_dhcp_client *client, int error) {
>> assert_return(client, NULL);
>>
>> -log_dhcp_client(client, "STOPPED: %s", strerror(-error));
>> +if (error < 0)
>> +log_dhcp_client(client, "STOPPED: %s", strerror(-error));
>> +else
>> +log_dhcp_client(client, "STOPPED: %d", error);
>
> The idea is good, but I don't think the error messages when error >= 0
> are going to be very helpful (just a number does not tell much). I
> suggest doing a switch statement with proper logging depending on the
> case. Something like (not even compile-tested):
>
> else {
> switch(error) {
> case DHCP_EVENT_STOPPED:
> log_dhcp_client(client, "STOPPED");
> case DHCP_EVENT_NO_LEASE:
> log_dhcp_client(client, "STOPPED: no lease");
> default:
> log_dhcp_client(client, "STOPPED: unknown reason");
> }
> }
>
> What do you think?
Great. Should I wrap all of this to a dhcp_client_sterror function?
dhcp_client_strerrr (int error) {
switch (error) {
case error < 0:
return sterror(-error)
case DHCP_EVENT_STOPPED:
return "normal" (or whatever)
case DHCP_EVENT_NO_LEASE
return "no lease"
default
return "not implemented"
}
Umut
>
> Cheers,
>
> Tom
> ___
> systemd-devel mailing list
> systemd-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/2] core: let selinux_setup() load policy more than once
On Sat, Apr 26, 2014 at 12:26 AM, Will Woods wrote:
> When you switch-root into a new root that has SELinux policy, you're
> supposed to to run selinux_init_load_policy() to set up SELinux and load
> policy. Normally this gets handled by selinux_setup().
>
> But if SELinux was already initialized, selinux_setup() skips loading
> policy and returns 0. So if you load policy normally, and then you
> switch-root to a new root that has new policy, selinux_setup() never
> loads the new policy. What gives?
>
> As far as I can tell, this check is an artifact of how selinux_setup()
> worked when it was first written (see commit c4dcdb9 / systemd v12):
>
> * when systemd starts, run selinux_setup()
> * if selinux_setup() loads policy OK, restart systemd
>
> So the "if policy already loaded, skip load and return 0" check was
> there to prevent an infinite re-exec loop.
>
> Modern systemd only calls selinux_setup() on initial load and after
> switch-root, and selinux_setup() no longer restarts systemd, so we don't
> need that check to guard against the infinite loop anymore.
>
> So: this patch removes the "return 0", thus allowing selinux_setup() to
> actually perform SELinux setup after switch-root.
>
> We still want to check to see if SELinux is initialized, because if
> selinux_init_load_policy() fails *but* SELinux is initialized that means
> we still have (old) policy active. So we don't need to halt if
> enforce=1.
Looks good to me, but I'll leave for others to comment.
Cheers,
Tom
> ---
> src/core/selinux-setup.c | 10 --
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/src/core/selinux-setup.c b/src/core/selinux-setup.c
> index 6d8bc89..578a18f 100644
> --- a/src/core/selinux-setup.c
> +++ b/src/core/selinux-setup.c
> @@ -51,6 +51,7 @@ int selinux_setup(bool *loaded_policy) {
> security_context_t con;
> int r;
> union selinux_callback cb;
> +bool initialized = false;
>
> assert(loaded_policy);
>
> @@ -68,13 +69,8 @@ int selinux_setup(bool *loaded_policy) {
> /* Already initialized by somebody else? */
> r = getcon_raw(&con);
> if (r == 0) {
> -bool initialized;
> -
> initialized = !streq(con, "kernel");
> freecon(con);
> -
> -if (initialized)
> -return 0;
> }
>
> /* Make sure we have no fds open while loading the policy and
> @@ -115,9 +111,11 @@ int selinux_setup(bool *loaded_policy) {
> } else {
> log_open();
>
> -if (enforce > 0) {
> +if ((enforce > 0) && (!initialized)) {
> log_error("Failed to load SELinux policy.
> Freezing.");
> return -EIO;
> +} else if (enforce > 0) {
> +log_warning("Failed to load new SELinux policy.
> Continuing with old policy.");
> } else
> log_debug("Unable to load SELinux policy.
> Ignoring.");
> }
> --
> 1.9.0
>
> ___
> systemd-devel mailing list
> systemd-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/2] core: reindent {selinux, ima, smack}-setup.c
Applied. Thanks!
Tom
On Sat, Apr 26, 2014 at 12:26 AM, Will Woods wrote:
> 7-space indentation is just too weird to leave alone.
> Make it 8 spaces, as per CODING_STYLE. No other changes.
> ---
> src/core/ima-setup.c | 110 +-
> src/core/selinux-setup.c | 152
> +++
> 2 files changed, 131 insertions(+), 131 deletions(-)
>
> diff --git a/src/core/ima-setup.c b/src/core/ima-setup.c
> index ed65096..7bffd8d 100644
> --- a/src/core/ima-setup.c
> +++ b/src/core/ima-setup.c
> @@ -44,63 +44,63 @@
> int ima_setup(void) {
>
> #ifdef HAVE_IMA
> - struct stat st;
> - ssize_t policy_size = 0, written = 0;
> - char *policy;
> - _cleanup_close_ int policyfd = -1, imafd = -1;
> - int result = 0;
> -
> - if (stat(IMA_POLICY_PATH, &st) < 0)
> - return 0;
> -
> - policy_size = st.st_size;
> - if (stat(IMA_SECFS_DIR, &st) < 0) {
> - log_debug("IMA support is disabled in the kernel, ignoring.");
> - return 0;
> - }
> -
> - if (stat(IMA_SECFS_POLICY, &st) < 0) {
> - log_error("Another IMA custom policy has already been loaded,
> "
> - "ignoring.");
> - return 0;
> - }
> -
> - policyfd = open(IMA_POLICY_PATH, O_RDONLY|O_CLOEXEC);
> - if (policyfd < 0) {
> - log_error("Failed to open the IMA custom policy file %s (%m),
> "
> - "ignoring.", IMA_POLICY_PATH);
> - return 0;
> - }
> -
> - imafd = open(IMA_SECFS_POLICY, O_WRONLY|O_CLOEXEC);
> - if (imafd < 0) {
> - log_error("Failed to open the IMA kernel interface %s (%m), "
> - "ignoring.", IMA_SECFS_POLICY);
> - goto out;
> - }
> -
> - policy = mmap(NULL, policy_size, PROT_READ, MAP_PRIVATE, policyfd, 0);
> - if (policy == MAP_FAILED) {
> - log_error("mmap() failed (%m), freezing");
> - result = -errno;
> - goto out;
> - }
> -
> - written = loop_write(imafd, policy, (size_t)policy_size, false);
> - if (written != policy_size) {
> - log_error("Failed to load the IMA custom policy file %s (%m),
> "
> - "ignoring.", IMA_POLICY_PATH);
> - goto out_mmap;
> - }
> -
> - log_info("Successfully loaded the IMA custom policy %s.",
> -IMA_POLICY_PATH);
> +struct stat st;
> +ssize_t policy_size = 0, written = 0;
> +char *policy;
> +_cleanup_close_ int policyfd = -1, imafd = -1;
> +int result = 0;
> +
> +if (stat(IMA_POLICY_PATH, &st) < 0)
> +return 0;
> +
> +policy_size = st.st_size;
> +if (stat(IMA_SECFS_DIR, &st) < 0) {
> +log_debug("IMA support is disabled in the kernel,
> ignoring.");
> +return 0;
> +}
> +
> +if (stat(IMA_SECFS_POLICY, &st) < 0) {
> +log_error("Another IMA custom policy has already been
> loaded, "
> + "ignoring.");
> +return 0;
> +}
> +
> +policyfd = open(IMA_POLICY_PATH, O_RDONLY|O_CLOEXEC);
> +if (policyfd < 0) {
> +log_error("Failed to open the IMA custom policy file %s
> (%m), "
> + "ignoring.", IMA_POLICY_PATH);
> +return 0;
> +}
> +
> +imafd = open(IMA_SECFS_POLICY, O_WRONLY|O_CLOEXEC);
> +if (imafd < 0) {
> +log_error("Failed to open the IMA kernel interface %s (%m), "
> + "ignoring.", IMA_SECFS_POLICY);
> +goto out;
> +}
> +
> +policy = mmap(NULL, policy_size, PROT_READ, MAP_PRIVATE, policyfd,
> 0);
> +if (policy == MAP_FAILED) {
> +log_error("mmap() failed (%m), freezing");
> +result = -errno;
> +goto out;
> +}
> +
> +written = loop_write(imafd, policy, (size_t)policy_size, false);
> +if (written != policy_size) {
> +log_error("Failed to load the IMA custom policy file %s
> (%m), "
> + "ignoring.", IMA_POLICY_PATH);
> +goto out_mmap;
> +}
> +
> +log_info("Successfully loaded the IMA custom policy %s.",
> + IMA_POLICY_PATH);
> out_mmap:
> - munmap(policy, policy_size);
> +munmap(policy, policy_size);
> out:
> - if (result)
> -return result;
> +if (result)
> + return result;
> #endif /* HAVE_IMA */
>
> - return 0;
> +return 0;
> }
> diff --git a/src/core/selinux-setup.c b/src/core/selinux-setup.c
> index 9a5d6b2..6d8bc89 100644
> --- a/src/core/selinux-setup.c
> +++ b/src/core/selinux-setup.c
> @@ -46,82 +46,82 @@ static int null_
Re: [systemd-devel] [PATCH] unit: add waiting jobs to run queue in unit_coldplug
On Wed, Apr 23, 2014 at 2:36 PM, Lennart Poettering wrote: > This looks correct, but could you move this into job_coldplug()? I rewrote the patch to be in job_coldplug() and tested. Patch attached. 0001-job-add-waiting-jobs-to-run-queue-in-unit_coldplug.patch Description: Binary data ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] sd-dhcp-client: log positive error number
On Sun, Apr 13, 2014 at 12:52 PM, Umut Tezduyar Lindskog
wrote:
> Log error no for such client_stop(client, DHCP_EVENT_STOP)
> ---
> src/libsystemd-network/sd-dhcp-client.c |5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/src/libsystemd-network/sd-dhcp-client.c
> b/src/libsystemd-network/sd-dhcp-client.c
> index 4892203..9715953 100644
> --- a/src/libsystemd-network/sd-dhcp-client.c
> +++ b/src/libsystemd-network/sd-dhcp-client.c
> @@ -230,7 +230,10 @@ static int client_initialize(sd_dhcp_client *client) {
> static sd_dhcp_client *client_stop(sd_dhcp_client *client, int error) {
> assert_return(client, NULL);
>
> -log_dhcp_client(client, "STOPPED: %s", strerror(-error));
> +if (error < 0)
> +log_dhcp_client(client, "STOPPED: %s", strerror(-error));
> +else
> +log_dhcp_client(client, "STOPPED: %d", error);
The idea is good, but I don't think the error messages when error >= 0
are going to be very helpful (just a number does not tell much). I
suggest doing a switch statement with proper logging depending on the
case. Something like (not even compile-tested):
else {
switch(error) {
case DHCP_EVENT_STOPPED:
log_dhcp_client(client, "STOPPED");
case DHCP_EVENT_NO_LEASE:
log_dhcp_client(client, "STOPPED: no lease");
default:
log_dhcp_client(client, "STOPPED: unknown reason");
}
}
What do you think?
Cheers,
Tom
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH 1/2] core: reindent {selinux, ima, smack}-setup.c
7-space indentation is just too weird to leave alone.
Make it 8 spaces, as per CODING_STYLE. No other changes.
---
src/core/ima-setup.c | 110 +-
src/core/selinux-setup.c | 152 +++
2 files changed, 131 insertions(+), 131 deletions(-)
diff --git a/src/core/ima-setup.c b/src/core/ima-setup.c
index ed65096..7bffd8d 100644
--- a/src/core/ima-setup.c
+++ b/src/core/ima-setup.c
@@ -44,63 +44,63 @@
int ima_setup(void) {
#ifdef HAVE_IMA
- struct stat st;
- ssize_t policy_size = 0, written = 0;
- char *policy;
- _cleanup_close_ int policyfd = -1, imafd = -1;
- int result = 0;
-
- if (stat(IMA_POLICY_PATH, &st) < 0)
- return 0;
-
- policy_size = st.st_size;
- if (stat(IMA_SECFS_DIR, &st) < 0) {
- log_debug("IMA support is disabled in the kernel, ignoring.");
- return 0;
- }
-
- if (stat(IMA_SECFS_POLICY, &st) < 0) {
- log_error("Another IMA custom policy has already been loaded, "
- "ignoring.");
- return 0;
- }
-
- policyfd = open(IMA_POLICY_PATH, O_RDONLY|O_CLOEXEC);
- if (policyfd < 0) {
- log_error("Failed to open the IMA custom policy file %s (%m), "
- "ignoring.", IMA_POLICY_PATH);
- return 0;
- }
-
- imafd = open(IMA_SECFS_POLICY, O_WRONLY|O_CLOEXEC);
- if (imafd < 0) {
- log_error("Failed to open the IMA kernel interface %s (%m), "
- "ignoring.", IMA_SECFS_POLICY);
- goto out;
- }
-
- policy = mmap(NULL, policy_size, PROT_READ, MAP_PRIVATE, policyfd, 0);
- if (policy == MAP_FAILED) {
- log_error("mmap() failed (%m), freezing");
- result = -errno;
- goto out;
- }
-
- written = loop_write(imafd, policy, (size_t)policy_size, false);
- if (written != policy_size) {
- log_error("Failed to load the IMA custom policy file %s (%m), "
- "ignoring.", IMA_POLICY_PATH);
- goto out_mmap;
- }
-
- log_info("Successfully loaded the IMA custom policy %s.",
-IMA_POLICY_PATH);
+struct stat st;
+ssize_t policy_size = 0, written = 0;
+char *policy;
+_cleanup_close_ int policyfd = -1, imafd = -1;
+int result = 0;
+
+if (stat(IMA_POLICY_PATH, &st) < 0)
+return 0;
+
+policy_size = st.st_size;
+if (stat(IMA_SECFS_DIR, &st) < 0) {
+log_debug("IMA support is disabled in the kernel, ignoring.");
+return 0;
+}
+
+if (stat(IMA_SECFS_POLICY, &st) < 0) {
+log_error("Another IMA custom policy has already been loaded, "
+ "ignoring.");
+return 0;
+}
+
+policyfd = open(IMA_POLICY_PATH, O_RDONLY|O_CLOEXEC);
+if (policyfd < 0) {
+log_error("Failed to open the IMA custom policy file %s (%m), "
+ "ignoring.", IMA_POLICY_PATH);
+return 0;
+}
+
+imafd = open(IMA_SECFS_POLICY, O_WRONLY|O_CLOEXEC);
+if (imafd < 0) {
+log_error("Failed to open the IMA kernel interface %s (%m), "
+ "ignoring.", IMA_SECFS_POLICY);
+goto out;
+}
+
+policy = mmap(NULL, policy_size, PROT_READ, MAP_PRIVATE, policyfd, 0);
+if (policy == MAP_FAILED) {
+log_error("mmap() failed (%m), freezing");
+result = -errno;
+goto out;
+}
+
+written = loop_write(imafd, policy, (size_t)policy_size, false);
+if (written != policy_size) {
+log_error("Failed to load the IMA custom policy file %s (%m), "
+ "ignoring.", IMA_POLICY_PATH);
+goto out_mmap;
+}
+
+log_info("Successfully loaded the IMA custom policy %s.",
+ IMA_POLICY_PATH);
out_mmap:
- munmap(policy, policy_size);
+munmap(policy, policy_size);
out:
- if (result)
-return result;
+if (result)
+ return result;
#endif /* HAVE_IMA */
- return 0;
+return 0;
}
diff --git a/src/core/selinux-setup.c b/src/core/selinux-setup.c
index 9a5d6b2..6d8bc89 100644
--- a/src/core/selinux-setup.c
+++ b/src/core/selinux-setup.c
@@ -46,82 +46,82 @@ static int null_log(int type, const char *fmt, ...) {
int selinux_setup(bool *loaded_policy) {
#ifdef HAVE_SELINUX
- int enforce = 0;
- usec_t before_load, after_load;
- security_context_t con;
- int r;
- union selinux_callback cb;
-
- assert(loaded_policy);
-
- /* Turn off all of SELinux' own logging, we want to do that */
- cb.f
[systemd-devel] [PATCH 2/2] core: let selinux_setup() load policy more than once
When you switch-root into a new root that has SELinux policy, you're
supposed to to run selinux_init_load_policy() to set up SELinux and load
policy. Normally this gets handled by selinux_setup().
But if SELinux was already initialized, selinux_setup() skips loading
policy and returns 0. So if you load policy normally, and then you
switch-root to a new root that has new policy, selinux_setup() never
loads the new policy. What gives?
As far as I can tell, this check is an artifact of how selinux_setup()
worked when it was first written (see commit c4dcdb9 / systemd v12):
* when systemd starts, run selinux_setup()
* if selinux_setup() loads policy OK, restart systemd
So the "if policy already loaded, skip load and return 0" check was
there to prevent an infinite re-exec loop.
Modern systemd only calls selinux_setup() on initial load and after
switch-root, and selinux_setup() no longer restarts systemd, so we don't
need that check to guard against the infinite loop anymore.
So: this patch removes the "return 0", thus allowing selinux_setup() to
actually perform SELinux setup after switch-root.
We still want to check to see if SELinux is initialized, because if
selinux_init_load_policy() fails *but* SELinux is initialized that means
we still have (old) policy active. So we don't need to halt if
enforce=1.
---
src/core/selinux-setup.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/src/core/selinux-setup.c b/src/core/selinux-setup.c
index 6d8bc89..578a18f 100644
--- a/src/core/selinux-setup.c
+++ b/src/core/selinux-setup.c
@@ -51,6 +51,7 @@ int selinux_setup(bool *loaded_policy) {
security_context_t con;
int r;
union selinux_callback cb;
+bool initialized = false;
assert(loaded_policy);
@@ -68,13 +69,8 @@ int selinux_setup(bool *loaded_policy) {
/* Already initialized by somebody else? */
r = getcon_raw(&con);
if (r == 0) {
-bool initialized;
-
initialized = !streq(con, "kernel");
freecon(con);
-
-if (initialized)
-return 0;
}
/* Make sure we have no fds open while loading the policy and
@@ -115,9 +111,11 @@ int selinux_setup(bool *loaded_policy) {
} else {
log_open();
-if (enforce > 0) {
+if ((enforce > 0) && (!initialized)) {
log_error("Failed to load SELinux policy. Freezing.");
return -EIO;
+} else if (enforce > 0) {
+log_warning("Failed to load new SELinux policy.
Continuing with old policy.");
} else
log_debug("Unable to load SELinux policy. Ignoring.");
}
--
1.9.0
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] core: let selinux_setup() load policy more than once
Hey all, Currently, systemd refuses to load SELinux policy more than once. Normal systems don't care, because they either: a) have initramfs without policy, then load policy after switch-root, or b) load policy in initramfs, and never switch-root out. But if you *do* switch-root more than once - which fedup does! - you're supposed to run selinux_init_load_policy() afterward to ensure that you set up selinuxfs and load the new SELinux policy correctly. So: this patchset fixes some weird and subtle upgrade bugs[1] by allowing systemd to load policy after each switch-root. The first patch is cosmetic: selinux-setup.c was indented 7 spaces (!?) and it bothered me too much to leave it alone. The second patch makes systemd try to load policy regardless of whether or not SELinux is already initialized. It also changes the failure behavior slightly: normally if policy load fails and the user requested enforcing, we freeze the system rather than running without policy. But if SELinux was already initialized, then we *do* have policy, so we don't have to freeze. Diffstat ignoring whitespace: src/core/ima-setup.c | 0 src/core/selinux-setup.c | 10 -- 2 files changed, 4 insertions(+), 6 deletions(-) Thanks, -w [1] Here's one weird and subtle bug: https://bugzilla.redhat.com/show_bug.cgi?id=1045864 It turns out there's actually a race between systemd-cryptsetup and systemd-ask-password-*.path to create /run/systemd/ask-password. If systemd wins, it labels the directory correctly, but systemd-cryptsetup doesn't do label_init(), so it doesn't know how to label the directory. Normally that doesn't matter, because systemd relabels /run after it reloads policy... but if it doesn't load policy, it doesn't relabel /run, the directory keeps the wrong label, and then the password agent can't read files from there, and your boot hangs forever waiting for a password... ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 3/3] nspawn: allow to bind mount journal on top of a non empty container journal dentry
On Fri, Apr 11, 2014 at 2:45 AM, Djalal Harouni wrote:
> Currently if nspawn was called with --link-journal=host or
> --link-journal=auto and the right /var/log/journal/machine-id/ exists
> then the bind mount the subdirectory into the container might fail due
> to the ~/mycontainer/var/log/journal/machine-id/ of the container not
> being empty.
>
> There is no reason to check if the container journal subdir is empty
> since there will be a bind mount on top of it. The user asked for a bind
> mount so give it.
>
> Note: a next call with --link-journal=guest may fail due to the
> /var/log/journal/machine-id/ on the host not being empty.
>
> https://bugs.freedesktop.org/show_bug.cgi?id=76193
Hm, so this will allow some journal entries to be saved on the host
and some on the guest, but only one of them
will be shown by "journalctl --merge" at any given time... Won't this
be confusing? Either way I guess this case
should be documented in the manpage (either that it is not allowed, or
that it may be confusing)...
> Reported-by: Tobias Hunger
> ---
> src/nspawn/nspawn.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c
> index 21322f7..afb003c 100644
> --- a/src/nspawn/nspawn.c
> +++ b/src/nspawn/nspawn.c
> @@ -1152,11 +1152,6 @@ static int setup_journal(const char *directory) {
> } else if (access(p, F_OK) < 0)
> return 0;
>
> -if (dir_is_empty(q) == 0) {
> -log_error("%s not empty.", q);
> -return -ENOTEMPTY;
> -}
> -
> r = mkdir_p(q, 0755);
> if (r < 0) {
> log_error("Failed to create %s: %m", q);
> --
> 1.8.5.3
>
> ___
> systemd-devel mailing list
> systemd-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/3] nspawn: make nspawn able to cleanly terminate on container errors
On Fri, Apr 11, 2014 at 2:45 AM, Djalal Harouni wrote:
> nspawn and the container child use eventfd to wait and notify each other
> that they are ready so the container setup can be completed.
>
> However in its current form the wait/notify event ignore errors that
> may especially affect the child (container).
>
> On errors the child will jump to the "child_fail" label and terminate
> with _exit(EXIT_FAILURE) without notifying the parent. Since the eventfd
> is created without the "EFD_NONBLOCK" flag, this leaves the parent
> blocking on the eventfd_read() call.
>
> To fix this without adding extra overheads, we keep the eventfd logic
> and improve it by adding:
>
> * States of the parent and child setups:
> SETUP_INIT, SETUP_SUCCEEDED and SETUP_FAILED
>
> * In the child if the setup succeeded we notify parent by writing a
> SETUP_SUCCEEDED value, otherwise we make sure to write a SETUP_FAILED
> before the _exit(). This prevents the parent from waiting on an event
> that will never come.
>
> * In parent read the child setup state, if SETUP_SUCCEEDED continue,
> otherwise jump to "check_container_status" label, get the container
> child status and release resources.
>
> https://bugs.freedesktop.org/show_bug.cgi?id=76193
>
> Reported-by: Tobias Hunger
Looks good to me.
Cheers,
Tom
> ---
> src/nspawn/nspawn.c | 50 ++
> 1 file changed, 38 insertions(+), 12 deletions(-)
>
> diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c
> index d606bf2..21322f7 100644
> --- a/src/nspawn/nspawn.c
> +++ b/src/nspawn/nspawn.c
> @@ -104,6 +104,16 @@ typedef enum LinkJournal {
> LINK_GUEST
> } LinkJournal;
>
> +enum {
> +/* States of the parent (nspawn) and child (container) setups
> + * These are used for event notification so we can inform
> + * the other party if the appropriate setup succeeded or
> + * failed. */
> +SETUP_INIT,
> +SETUP_SUCCEEDED,
> +SETUP_FAILED
> +};
> +
> static char *arg_directory = NULL;
> static char *arg_user = NULL;
> static sd_id128_t arg_uuid = {};
> @@ -2815,16 +2825,16 @@ int main(int argc, char *argv[]) {
>
> for (;;) {
> ContainerStatus container_status;
> +eventfd_t event_status;
> int parent_ready_fd = -1, child_ready_fd = -1;
> -eventfd_t x;
>
> -parent_ready_fd = eventfd(0, EFD_CLOEXEC);
> +parent_ready_fd = eventfd(SETUP_INIT, EFD_CLOEXEC);
> if (parent_ready_fd < 0) {
> log_error("Failed to create event fd: %m");
> goto finish;
> }
>
> -child_ready_fd = eventfd(0, EFD_CLOEXEC);
> +child_ready_fd = eventfd(SETUP_INIT, EFD_CLOEXEC);
> if (child_ready_fd < 0) {
> log_error("Failed to create event fd: %m");
> goto finish;
> @@ -2980,7 +2990,7 @@ int main(int argc, char *argv[]) {
> /* Tell the parent that we are ready, and that
> * it can cgroupify us to that we lack access
> * to certain devices and resources. */
> -eventfd_write(child_ready_fd, 1);
> +eventfd_write(child_ready_fd, SETUP_SUCCEEDED);
> child_ready_fd = safe_close(child_ready_fd);
>
> if (chdir(arg_directory) < 0) {
> @@ -3081,7 +3091,7 @@ int main(int argc, char *argv[]) {
> env_use = (char**) envp;
>
> /* Wait until the parent is ready with the setup,
> too... */
> -eventfd_read(parent_ready_fd, &x);
> +eventfd_read(parent_ready_fd, &event_status);
> parent_ready_fd = safe_close(parent_ready_fd);
>
> if (arg_boot) {
> @@ -3113,17 +3123,29 @@ int main(int argc, char *argv[]) {
> log_error("execv() failed: %m");
>
> child_fail:
> +/* Tell the parent that the setup failed, so he
> + * can clean up resources and terminate. */
> +if (child_ready_fd != -1) {
> +eventfd_write(child_ready_fd, SETUP_FAILED);
> +child_ready_fd = safe_close(child_ready_fd);
> +}
> _exit(EXIT_FAILURE);
> }
>
> fdset_free(fds);
> fds = NULL;
>
> -/* Wait until the child reported that it is ready with
> - * all it needs to do with priviliges. After we got
> - * the notification we can make the process join its
> - * cgroup which might limit what it can do */
> -
Re: [systemd-devel] [PATCH 1/3] nspawn: move container wait logic into wait_for_container() function
On Fri, Apr 11, 2014 at 2:45 AM, Djalal Harouni wrote:
> Move the container wait logic into its own wait_for_container() function
> and add two status codes: CONTAINER_TERMINATED or CONTAINER_REBOOTED
>
> These status codes are used to terminate nspawn or loop again in case of
> CONTAINER_REBOOTED.
Looks good, but some nit-picks below.
> ---
> src/nspawn/nspawn.c | 114
> ++--
> 1 file changed, 75 insertions(+), 39 deletions(-)
>
> diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c
> index 0bd52da..d606bf2 100644
> --- a/src/nspawn/nspawn.c
> +++ b/src/nspawn/nspawn.c
> @@ -92,6 +92,11 @@
> #include "seccomp-util.h"
> #endif
>
> +typedef enum ContainerStatus {
> +CONTAINER_TERMINATED,
> +CONTAINER_REBOOTED
> +} ContainerStatus;
> +
> typedef enum LinkJournal {
> LINK_NO,
> LINK_AUTO,
> @@ -2565,6 +2570,72 @@ static int change_uid_gid(char **_home) {
> return 0;
> }
>
> +/* Return 0 in case the container is being rebooted, has been shut
> + * down or exited succesfully. On failures a non-zero value is
> + * returned.
> + *
> + * The status of the container "CONTAINER_TERMINATED" or
> + * "CONTAINER_REBOOTED" will be saved in the container argument */
> +static int wait_for_container(pid_t pid, ContainerStatus *container) {
> +int r, k;
> +siginfo_t status;
> +
> +/* Explicitly set this to CONTAINER_TERMINATED. If the reboot
> + * conditions are met it will be updated to CONTAINER_REBOOTED */
> +*container = CONTAINER_TERMINATED;
We don't usually clobber the arguments unless we return successfully,
so maybe delay this until we return.
> +r = EXIT_FAILURE;
> +k = wait_for_terminate(pid, &status);
> +if (k < 0)
> +return r;
Maybe better to just return k here, as EXIT_FAILURE is usually only
used in main() (also I think you can then drop k and just use r).
> +if (status.si_code == CLD_EXITED) {
Wouldn't this be better as a big switch?
> +r = status.si_status;
> +if (status.si_status != 0) {
> +log_error("Container %s failed with error code %i.",
> + arg_machine, status.si_status);
> +goto out;
Why not just "return status.si_status" and drop the out: label?
> +}
> +
> +if (!arg_quiet)
> +log_debug("Container %s exited successfully.",
> + arg_machine);
> +
> +} else if (status.si_code == CLD_KILLED &&
> + status.si_status == SIGINT) {
> +
> +if (!arg_quiet)
> +log_info("Container %s has been shut down.",
> + arg_machine);
> +r = 0;
Same, just return directly, or let it fall through to a final "return
0" (and set *container here).
> +} else if (status.si_code == CLD_KILLED &&
> + status.si_status == SIGHUP) {
> +
> +if (!arg_quiet)
> +log_info("Container %s is being rebooted.",
> + arg_machine);
> +r = 0;
> +*container = CONTAINER_REBOOTED;
> +
> +} else if (status.si_code == CLD_KILLED ||
> + status.si_code == CLD_DUMPED) {
> +
> +log_error("Container %s terminated by signal %s.",
> + arg_machine, signal_to_string(status.si_status));
> +r = EXIT_FAILURE;
> +
> +} else {
> +log_error("Container %s failed due to unknown reason.",
> + arg_machine);
> +r = EXIT_FAILURE;
> +}
> +
> +out:
> +return r;
> +}
> +
> int main(int argc, char *argv[]) {
>
> _cleanup_free_ char *kdbus_domain = NULL, *device_path = NULL,
> *root_device = NULL, *home_device = NULL, *srv_device = NULL;
> @@ -2743,8 +2814,8 @@ int main(int argc, char *argv[]) {
> assert_se(sigprocmask(SIG_BLOCK, &mask, NULL) == 0);
>
> for (;;) {
> +ContainerStatus container_status;
> int parent_ready_fd = -1, child_ready_fd = -1;
> -siginfo_t status;
> eventfd_t x;
>
> parent_ready_fd = eventfd(0, EFD_CLOEXEC);
> @@ -3094,48 +3165,13 @@ int main(int argc, char *argv[]) {
> /* Redundant, but better safe than sorry */
> kill(pid, SIGKILL);
>
> -k = wait_for_terminate(pid, &status);
> +r = wait_for_container(pid, &container_status);
> pid = 0;
>
> -if (k < 0) {
> -r = EXIT_FAILURE;
> -break;
> -}
> -
> -if (status.si_code == CLD_EXITED) {
> -r = status.si_statu
Re: [systemd-devel] [PATCH] systemd-networkd man repair
Hi poma, The patch looks correct, thanks. However, Lennart requested that I drop the whole config file (and hence the man page), so the patch I'm working on now makes this one redundant. Sorry about that. Cheers, Tom On Fri, Apr 25, 2014 at 5:42 AM, poma wrote: > Corrections to the manuals related to networkd-wait-online. > > --- > man/networkd-wait-online.conf.xml| 4 ++-- > man/systemd-networkd-wait-online.service.xml | 4 ++-- > man/systemd-networkd.service.xml | 4 ++-- > 3 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/man/networkd-wait-online.conf.xml > b/man/networkd-wait-online.conf.xml > index 821b0ef..684326f 100644 > --- a/man/networkd-wait-online.conf.xml > +++ b/man/networkd-wait-online.conf.xml > @@ -75,8 +75,7 @@ > to wait for before deciding if the machine > is considered to be online. The default is > to wait for at least one that is managed by > - > systemd-networkd.service > - > 8, > + > systemd-networkd.service8, > or one other which is brought up by someone > else. > > > @@ -89,6 +88,7 @@ > > > systemd1, > > systemd-networkd.service8, > + > systemd-networkd-wait-online.service8 > > > > diff --git a/man/systemd-networkd-wait-online.service.xml > b/man/systemd-networkd-wait-online.service.xml > index d8f2226..78794be 100644 > --- a/man/systemd-networkd-wait-online.service.xml > +++ b/man/systemd-networkd-wait-online.service.xml > @@ -64,7 +64,7 @@ > systemd-networkd to appear, or for a link to be brought up > by someone else. > > To wait for a specific set of links, see > - > systemd-networkd.conf5. > + > networkd-wait-online.conf5. > This may be necessary on systems with more than one active > link. > > > @@ -73,8 +73,8 @@ > See Also > > > systemd1, > - > systemd-networkd.conf5, > > systemd-networkd.service8, > + > networkd-wait-online.conf5 > > > > diff --git a/man/systemd-networkd.service.xml > b/man/systemd-networkd.service.xml > index ca35a88..76ce73f 100644 > --- a/man/systemd-networkd.service.xml > +++ b/man/systemd-networkd.service.xml > @@ -100,8 +100,8 @@ > > systemd1, > > systemd.link5, > > systemd.network5, > - > systemd.netdev5 > - > systemd-network-wait-online.service8 > + > systemd.netdev5, > + > systemd-networkd-wait-online.service8 > > > > -- > 1.9.0 > > ___ > systemd-devel mailing list > systemd-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/systemd-devel ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/1] networkd: Introduce ipip tunnel
On Fri, Apr 11, 2014 at 6:29 PM, Susant Sahani wrote: > On 04/08/2014 12:54 PM, "Jóhann B. Guðmundsson" wrote: > > > On 04/08/2014 03:22 AM, Susant Sahani wrote: > > file: ipip.netdev > -- > [NetDev] > Name=ipip-tun > Kind=ipip > > [Tunnel] > Local=192.168.8.102 > Remote=10.4.4.4 > TTL=64 > MTUBytes=1480 > > file: ipip.network > -- > [Match] > Name=eth0 > > [Network] > Tunnel=ipip-tun > > > I think this is worse from previous example since now you have moved the > network definitions out from the network file and into the net device file. > > > Well thanks for the comment . I am open for the change . Leaving tom to > comment on this . [sorry for breaking the quoting, hopefully it is clear who said what] I actually think this is the correct way to do it, as the addresses (which I assume is what Jóhann is objecting to?) are properties of the link (similar to mac addresses, mtu, etc) rather than regular ip addresses that you can configure for normal devices. Or maybe I'm missing something? Anyone else have any input on this? If no one else objects I'll merge this (and I'll fix up Zbiginew's comment as I do so, so no need to resend just for that). Cheers, Tom ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/2] networkd: ipip tunell add address support
On Mon, Apr 14, 2014 at 12:41 PM, Susant Sahani wrote: > On 04/14/2014 04:04 PM, "Jóhann B. Guðmundsson" wrote: >> >> >> On 04/14/2014 10:31 AM, Susant Sahani wrote: >>> >>> It's a name not tunnel type. Tunnel type configured in .netdev . you can >>> put anything here . Should match the .netdev Name='XYZ' >>> >>> The Kind is mode which you can replace with ipip/sit/gre . >>> >>> file: ipip.netdev >>> -- >>> [NetDev] >>> Name=ipip-tun<===Name >>> Kind=ipip <== tunnel type >> >> >> I thought you had switched to correct it Kind=tunnel just like you define >> bridging, bonding and vlan there, with Tunnel= in the network section >> setting the mode of the tunnel. >> > hmm . That was my original idea. You might want to check . > >> I guess Tom just has to rule on this since to me how you are implementing >> things adds an additional learning curve to administrators both since it >> deviates from the command line as well as configuration this from /etc/net >> configuration perspective. Hi guys, Sorry for the delay in getting back to this. My thoughts have been that we should not treat tunnels differently from other types of netdevs. I.e., we can do # ip link add foo type bridge or we can do # ip link add foo type ipip So from iproute2(8)'s point of view they behave the same (and from the kernel's point of view they behave the same), so it makes sense to me to treat them the same in networkd too. Moreover, we currently have two kinds of vlan's (vlan and macvlan) and we will undoubtedly get two kinds of bonding (bond and team). If we were to subdivide the types of netdevs, I guess we should do that uniformly and not only for tunnels, so either way I'm tempted to merge this for now and then we can possibly get back to subdividing these things later. Hope that makes sense. Cheers, Tom ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] Cache passphrase for cryptsetup?
On Sun, Apr 20, 2014 at 09:55:11PM +0300, Mantas Mikulėnas wrote: >On 2014-04-20 21:45, Matthew Monaco wrote: >> And of course, the third option would be to submit a patch. The >> src/cryptsetup >> stuff is pretty straightforward. > >Wasn't one submitted just a month ago? Yes, patches 1/3 and 2/3 were committed very recently and I still need to post patch 3/3. Then a separate patch is necessary for the cryptsetup package in Debian and after that, keyscript= will work for Debian at least. -- David Härdeman ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
