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

[Gonzalo Odiard]

On Thu, May 10, 2012 at 12:58 AM, James Cameron <quozl at laptop.org> wrote:

> 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.
>
>
I agree.
I really was surprised we do not have other cases of inhibit suspend in
sugar,
probably the implementation used in stopwatch/powerd.py is better in the
long run.

Thanks by the review, I will send another patch.

Gonzalo



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



-- 
Gonzalo Odiard
SugarLabs Argentina
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20120510/762fa15b/attachment.html>