<br><br><div class="gmail_quote">On Thu, May 10, 2012 at 12:58 AM, James Cameron <span dir="ltr"><<a href="mailto:quozl@laptop.org" target="_blank">quozl@laptop.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Regarding your general approach, this is okay for the moment, since<br>
nothing else in the Sugar shell process will use it, but watch out in<br>
future.<br>
<br>
/var/run/powerd-inhibit-suspend/%s is unique by process id only.<br>
<br>
See stopwatch.git/powerd.py for a reference counted implementation<br>
that would avoid this.<br>
<br>
In my opinion a reference counted implementation should be added to<br>
the Sugar toolkit for use by activities as well as Sugar.<br>
<br></blockquote><div><br></div><div>I agree.</div><div>I really was surprised we do not have other cases of inhibit suspend in sugar,</div><div>probably the implementation used in stopwatch/powerd.py is better in the long run.</div>
<div><br></div><div>Thanks by the review, I will send another patch.</div><div><br></div><div>Gonzalo </div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

Reviewed-by: James Cameron <<a href="mailto:quozl@laptop.org">quozl@laptop.org</a>><br>
<div class="im"><br>
On Wed, May 09, 2012 at 02:39:02PM -0300, <a href="mailto:godiard@sugarlabs.org">godiard@sugarlabs.org</a> wrote:<br>
> +    def _verify_powerd_running(self):<br>
> +        self.using_powerd = os.access(POWERD_INHIBIT_DIR, os.W_OK)<br>
> +        logging.debug("using_powerd: %d", self.using_powerd)<br>
> +        return self.using_powerd<br>
<br>
</div>This function should not use the word "running", since it isn't<br>
actually testing if powerd is running, it only tests if powerd inhibit<br>
directory is present.<br>
<br>
There is no need to verify that powerd is present.  Just try to create<br>
or delete the files and ignore any IOError.<br>
<div class="im"><br>
> +<br>
> +    def _inhibit_suspend(self):<br>
> +        if self.using_powerd:<br>
> +            flag_file_name = POWERD_INHIBIT_DIR + "/%u" % os.getpid()<br>
> +            if not os.path.exists(flag_file_name):<br>
> +                fd = open(flag_file_name, 'w')<br>
> +                logging.debug("inhibit_suspend file is %s" % flag_file_name)<br>
> +                fd.close()<br>
<br>
</div>This only creates the file if it does not exist, and is a potential race<br>
condition.  You should just create the file, for example as done in<br>
distance.git/activity.py or stopwatch.git/powerd.py<br>
<div class="im"><br>
> +<br>
> +    def _allow_suspend(self):<br>
> +        if self.using_powerd:<br>
> +            flag_file_name = POWERD_INHIBIT_DIR + "/%u" % os.getpid()<br>
> +            if os.path.exists(flag_file_name):<br>
> +                os.unlink(flag_file_name)<br>
> +            logging.debug("allow_suspend unlinking %s" % flag_file_name)<br>
> +<br>
<br>
</div>Again, there is no need to check that it exists before unlinking it.<br>
<br>
The determination of flag_file_name might be placed into a common<br>
function, as done in stopwatch/powerd.py<br>
<span class="HOEnZb"><font color="#888888"><br>
--<br>
James Cameron<br>
<a href="http://quozl.linux.org.au/" target="_blank">http://quozl.linux.org.au/</a><br>
</font></span></blockquote></div><br><br clear="all"><div><br></div>-- <br>Gonzalo Odiard<br>SugarLabs Argentina<br><br>