[Pm-utils] [RFC] [patch review] Hook independence and security fixups, part 1

[Victor Lowther]

On Feb 2, 2008 6:41 PM, Dan Nicholson <dbn.lists at gmail.com> wrote:
>
> On Feb 2, 2008 3:22 PM, Victor Lowther <victor.lowther at gmail.com> wrote:
> > On Feb 2, 2008 4:32 PM, Victor Lowther <victor.lowther at gmail.com> wrote:
> > > Hmmm... how about something along these lines:
> > >
> > > @PM-UTILS-LIBDIR@/methods.d/, which is populated by files named
> > >   suspend_method-suspend_type (kernel-suspend, kernel-hibernate,
> > > tuxonice-suspend-hybrid, etc.)
> > >   These files accept a single argument, which (right now) can be
> > > either "inhibited" or "", and output either of "resume" or "thaw" on
> > > stdout, depending on whether the system is resuming from suspend or
> > > hibernate.
> > >
> > > All the do_suspend_type functions then collapse into a single
> > > do_sleep() function, which is passed either "hibernate", "suspend", or
> > > "suspend-hybrid".  The do_sleep method then calls
> > > @PM-UTILS-LIBDIR@/method.d/${SUSPEND_METHOD}-$1 with the appropriate
> > > value of inhibit
> > >
> > > That way, there is no patching of case statements needed to add a new
> > > suspend method -- just drop an appropriatly-named script in the
> > > method.d directory, and let it handle doing The Right Thing.
> >
> > Test patch attached.  It may not actually run, and there are a few
> > niceties to add, but it is a first stab at things.  Should apply
> > cleanly to current git head, and is also available as
> > vlowther-pluggable-sleep-methods in my git repo.
>
> I don't see many flaws in your patch, except that you have methoddir =
> .../config.d and you removed the zzz hook, which doesn't have to do
> with this change.

hm?  removing zzz is quite relevant, all it did was actually perform
the suspend/hibernate/whatever, and with this patch we no longer need
it.

> My personal feeling is that the methods.d stuff is
> adding more complexity than the gain it provides. There's definitely a
> benefit to being able to drop in a new file and change a config
> variable to use a new suspend backend. However, passing a command to
> run_hooks that it will expand is not something I find particularly
> attractive.

Heh, I guess I come too much of a functional programming background --
passing a curried function to another function seems like a perfectly
obvious and noncontroversial thing to do. sh just has weird syntax for
doing it. :)

> The alternative that I suggested before of just having a case
> statement with the accepted methods is conceptually much simpler,
> especially for reading and understanding the code. In this case,
> adding tuxonice support means adding one option to a case statement
> and one or two functions.

Right now there are 3 mainstream backends for implementing sleep --
the swsusp code built into the kernel, the tuxonice code Nigel
maintains, and the uswsusp code.  If we maintain compatibility with
all of them, that comes out to 8 cases that we have to worry about in
the pm-utils code.   With the modular backend, we don't have to worry
about any of it in the main code, and we provide the swsusp modules as
API demos.

>I would also hope that the number of suspend
> methods is decreasing to the point where having a modular backend
> doesn't provide much gain.

Hmmm... with the modular backend, we can let Nigel, Rafael, and the
distros worry about making sure that the actual backend sleep/wakeup
modules do The Right Thing without having to worry about our code, and
we can more easily adapt to whatever Next Great Revolution in
suspend/resume technology comes about.

> Just my thoughts.