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

[Dan Nicholson]

On Feb 2, 2008 6:26 PM, Victor Lowther <victor.lowther at gmail.com> wrote:
>
> 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.

Except you didn't actually put the sync back anywhere.

> > 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. :)

With a real programming language, I'd feel more comfortable "passing a
function pointer", but it seems sketchy in sh. More importantly, I
just doing think having a plugin type architecture is worth that added
complexity.

> > 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 think we _do_ want to have all of that in the pm-utils repo. Adding
an API to pm-utils is just not worth the effort of properly designing
it. How are we going to enforce this API? Do we know all the
interfaces that would be needed?

On the other hand, having that code in pm-utils means we can change
the interfaces whenever necessary. And the people that know how
pm-utils works intimately will have their eyes on the backend methods.
Like the kernel and external modules. Yeah, it works, but everyone is
a lot happier when those modules move into mainline.

> >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.

They can also "do the right thing" by adding 40 line patch to
pm-utils. IMO, having an API is overkill, adding complexity and
codepaths to a conceptually simple idea.

--
Dan