[systemd-devel] [PATCH] shutdown: avoid calling `kexec` binary unnessecarily

[Zbigniew Jędrzejewski-Szmek]

On Sat, Feb 14, 2015 at 10:00:54AM -0800, Shawn Landden wrote:
> On Sat, Feb 14, 2015 at 5:54 AM, Zbigniew Jędrzejewski-Szmek <
> zbyszek at in.waw.pl> wrote:
> 
> > On Fri, Feb 13, 2015 at 02:18:07PM -0800, Shawn Landden wrote:
> > > Still use helper when Xen Dom0, to avoid duplicating some hairy code.
> > > So we don't have any logic to load kexec kernels?
> > > ---
> > >  TODO                |  3 ---
> > >  src/core/shutdown.c | 33 ++++++++++++++++++++-------------
> > >  2 files changed, 20 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/TODO b/TODO
> > > index 883b994..68b0af6 100644
> > > --- a/TODO
> > > +++ b/TODO
> > > @@ -85,9 +85,6 @@ Features:
> > >  * maybe introduce WantsMountsFor=? Usecase:
> > >
> > http://lists.freedesktop.org/archives/systemd-devel/2015-January/027729.html
> > >
> > > -* rework kexec logic to use new kexec_file_load() syscall, so that we
> > > -  don't have to call kexec tool anymore.
> > You patch does not really change this. We can still use kexec_file_load()
> > to
> > load a kernel. But removing a call to kexec to actually do the reboot
> > seems good.
> > So I guess this TODO item should stay.
> >
> I would be happy to change that, but I couldn't find any uses of
> kexec_load() or any other calls to the kexec binary in systemd's git.
I think we should try to load the kernel when 'systemctl kexec' is invoked.
Current behaviour of silently falling through to reboot is annoying.

> > One comment below.
> >
> > >  * The udev blkid built-in should expose a property that reflects
> > >    whether media was sensed in USB CF/SD card readers. This should then
> > >    be used to control SYSTEMD_READY=1/0 so that USB card readers aren't
> > > diff --git a/src/core/shutdown.c b/src/core/shutdown.c
> > > index 71f001a..c64c05d 100644
> > > --- a/src/core/shutdown.c
> > > +++ b/src/core/shutdown.c
> > > @@ -350,26 +350,33 @@ int main(int argc, char *argv[]) {
> > >          case LINUX_REBOOT_CMD_KEXEC:
> > >
> > >                  if (!in_container) {
> > > -                        /* We cheat and exec kexec to avoid doing all
> > its work */
> > > -                        pid_t pid;
> > > +                        _cleanup_free_ char *param = NULL;
> > >
> > >                          log_info("Rebooting with kexec.");
> > >
> > > -                        pid = fork();
> > > -                        if (pid < 0)
> > > -                                log_error_errno(errno, "Failed to fork:
> > %m");
> > > -                        else if (pid == 0) {
> > > +                        /* kexec-tools has a bunch of special code to
> > make Xen Dom0 work,
> > > +                         * otherwise it is only doing stuff we have
> > already done */
> > > +                        if (access("/proc/xen", F_OK) == 0) {
> > Wouldn't it be better to use detect_virtualization() here instead of
> > open-coding the check?
> >
>  That would be *way* more code, and I don't have a xen system to check if
> that detects Dom0, which is all that we are interested in (otherwise kexec
> doesn't work in virtualized environments.)
I think your test detects Dom0 and also DomU. detect_virtualization() only
detects (or should only detect) DomU. So d_v() is wrong for this usecase.

So I think your patch is fine, but a comment should be added explaining
that the test is imprecise and covers all xen domains, but this is OK, since
the fallback to real kexec is OK.

Zbyszek