[Sugar-devel] [PATCH Sugar] Inhibit power suspend while playing text to speech - OLPC #11830

[James Cameron]

Regarding your general approach, this is okay for the moment, since
nothing else in the Sugar shell process will use it, but watch out in
future.

/var/run/powerd-inhibit-suspend/%s is unique by process id only.

See stopwatch.git/powerd.py for a reference counted implementation
that would avoid this.

In my opinion a reference counted implementation should be added to
the Sugar toolkit for use by activities as well as Sugar.

Reviewed-by: James Cameron <quozl at laptop.org>

On Wed, May 09, 2012 at 02:39:02PM -0300, godiard at sugarlabs.org wrote:
> +    def _verify_powerd_running(self):
> +        self.using_powerd = os.access(POWERD_INHIBIT_DIR, os.W_OK)
> +        logging.debug("using_powerd: %d", self.using_powerd)
> +        return self.using_powerd

This function should not use the word "running", since it isn't
actually testing if powerd is running, it only tests if powerd inhibit
directory is present.

There is no need to verify that powerd is present.  Just try to create
or delete the files and ignore any IOError.

> +
> +    def _inhibit_suspend(self):
> +        if self.using_powerd:
> +            flag_file_name = POWERD_INHIBIT_DIR + "/%u" % os.getpid()
> +            if not os.path.exists(flag_file_name):
> +                fd = open(flag_file_name, 'w')
> +                logging.debug("inhibit_suspend file is %s" % flag_file_name)
> +                fd.close()

This only creates the file if it does not exist, and is a potential race
condition.  You should just create the file, for example as done in
distance.git/activity.py or stopwatch.git/powerd.py

> +
> +    def _allow_suspend(self):
> +        if self.using_powerd:
> +            flag_file_name = POWERD_INHIBIT_DIR + "/%u" % os.getpid()
> +            if os.path.exists(flag_file_name):
> +                os.unlink(flag_file_name)
> +            logging.debug("allow_suspend unlinking %s" % flag_file_name)
> +

Again, there is no need to check that it exists before unlinking it.

The determination of flag_file_name might be placed into a common
function, as done in stopwatch/powerd.py

-- 
James Cameron
http://quozl.linux.org.au/