Re: [systemd-devel] [PATCH] CODING_STYLE: this also help with unaligned memory accesses
On Thu, Mar 26, 2015 at 1:31 AM, Lennart Poettering wrote: > On Tue, 24.03.15 11:16, Shawn Landden (sh...@churchofgit.com) wrote: > >> And those arches don't get much testing too. >> --- >> CODING_STYLE | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/CODING_STYLE b/CODING_STYLE >> index b687e72..8934954 100644 >> --- a/CODING_STYLE >> +++ b/CODING_STYLE >> @@ -14,7 +14,8 @@ >> - The destructors always unregister the object from the next bigger >>object, not the other way around >> >> -- To minimize strict aliasing violations, we prefer unions over casting >> +- To minimize strict aliasing violations and unaligned memory accesses, >> + we prefer unions over casting > > Unaligned memory accesses are an orthogonal problem really. I don't > see how the change above really makes sense? casting often leads to unaligned memory accesses. Without casting the compiler usually gets it right, but yeah this has little to do with unions. I was confusing with having a packed and unpacked struct and converting between the two like thus: typedef struct { uint8_t a uint32_t b } packed __attribute__((packed)); #if HAVE_FAST_UNALIGNED #define unpacked packed #elsif typedef struct { uint8_t a // uint8_t __padding[3]; gcc will do this for you uint32_t b } unpacked; #endif foo get_foo(p *void) { unpacked e; e.a = (*packed)->a; e.b = (*packed)->b; return e; } > > Lennart > > -- > Lennart Poettering, Red Hat > ___ > systemd-devel mailing list > systemd-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/systemd-devel -- Shawn Landden ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] How to debug blocking service start?
Hello Kai, Kai Hendry [2015-03-27 12:18 +0800]: > Wish there was a service validator service. Not sure what you changed, but "systemd-analyze verify foo.service" might be a good start? Martin -- Martin Pitt| http://www.piware.de Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org) ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/2] macro: allow assert_se() assertions to also be optimized out when NDEBUG is set
On Thu, Mar 26, 2015 at 5:47 PM, Djalal Harouni wrote: > On Fri, Mar 27, 2015 at 12:30:53AM +0100, Tom Gundersen wrote: >> On Thu, Mar 26, 2015 at 9:19 AM, Lennart Poettering >> wrote: >> > On Tue, 24.03.15 11:11, Shawn Landden (sh...@churchofgit.com) wrote: >> > >> >> Will result in slightly smaller binaries, and cuts out the branch, even if >> >> the expression is still executed. >> > >> > I am sorry, but the whole point of assert_se() is that it has side >> > effects. That's why it is called that way (the "se" suffix stands for >> > "side effects"), and is different from assert(). You are supposed to >> > use it whenever the code enclosed in it shall not be suppressed if >> > NDEBUG is defined. This patch of yours breaks that whole logic! >> >> Hm, am I reading the patch wrong? It looks good to me. It is simply >> dropping the branching and logging in the NDEBUG case, but the >> expression itself is still evaluated. > Yep but it seems that abort() will never be called, > log_assert_failed() => abort() > > And the logging mechanism is also one of those side effects. IMO unless > there are real valid reasons for any change to these macors... changing > anything here will just bring more bugs that we may never notice. > Those are the side effects of assert(), the side effects of the expression are still evaluated. > >> So in the NDEBUG case "assert_se(foo())" becomes equivalent to >> "foo()", which I guess makes sense (though I doubt it makes much of a >> difference). >> >> -t >> >> >> --- >> >> src/shared/macro.h | 12 ++-- >> >> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> >> >> diff --git a/src/shared/macro.h b/src/shared/macro.h >> >> index 7f89951..02219ea 100644 >> >> --- a/src/shared/macro.h >> >> +++ b/src/shared/macro.h >> >> @@ -212,17 +212,17 @@ static inline unsigned long ALIGN_POWER2(unsigned >> >> long u) { >> >> (__x / __y + !!(__x % __y));\ >> >> }) >> >> >> >> -#define assert_se(expr) \ >> >> -do {\ >> >> -if (_unlikely_(!(expr)))\ >> >> -log_assert_failed(#expr, __FILE__, __LINE__, >> >> __PRETTY_FUNCTION__); \ >> >> -} while (false) \ >> >> - >> >> /* We override the glibc assert() here. */ >> >> #undef assert >> >> #ifdef NDEBUG >> >> +#define assert_se(expr) do {expr} while(false) >> >> #define assert(expr) do {} while(false) >> >> #else >> >> +#define assert_se(expr) \ >> >> +do {\ >> >> +if (_unlikely_(!(expr)))\ >> >> +log_assert_failed(#expr, __FILE__, __LINE__, >> >> __PRETTY_FUNCTION__); \ >> >> +} while (false) >> >> #define assert(expr) assert_se(expr) >> >> #endif >> >> >> >> -- >> >> 2.2.1.209.g41e5f3a >> >> >> >> ___ >> >> systemd-devel mailing list >> >> systemd-devel@lists.freedesktop.org >> >> http://lists.freedesktop.org/mailman/listinfo/systemd-devel >> > >> > >> > Lennart >> > >> > -- >> > Lennart Poettering, Red Hat >> > ___ >> > 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 > > -- > Djalal Harouni > http://opendz.org > ___ > systemd-devel mailing list > systemd-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/systemd-devel -- Shawn Landden ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/2] macro: allow assert_se() assertions to also be optimized out when NDEBUG is set
On Thu, Mar 26, 2015 at 1:19 AM, Lennart Poettering wrote: > On Tue, 24.03.15 11:11, Shawn Landden (sh...@churchofgit.com) wrote: > >> Will result in slightly smaller binaries, and cuts out the branch, even if >> the expression is still executed. > > I am sorry, but the whole point of assert_se() is that it has side > effects. That's why it is called that way (the "se" suffix stands for > "side effects"), and is different from assert(). You are supposed to > use it whenever the code enclosed in it shall not be suppressed if > NDEBUG is defined. This patch of yours breaks that whole logic! +#define assert_se(expr) do {expr} while(false) #define assert(expr) do {} while(false) No it does not. see "{expr}", it still is doing the side effect. It just doesn't check if the return value is as expected. > > Lennart > >> --- >> src/shared/macro.h | 12 ++-- >> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/src/shared/macro.h b/src/shared/macro.h >> index 7f89951..02219ea 100644 >> --- a/src/shared/macro.h >> +++ b/src/shared/macro.h >> @@ -212,17 +212,17 @@ static inline unsigned long ALIGN_POWER2(unsigned long >> u) { >> (__x / __y + !!(__x % __y));\ >> }) >> >> -#define assert_se(expr) \ >> -do {\ >> -if (_unlikely_(!(expr)))\ >> -log_assert_failed(#expr, __FILE__, __LINE__, >> __PRETTY_FUNCTION__); \ >> -} while (false) \ >> - >> /* We override the glibc assert() here. */ >> #undef assert >> #ifdef NDEBUG >> +#define assert_se(expr) do {expr} while(false) >> #define assert(expr) do {} while(false) >> #else >> +#define assert_se(expr) \ >> +do {\ >> +if (_unlikely_(!(expr)))\ >> +log_assert_failed(#expr, __FILE__, __LINE__, >> __PRETTY_FUNCTION__); \ >> +} while (false) >> #define assert(expr) assert_se(expr) >> #endif >> >> -- >> 2.2.1.209.g41e5f3a >> >> ___ >> systemd-devel mailing list >> systemd-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/systemd-devel > > > Lennart > > -- > Lennart Poettering, Red Hat > ___ > systemd-devel mailing list > systemd-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/systemd-devel -- Shawn Landden ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] cgroup: propagate cgroup mask only for proportional properties
Some of cgroup properties does not affect to sibling cgroups. CPUShares and BlockIOWeight are only needed to be propagated. --- src/core/cgroup.c | 29 - src/core/cgroup.h | 2 ++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 6b8abb4..b4b9678 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -458,6 +458,23 @@ void cgroup_context_apply(CGroupContext *c, CGroupControllerMask mask, const cha } } +CGroupControllerMask cgroup_context_get_proportional_mask(CGroupContext *c) { +CGroupControllerMask mask = 0; + +/* Get only proportional mask */ + +if (c->cpu_shares != (unsigned long) -1 || +c->startup_cpu_shares != (unsigned long) -1) +mask |= CGROUP_CPU; + +if (c->blockio_weight != (unsigned long) -1 || +c->startup_blockio_weight != (unsigned long) -1 || +c->blockio_device_weights) +mask |= CGROUP_BLKIO; + +return mask; +} + CGroupControllerMask cgroup_context_get_mask(CGroupContext *c) { CGroupControllerMask mask = 0; @@ -487,6 +504,16 @@ CGroupControllerMask cgroup_context_get_mask(CGroupContext *c) { return mask; } +CGroupControllerMask unit_get_cgroup_proportional_mask(Unit *u) { +CGroupContext *c; + +c = unit_get_cgroup_context(u); +if (!c) +return 0; + +return cgroup_context_get_proportional_mask(c); +} + CGroupControllerMask unit_get_cgroup_mask(Unit *u) { CGroupContext *c; @@ -531,7 +558,7 @@ CGroupControllerMask unit_get_members_mask(Unit *u) { continue; u->cgroup_members_mask |= -unit_get_cgroup_mask(member) | +unit_get_cgroup_proportional_mask(member) | unit_get_members_mask(member); } } diff --git a/src/core/cgroup.h b/src/core/cgroup.h index 869ddae..2371158 100644 --- a/src/core/cgroup.h +++ b/src/core/cgroup.h @@ -98,12 +98,14 @@ void cgroup_context_done(CGroupContext *c); void cgroup_context_dump(CGroupContext *c, FILE* f, const char *prefix); void cgroup_context_apply(CGroupContext *c, CGroupControllerMask mask, const char *path, ManagerState state); +CGroupControllerMask cgroup_context_get_proportional_mask(CGroupContext *c); CGroupControllerMask cgroup_context_get_mask(CGroupContext *c); void cgroup_context_free_device_allow(CGroupContext *c, CGroupDeviceAllow *a); void cgroup_context_free_blockio_device_weight(CGroupContext *c, CGroupBlockIODeviceWeight *w); void cgroup_context_free_blockio_device_bandwidth(CGroupContext *c, CGroupBlockIODeviceBandwidth *b); +CGroupControllerMask unit_get_cgroup_proportional_mask(Unit *u); CGroupControllerMask unit_get_cgroup_mask(Unit *u); CGroupControllerMask unit_get_siblings_mask(Unit *u); CGroupControllerMask unit_get_members_mask(Unit *u); -- 1.9.3 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] How to debug blocking service start?
On Fri, 27 Mar 2015, at 12:14 PM, Daurnimator wrote: > My first guess based on that screenshot is case: Simple vs simple. No I fixed that problem. :) http://ix.io/h8U Wish there was a service validator service. ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] How to debug blocking service start?
On 27 March 2015 at 13:32, Kai Hendry wrote: > It's still getting stuck with Type=simple. > > http://s.natalian.org/2015-03-27/simple.png > > Isn't there a better way to debug than running journalctl -u > -f in parallel? > > The frustrating thing is that the SAME service file works fine on > another rpi2. What does your .service file currently look like? My first guess based on that screenshot is case: Simple vs simple. ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] regarding to cgroup siblings mask
On 03/27/2015 05:33 AM, David Timothy Strauss wrote: > On Tue, Mar 24, 2015 at 4:29 AM, WaLyong Cho wrote: >> Could anyone explain why? > > An admin using CPUShares= or a similar proportional CGroup controller > probably assumes that setting the shares to twice the default (for > example) increases the relative proportion of resources for that unit. > However, that is only true if other units competing for that resource > have the same controller(s) enabled so that the kernel knows to > balance the resources accordingly. > > The code in systemd ensures that if any unit uses a proportional > CGroups controller in a slice, all other units in that same slice > enable that controller as well, usually with the default proportions. Thanks, understood. But I think this propagation is needed only for taking weight argument such like CPUShares=weight, StartupCPUShares=weight, BlockIOWeight=weight, StartupBlockIOWeight=weight, BlockIODeviceWeight=device weight. For example, I don't think MemoryLimit= is not option of proportional. It just only limit of its cgroup and does not race with other cgroup. If I'm right, we need to modify unit_get_target_mask() to get only mask for proportional properties. > > unit_get_target_mask() is part of an optimization I added so that > initializing CGroups controllers for a given unit doesn't require > iterating through every other unit in a slice to figure out the > necessary controllers. It provides a bitmask indicating the > controllers in use by its siblings so the unit can enable, say, > CPUShares= if one of its siblings is doing so. > ___ > 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] How to debug blocking service start?
First, thanks for trying to help me Kai. Awesome name btw. On Fri, 27 Mar 2015, at 03:26 AM, Kai Krakow wrote: > Try Type=simple to not let it wait. That is telling systemd, that the > binary > will not daemonize - athough it should be default according to [1]. It's still getting stuck with Type=simple. http://s.natalian.org/2015-03-27/simple.png Isn't there a better way to debug than running journalctl -u -f in parallel? The frustrating thing is that the SAME service file works fine on another rpi2. > However, I'm not sure whether the rest of the setup will work. You should > probably make it a user service, so that you won't have to pass DISPLAY. > Your special setup may be part of the problem. Also keep in mind that > After=graphical.target doesn't imply that X11 is ready to accept > connections You mean systemctl --user right? https://wiki.archlinux.org/index.php/Systemd/User I don't like that since it seems like a lot of extra crud. What's the big deal about passing in the DISPLAY environment? I don't see the point of having a non-root user. Trying to keep things as simple as possible to achieve my use case. Which is to start a browser once online. > - even "worse": it doesn't imply that it is started late in the boot > process. Your service may start as early as graphical.target becomes > scheduled to start [3] - and that may be well before even basic system > setup > is finished. Instead, I suggest using a DM with autologin, and then spawn > a > user service from there. As an alternative, you may want to try working > with > timer units [2] to activate surf.service a few seconds after X11 is > guarenteed to be up, but that would just be a bandaid. Well, if X wasn't up up, I was hoping Restart=always would do the rest. > [1]: man systemd.service > [2]: man systemd.timer Timers seem like a kluge, just to know when X is ready. Cheers, ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/2] macro: allow assert_se() assertions to also be optimized out when NDEBUG is set
On Fri, Mar 27, 2015 at 12:30:53AM +0100, Tom Gundersen wrote: > On Thu, Mar 26, 2015 at 9:19 AM, Lennart Poettering > wrote: > > On Tue, 24.03.15 11:11, Shawn Landden (sh...@churchofgit.com) wrote: > > > >> Will result in slightly smaller binaries, and cuts out the branch, even if > >> the expression is still executed. > > > > I am sorry, but the whole point of assert_se() is that it has side > > effects. That's why it is called that way (the "se" suffix stands for > > "side effects"), and is different from assert(). You are supposed to > > use it whenever the code enclosed in it shall not be suppressed if > > NDEBUG is defined. This patch of yours breaks that whole logic! > > Hm, am I reading the patch wrong? It looks good to me. It is simply > dropping the branching and logging in the NDEBUG case, but the > expression itself is still evaluated. Yep but it seems that abort() will never be called, log_assert_failed() => abort() And the logging mechanism is also one of those side effects. IMO unless there are real valid reasons for any change to these macors... changing anything here will just bring more bugs that we may never notice. > So in the NDEBUG case "assert_se(foo())" becomes equivalent to > "foo()", which I guess makes sense (though I doubt it makes much of a > difference). > > -t > > >> --- > >> src/shared/macro.h | 12 ++-- > >> 1 file changed, 6 insertions(+), 6 deletions(-) > >> > >> diff --git a/src/shared/macro.h b/src/shared/macro.h > >> index 7f89951..02219ea 100644 > >> --- a/src/shared/macro.h > >> +++ b/src/shared/macro.h > >> @@ -212,17 +212,17 @@ static inline unsigned long ALIGN_POWER2(unsigned > >> long u) { > >> (__x / __y + !!(__x % __y));\ > >> }) > >> > >> -#define assert_se(expr) \ > >> -do {\ > >> -if (_unlikely_(!(expr)))\ > >> -log_assert_failed(#expr, __FILE__, __LINE__, > >> __PRETTY_FUNCTION__); \ > >> -} while (false) \ > >> - > >> /* We override the glibc assert() here. */ > >> #undef assert > >> #ifdef NDEBUG > >> +#define assert_se(expr) do {expr} while(false) > >> #define assert(expr) do {} while(false) > >> #else > >> +#define assert_se(expr) \ > >> +do {\ > >> +if (_unlikely_(!(expr)))\ > >> +log_assert_failed(#expr, __FILE__, __LINE__, > >> __PRETTY_FUNCTION__); \ > >> +} while (false) > >> #define assert(expr) assert_se(expr) > >> #endif > >> > >> -- > >> 2.2.1.209.g41e5f3a > >> > >> ___ > >> systemd-devel mailing list > >> systemd-devel@lists.freedesktop.org > >> http://lists.freedesktop.org/mailman/listinfo/systemd-devel > > > > > > Lennart > > > > -- > > Lennart Poettering, Red Hat > > ___ > > 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 -- Djalal Harouni http://opendz.org ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/2] macro: allow assert_se() assertions to also be optimized out when NDEBUG is set
On Thu, Mar 26, 2015 at 9:19 AM, Lennart Poettering wrote: > On Tue, 24.03.15 11:11, Shawn Landden (sh...@churchofgit.com) wrote: > >> Will result in slightly smaller binaries, and cuts out the branch, even if >> the expression is still executed. > > I am sorry, but the whole point of assert_se() is that it has side > effects. That's why it is called that way (the "se" suffix stands for > "side effects"), and is different from assert(). You are supposed to > use it whenever the code enclosed in it shall not be suppressed if > NDEBUG is defined. This patch of yours breaks that whole logic! Hm, am I reading the patch wrong? It looks good to me. It is simply dropping the branching and logging in the NDEBUG case, but the expression itself is still evaluated. So in the NDEBUG case "assert_se(foo())" becomes equivalent to "foo()", which I guess makes sense (though I doubt it makes much of a difference). -t >> --- >> src/shared/macro.h | 12 ++-- >> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/src/shared/macro.h b/src/shared/macro.h >> index 7f89951..02219ea 100644 >> --- a/src/shared/macro.h >> +++ b/src/shared/macro.h >> @@ -212,17 +212,17 @@ static inline unsigned long ALIGN_POWER2(unsigned long >> u) { >> (__x / __y + !!(__x % __y));\ >> }) >> >> -#define assert_se(expr) \ >> -do {\ >> -if (_unlikely_(!(expr)))\ >> -log_assert_failed(#expr, __FILE__, __LINE__, >> __PRETTY_FUNCTION__); \ >> -} while (false) \ >> - >> /* We override the glibc assert() here. */ >> #undef assert >> #ifdef NDEBUG >> +#define assert_se(expr) do {expr} while(false) >> #define assert(expr) do {} while(false) >> #else >> +#define assert_se(expr) \ >> +do {\ >> +if (_unlikely_(!(expr)))\ >> +log_assert_failed(#expr, __FILE__, __LINE__, >> __PRETTY_FUNCTION__); \ >> +} while (false) >> #define assert(expr) assert_se(expr) >> #endif >> >> -- >> 2.2.1.209.g41e5f3a >> >> ___ >> systemd-devel mailing list >> systemd-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/systemd-devel > > > Lennart > > -- > Lennart Poettering, Red Hat > ___ > 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] regarding to cgroup siblings mask
On Tue, Mar 24, 2015 at 4:29 AM, WaLyong Cho wrote: > If this can be configurable, how about add a configuration for cgroup > mask propagation to siblings? I believe the way to prevent propagation is to separate the units into different slices. ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] regarding to cgroup siblings mask
On Tue, Mar 24, 2015 at 4:29 AM, WaLyong Cho wrote: > Could anyone explain why? An admin using CPUShares= or a similar proportional CGroup controller probably assumes that setting the shares to twice the default (for example) increases the relative proportion of resources for that unit. However, that is only true if other units competing for that resource have the same controller(s) enabled so that the kernel knows to balance the resources accordingly. The code in systemd ensures that if any unit uses a proportional CGroups controller in a slice, all other units in that same slice enable that controller as well, usually with the default proportions. unit_get_target_mask() is part of an optimization I added so that initializing CGroups controllers for a given unit doesn't require iterating through every other unit in a slice to figure out the necessary controllers. It provides a bitmask indicating the controllers in use by its siblings so the unit can enable, say, CPUShares= if one of its siblings is doing so. ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] How to debug blocking service start?
Kai Hendry schrieb: > Hi there, > > How do I figure out why or where something is stuck? > http://s.natalian.org/2015-03-25/systemd-start-issue.png > > `journalctl -u surf -f` prints nothing. > > Binary surf runs fine when I run it manually. It probably is not stuck, it's systemd waiting for the command to return. Try Type=simple to not let it wait. That is telling systemd, that the binary will not daemonize - athough it should be default according to [1]. However, I'm not sure whether the rest of the setup will work. You should probably make it a user service, so that you won't have to pass DISPLAY. Your special setup may be part of the problem. Also keep in mind that After=graphical.target doesn't imply that X11 is ready to accept connections - even "worse": it doesn't imply that it is started late in the boot process. Your service may start as early as graphical.target becomes scheduled to start [3] - and that may be well before even basic system setup is finished. Instead, I suggest using a DM with autologin, and then spawn a user service from there. As an alternative, you may want to try working with timer units [2] to activate surf.service a few seconds after X11 is guarenteed to be up, but that would just be a bandaid. [1]: man systemd.service [2]: man systemd.timer [3]: A target is, AFAIK, reached as soon as all services making up the target are scheduled to start - so you can rely on sockets being ready but nothing more. Targets are sets of functionality to offer, not points in time of the boot process - this is different to what you know of sysvinit. Multiple targets can be active at the same time, e.g. printer.target pulls in cups and becomes activated by udev if you plug in your printer. -- Replies to list only preferred. ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH v2] Add reboot to EFI support
--- man/systemctl.xml | 6 +++- src/libsystemd/sd-bus/bus-common-errors.h | 1 + src/login/logind-dbus.c | 49 +++-- src/login/org.freedesktop.login1.conf | 8 + src/shared/efivars.c | 52 +++ src/shared/efivars.h | 2 ++ src/systemctl/systemctl.c | 16 -- 7 files changed, 127 insertions(+), 7 deletions(-) diff --git a/man/systemctl.xml b/man/systemctl.xml index 50e6bc9..eafdd73 100644 --- a/man/systemctl.xml +++ b/man/systemctl.xml @@ -1538,7 +1538,11 @@ kobject-uevent 1 systemd-udevd-kernel.socket systemd-udevd.service systems. This may result in data loss. If the optional argument -arg is given, it will be passed +arg is given and is equal to +efi, the system will be rebooted to +the EFI firmware interface on machines that support it. +Note that this requires the system to be booted in EFI mode. +Otherwise, the argument will be passed as the optional argument to the reboot2 system call. The value is architecture and firmware diff --git a/src/libsystemd/sd-bus/bus-common-errors.h b/src/libsystemd/sd-bus/bus-common-errors.h index b17b62a..3019140 100644 --- a/src/libsystemd/sd-bus/bus-common-errors.h +++ b/src/libsystemd/sd-bus/bus-common-errors.h @@ -57,6 +57,7 @@ #define BUS_ERROR_DEVICE_IS_TAKEN "org.freedesktop.login1.DeviceIsTaken" #define BUS_ERROR_DEVICE_NOT_TAKEN "org.freedesktop.login1.DeviceNotTaken" #define BUS_ERROR_OPERATION_IN_PROGRESS "org.freedesktop.login1.OperationInProgress" +#define BUS_ERROR_REBOOT_TO_EFI_NOT_SUPPORTED "org.freedesktop.login1.RebootToEfiNotSupported" #define BUS_ERROR_SLEEP_VERB_NOT_SUPPORTED "org.freedesktop.login1.SleepVerbNotSupported" #define BUS_ERROR_AUTOMATIC_TIME_SYNC_ENABLED "org.freedesktop.timedate1.AutomaticTimeSyncEnabled" diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index a3d49ef..8fec90f 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -38,8 +38,11 @@ #include "bus-common-errors.h" #include "udev-util.h" #include "selinux-util.h" +#include "efivars.h" #include "logind.h" +#define SPECIAL_REBOOT_TO_EFI_TARGET "x-logind-reboot-to-efi.target" + int manager_get_session_from_creds(Manager *m, sd_bus_message *message, const char *name, sd_bus_error *error, Session **ret) { _cleanup_bus_creds_unref_ sd_bus_creds *creds = NULL; Session *session; @@ -1422,6 +1425,13 @@ static int execute_shutdown_or_sleep( assert(w < _INHIBIT_WHAT_MAX); assert(unit_name); +if (streq(unit_name, SPECIAL_REBOOT_TO_EFI_TARGET)) { +unit_name = SPECIAL_REBOOT_TARGET; +r = efi_indicate_reboot_to_fw(); +if (r < 0) +return r; +} + bus_manager_log_shutdown(m, w, unit_name); r = sd_bus_call_method( @@ -1563,6 +1573,9 @@ static int method_do_shutdown_or_sleep( if (m->action_what) return sd_bus_error_setf(error, BUS_ERROR_OPERATION_IN_PROGRESS, "There's already a shutdown or sleep operation in progress"); +if (streq(unit_name, SPECIAL_REBOOT_TO_EFI_TARGET) && !is_efi_reboot_to_fw_supported()) +return sd_bus_error_setf(error, BUS_ERROR_REBOOT_TO_EFI_NOT_SUPPORTED, "Reboot to EFI not supported"); + if (sleep_verb) { r = can_sleep(sleep_verb); if (r < 0) @@ -1648,6 +1661,21 @@ static int method_reboot(sd_bus *bus, sd_bus_message *message, void *userdata, s error); } +static int method_reboot_to_efi(sd_bus *bus, sd_bus_message *message, void *userdata, sd_bus_error *error) { +Manager *m = userdata; + +return method_do_shutdown_or_sleep( +m, message, +SPECIAL_REBOOT_TO_EFI_TARGET, +INHIBIT_SHUTDOWN, +"org.freedesktop.login1.reboot", +"org.freedesktop.login1.reboot-multiple-sessions", +"org.freedesktop.login1.reboot-ignore-inhibit", +NULL, +method_reboot_to_efi, +error); +} + static int method_suspend(sd_bus *bus, sd_bus_message *message, void *userdata, sd_bus_error *error) { Manager *m = userdata; @@ -1700,7 +1728,7 @@ static int method_can_shutdown_or_sleep( const char *action, const char *action_multiple_sessions, const char *action_ignore_inhibit, -const char *sleep_verb, +const char *arg, sd_bus_error *error) { _cleanup_bus_creds_unref_ sd_bus_creds *creds = NULL; @@ -1717,8 +1745,8 @@ static int method_ca
Re: [systemd-devel] parsing audit messages
On Thu, Mar 26, 2015 at 09:42:45AM +0100, Lennart Poettering wrote: > On Sun, 15.03.15 03:51, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote: > > > On Sun, Mar 15, 2015 at 03:49:07AM +0100, Zbigniew Jędrzejewski-Szmek wrote: > > > Hi, > > > > > > I was looking at some debug logs, and the audit messages are > > > semi-useless in their current undecoded form: > > > > > > mar 14 22:24:02 fedora22 audit[1]: pid=1 uid=0 > > > auid=4294967295 ses=4294967295 subj=system_u:system_r:init_t:s0 > > > msg='unit=systemd-udev-trigger comm="systemd" > > > exe="/usr/lib/systemd/systemd" hostname=? addr=? terminal=? res=success' > > > mar 14 22:24:05 fedora22 audit: > > > proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D0069707461626C655F7365637572697479 > > > > > > You added code to parse this, and I think we should make use of it and > > > put msg= field as MESSAGE=, and maybe store the original message as > > > _AUDIT= or something. If there's no msg field, like with proctitle, > > > print all fields that are in the message, but using our cescape, and > > > not this hexadecimal form which is unreadable for humans. > > > > I think we should also translate type= to names... > > > > https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/6/html/Security_Guide/sec-Audit_Record_Types.html > > Well, we don't translate MESSAGE_ID fields to strings either... Here the mapping is stable, and maintained in one place... I think it's more like dns TYPE field, completely reversible, then MESSAGE_IDs. I see your point about the format being too messy to parse reliably. OTOH, currently, what we log is much harder to use than the standard audit logs. Dunno. Zbyszek ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [systemd-commits] Makefile.am src/shared src/timedate
On Thu, Mar 26, 2015 at 12:06 PM, Lennart Poettering wrote: > On Thu, 26.03.15 11:57, Kay Sievers (k...@vrfy.org) wrote: > >> > Now, to make calendar time triggers complete I think we must enable >> > people to at least trigger on threse three kinds of anomalies in the >> > time scale. As such I think it would make a *ton* of sense to add: >> > >> > OnTimeZoneChange= >> > OnClockChange= >> >> These are useful, sure. >> >> > OnDSTChange= >> >> This is absolutely not needed and we should not get into that >> business. > > That's just a statement of an opinion. I see no explanation for this. I see only repeated made-up arguments to add an exotic feature, which I don't see adding any real value. DST for the machine is "presentation only". Tools handling it turn the "presentation" into the actual machine's time and be done with it. Glibc does that for us today already and it already works sufficiently for systemd's calendar time support. If the system's time zone changes, or the time is adjusted manually, we just re-arm all timers, and all should be fine. I see no need or use to support explicit triggers on the event of DST changes, the system or calendar time support just does not need them. >> There is no need to fiddle with the raw tzfile data here. > > Well, it's good that things are so simple for you. I'm not willing to discuss things in that tone. Kay ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [systemd-commits] Makefile.am src/shared src/timedate
On Thu, 26.03.15 11:57, Kay Sievers (k...@vrfy.org) wrote: > > Now, to make calendar time triggers complete I think we must enable > > people to at least trigger on threse three kinds of anomalies in the > > time scale. As such I think it would make a *ton* of sense to add: > > > > OnTimeZoneChange= > > OnClockChange= > > These are useful, sure. > > > OnDSTChange= > > This is absolutely not needed and we should not get into that > business. That's just a statement of an opinion. I see no explanation for this. I think I gave a good explanation why I think we should cover all three kinds of calendar anomalies. Can you explain why the one anomaly that happens all the time, on pretty much everybody's system you consider irrelevant? > > I strongly believe that it is our duty to make the non-monotonicity of > > the calendar time scale as managable as possible for admins, and that > > not only includes triggers on DST changes, but in fact the DST changes > > are probably the most important kind of anomaly on the calendar scale, > > since even a completely NTP controlled, physically fixed system will > > experience them regularly, in contrast to the other kinds of > > anomalies. > > DST support works all fine already today. I don't see any need for > parsing the tz files here to solve an actual existing problem. True. And sysvinit worked "fine already today" too. It's not a question on wether something works fine or not, it's a question of actually solving real problems. If you think that DST handling for timer triggers is not a problem, then you live in a very simple world. > > So yeah, Kay, I think this should be readded and be made useful for > > timer units. And if we make use of this for the timer units we might > > as well show it in timedatectl... > > No, it shouldn't. > > We should stay out of the DST business. Glibc calculates everything > just fine for all needed tasks and we arm the timers accordingly. > There is no need to fiddle with the raw tzfile data here. Well, it's good that things are so simple for you. But calendar time handling is really not that simple. You *have* to think of DST. And if you don't you just avoid dealing with the complexity it brings. You know, Kay, DST is a real thing. It's an ugly thing, and we all wish it would not exist. But well, it does, and we better deal with it and make it managable. Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [systemd-commits] Makefile.am src/shared src/timedate
On Thu, Mar 26, 2015 at 11:48 AM, Lennart Poettering wrote: > On Tue, 24.03.15 07:04, Kay Sievers (k...@kemper.freedesktop.org) wrote: > >> Makefile.am|2 >> src/shared/time-dst.c | 329 >> - >> src/shared/time-dst.h | 26 --- >> src/timedate/timedatectl.c | 56 --- >> 4 files changed, 413 deletions(-) >> >> New commits: >> commit 16c6ea29348ddac73998f339166f863bee0dfef6 >> Author: Kay Sievers >> Date: Tue Mar 24 13:52:04 2015 +0100 >> >> timedate: remove daylight saving time handling and tzfile parser >> >> We planned to support (the conceptually broken) daylight saving >> time/local time features in the kernel, SCSI, networking, FAT >> filesystem, but it turned out to be a race we cannot win and do >> not want to get involved. Systemd should not fiddle with daylight >> saving time or parse timezone information itself. >> >> Leave everything to glibc or tools like date(1) and do not make any >> promises or raise expectations that systemd should handle anything >> like this. > > For what's it worth, I strongly disagree with the removal of this. > > We do provide OnCalendar= triggers in timer units. They trigger on > calendar time, not on UTC time, and are hence subject to local clock > changes, to local timezone changes, and to the DST changes of the > local timezone. Their time scale is non-linear due to this: it > *mostly* is linear, but under any of these three conditions it will > not be linear anymore, it will jump. > > Now, to make calendar time triggers complete I think we must enable > people to at least trigger on threse three kinds of anomalies in the > time scale. As such I think it would make a *ton* of sense to add: > > OnTimeZoneChange= > OnClockChange= These are useful, sure. > OnDSTChange= This is absolutely not needed and we should not get into that business. > settings to .timer units, so that users can explictly watch for any of > them, either separately of the actual calendar expressions, or in > addition to them. > > If we do have all four of OnCalendar=, OnTimeZoneChange=, > OnClockChange= and OnDSTChange= in place, then for the first time we > actually can express timer events in a complete way, and the anomalies > of the wallclock time scale becomes *manageable*, for the first time. > > Just removing this code blindly appears misguided to me. It just means > sticking the head in the sand, ignoring completely the fact that we > *do* already provide OnCalendar= timer units, and ignoring that their > time scale is so weirdly non-monotonic. And we leave our users in the > cold, because we provide them with no way to manage the fuckup that > DST is. > > I strongly believe that it is our duty to make the non-monotonicity of > the calendar time scale as managable as possible for admins, and that > not only includes triggers on DST changes, but in fact the DST changes > are probably the most important kind of anomaly on the calendar scale, > since even a completely NTP controlled, physically fixed system will > experience them regularly, in contrast to the other kinds of > anomalies. DST support works all fine already today. I don't see any need for parsing the tz files here to solve an actual existing problem. > So yeah, Kay, I think this should be readded and be made useful for > timer units. And if we make use of this for the timer units we might > as well show it in timedatectl... No, it shouldn't. We should stay out of the DST business. Glibc calculates everything just fine for all needed tasks and we arm the timers accordingly. There is no need to fiddle with the raw tzfile data here. Kay ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [systemd-commits] Makefile.am src/shared src/timedate
On Tue, 24.03.15 07:04, Kay Sievers (k...@kemper.freedesktop.org) wrote: > Makefile.am|2 > src/shared/time-dst.c | 329 > - > src/shared/time-dst.h | 26 --- > src/timedate/timedatectl.c | 56 --- > 4 files changed, 413 deletions(-) > > New commits: > commit 16c6ea29348ddac73998f339166f863bee0dfef6 > Author: Kay Sievers > Date: Tue Mar 24 13:52:04 2015 +0100 > > timedate: remove daylight saving time handling and tzfile parser > > We planned to support (the conceptually broken) daylight saving > time/local time features in the kernel, SCSI, networking, FAT > filesystem, but it turned out to be a race we cannot win and do > not want to get involved. Systemd should not fiddle with daylight > saving time or parse timezone information itself. > > Leave everything to glibc or tools like date(1) and do not make any > promises or raise expectations that systemd should handle anything > like this. For what's it worth, I strongly disagree with the removal of this. We do provide OnCalendar= triggers in timer units. They trigger on calendar time, not on UTC time, and are hence subject to local clock changes, to local timezone changes, and to the DST changes of the local timezone. Their time scale is non-linear due to this: it *mostly* is linear, but under any of these three conditions it will not be linear anymore, it will jump. Now, to make calendar time triggers complete I think we must enable people to at least trigger on threse three kinds of anomalies in the time scale. As such I think it would make a *ton* of sense to add: OnTimeZoneChange= OnClockChange= OnDSTChange= settings to .timer units, so that users can explictly watch for any of them, either separately of the actual calendar expressions, or in addition to them. If we do have all four of OnCalendar=, OnTimeZoneChange=, OnClockChange= and OnDSTChange= in place, then for the first time we actually can express timer events in a complete way, and the anomalies of the wallclock time scale becomes *manageable*, for the first time. Just removing this code blindly appears misguided to me. It just means sticking the head in the sand, ignoring completely the fact that we *do* already provide OnCalendar= timer units, and ignoring that their time scale is so weirdly non-monotonic. And we leave our users in the cold, because we provide them with no way to manage the fuckup that DST is. I strongly believe that it is our duty to make the non-monotonicity of the calendar time scale as managable as possible for admins, and that not only includes triggers on DST changes, but in fact the DST changes are probably the most important kind of anomaly on the calendar scale, since even a completely NTP controlled, physically fixed system will experience them regularly, in contrast to the other kinds of anomalies. So yeah, Kay, I think this should be readded and be made useful for timer units. And if we make use of this for the timer units we might as well show it in timedatectl... Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] parsing audit messages
On Sun, 15.03.15 03:51, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote: > On Sun, Mar 15, 2015 at 03:49:07AM +0100, Zbigniew Jędrzejewski-Szmek wrote: > > Hi, > > > > I was looking at some debug logs, and the audit messages are > > semi-useless in their current undecoded form: > > > > mar 14 22:24:02 fedora22 audit[1]: pid=1 uid=0 auid=4294967295 > > ses=4294967295 subj=system_u:system_r:init_t:s0 > > msg='unit=systemd-udev-trigger comm="systemd" > > exe="/usr/lib/systemd/systemd" hostname=? addr=? terminal=? res=success' > > mar 14 22:24:05 fedora22 audit: > > proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D0069707461626C655F7365637572697479 > > > > You added code to parse this, and I think we should make use of it and > > put msg= field as MESSAGE=, and maybe store the original message as > > _AUDIT= or something. If there's no msg field, like with proctitle, > > print all fields that are in the message, but using our cescape, and > > not this hexadecimal form which is unreadable for humans. > > I think we should also translate type= to names... > > https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/6/html/Security_Guide/sec-Audit_Record_Types.html Well, we don't translate MESSAGE_ID fields to strings either... Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] parsing audit messages
On Sun, 15.03.15 03:49, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote: > Hi, > > I was looking at some debug logs, and the audit messages are > semi-useless in their current undecoded form: > > mar 14 22:24:02 fedora22 audit[1]: pid=1 uid=0 auid=4294967295 > ses=4294967295 subj=system_u:system_r:init_t:s0 > msg='unit=systemd-udev-trigger comm="systemd" exe="/usr/lib/systemd/systemd" > hostname=? addr=? terminal=? res=success' > mar 14 22:24:05 fedora22 audit: > proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D0069707461626C655F7365637572697479 > > You added code to parse this, and I think we should make use of it and > put msg= field as MESSAGE=, and maybe store the original message as > _AUDIT= or something. If there's no msg field, like with proctitle, > print all fields that are in the message, but using our cescape, and > not this hexadecimal form which is unreadable for humans. > > Thoughts? Well "msg=" is just where they place the userspace message, if it is a userspace generated message. It is little more than a separator between the kernel generated and userspace generated parts of the message. The userspace message is generally not more or less human readable than the whole message I fear... I am all for making the audit parsing logic smarter, but I don't see how that's possible, the kernel generated format is a complete disaster, the people who wrote that had no concept at all of computer security, and its' impossible to parse fully correctly without heuristics. For example, if we encounter 2proctitle=41" in the message, we simply don't know whether this is actually a process called "41", or just the hex encoded process name "A"... The formatting is not reversible. It's complete rubbish. It's an embarassment for the kernel community that a technology like audit -- that is supposed to improve security -- is so vulnerable to the most trivial script-kiddy attacks! I am not sure we can do much about this really... Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] CODING_STYLE: this also help with unaligned memory accesses
On Tue, 24.03.15 11:16, Shawn Landden (sh...@churchofgit.com) wrote: > And those arches don't get much testing too. > --- > CODING_STYLE | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/CODING_STYLE b/CODING_STYLE > index b687e72..8934954 100644 > --- a/CODING_STYLE > +++ b/CODING_STYLE > @@ -14,7 +14,8 @@ > - The destructors always unregister the object from the next bigger >object, not the other way around > > -- To minimize strict aliasing violations, we prefer unions over casting > +- To minimize strict aliasing violations and unaligned memory accesses, > + we prefer unions over casting Unaligned memory accesses are an orthogonal problem really. I don't see how the change above really makes sense? Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [HEADS-UP] hwdb: change 60-keyboard.hwdb matches
Hey David, (sorry for late reply, back from holidays) David Herrmann [2015-03-16 12:31 +0100]: > I've pushed some patches that change the hwdb 60-keyboard matching > logic. This was needed to support bluetooth devices, and then I just > went ahead and cleaned up the other rules. This email is meant as a > heads-up to you guys, as you are the most frequent contributors to > that file. Thanks for the heads-up! > Two notes: > > 1) Both of you added keyboard:usb:* matches that match on the USB > interface. This was now dropped in: > > http://cgit.freedesktop.org/systemd/systemd/commit/?id=b26e4ced91d0ac0eabdce1c505228ccafc65a23f > If this was intentional, we can re-add it, but it looked like a > safety-net to me, rather than an explicit interface match. Please let > me know if it is required! Some USB devices (Logitech and/or Microsoft, I don't remember any more) do need an interface match; e. g. some USB keyboards also include a mouse/touchpad and thus expose two input devices, and we must ensure to apply the keymap to the keyboard, not to the mouse. However, this does not apply to the Compaq ones, so this commit looks ok. It indeed makes much more sense to match on the input devices than on the USB ones, so handling the multi-function device case should be much easier now. > 2) The keyboard:dmi:* matches are now merged with the platform > fallback, as the platform-fallback is a superset of the dmi-match: > > http://cgit.freedesktop.org/systemd/systemd/commit/?id=ba76ee29bc02879fb42c048132af8889b00220d5 Looks fine. Thanks! Martin -- Martin Pitt| http://www.piware.de Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org) ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] Most important changes the last 15 months
On Mon, 16.03.15 08:05, Cecil Westerhof (cldwester...@gmail.com) wrote: > About 15 months ago I gave a presentation about systemd/journald. I am > asked to give another presentation about systemd/journald. What are the > most important changes I should integrate into my presentation? http://cgit.freedesktop.org/systemd/systemd/tree/NEWS Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/2] macro: allow assert_se() assertions to also be optimized out when NDEBUG is set
On Tue, 24.03.15 11:11, Shawn Landden (sh...@churchofgit.com) wrote: > Will result in slightly smaller binaries, and cuts out the branch, even if > the expression is still executed. I am sorry, but the whole point of assert_se() is that it has side effects. That's why it is called that way (the "se" suffix stands for "side effects"), and is different from assert(). You are supposed to use it whenever the code enclosed in it shall not be suppressed if NDEBUG is defined. This patch of yours breaks that whole logic! Lennart > --- > src/shared/macro.h | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/src/shared/macro.h b/src/shared/macro.h > index 7f89951..02219ea 100644 > --- a/src/shared/macro.h > +++ b/src/shared/macro.h > @@ -212,17 +212,17 @@ static inline unsigned long ALIGN_POWER2(unsigned long > u) { > (__x / __y + !!(__x % __y));\ > }) > > -#define assert_se(expr) \ > -do {\ > -if (_unlikely_(!(expr)))\ > -log_assert_failed(#expr, __FILE__, __LINE__, > __PRETTY_FUNCTION__); \ > -} while (false) \ > - > /* We override the glibc assert() here. */ > #undef assert > #ifdef NDEBUG > +#define assert_se(expr) do {expr} while(false) > #define assert(expr) do {} while(false) > #else > +#define assert_se(expr) \ > +do {\ > +if (_unlikely_(!(expr)))\ > +log_assert_failed(#expr, __FILE__, __LINE__, > __PRETTY_FUNCTION__); \ > +} while (false) > #define assert(expr) assert_se(expr) > #endif > > -- > 2.2.1.209.g41e5f3a > > ___ > systemd-devel mailing list > systemd-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/systemd-devel Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel