[Pm-utils] [patch commit] [000] POSIXification of pm-utils

[Dan Nicholson]

On Jan 27, 2008 6:15 PM, Victor Lowther <victor.lowther at gmail.com> wrote:
> On Jan 27, 2008 7:47 PM, Dan Nicholson <dbn.lists at gmail.com> wrote:
> > On Jan 27, 2008 12:25 PM, Victor Lowther <victor.lowther at gmail.com> wrote:
> > > Adds a generic locking mechanism built around directory creation/removal.
> >
> > The timeout support in spin_lock doesn't seem like it would work.
> >
> > +# spin waiting for the lock with optional timeout.
> > +# return once we have it, or the timeout has expired
> > +spin_lock()
> > +{
> > +       # $1 = directory to use as the lock directory
> > +       # $2 = optional timeout
> > +       local elapsed=0
> > +       while ! try_lock $1; do
> > +               sleep 1;
> > +               [ "x$2" != "x" ] && [ $(( $elapsed == $2 )) -ne 0 ] && return 1
> > +       done
> > +}
> >
> > $elapsed never seems to be incremented. I also don't believe that you
> > want to sleep before checking the timeout. This results in a `sleep 1'
> > even if you haven't specified a timeout; i.e., you want to return
> > immediately. Personally, I would write it like this (trying not to let
> > too much personal style seep in):
>
> You are correct -- I did not test that path, as nothing uses the
> timeout functionaloty right now.

No problem, I assumed as much.

> > spin_lock()
> > {
> >   local elapsed=0
> >   local timeout=${2:-0}
> >
> >   while ! try_lock $1; do
> >     [ $timeout -le $elapsed ] && return 1
> >     elapsed=$(( $elapsed + 1 ))
> >     sleep 1
> >   done
> > }
> >
> > What do you think?
>
> Patch against my patch series attached.

Would you mind not setting -u0 when generating patches? I realize it
cuts down on the size of the attachments, but I think the context is
much more useful than the bandwidth savings. Especially for interdiffs
like this, it can be vital to seeing what the change is doing. For
Mozilla, they used to request 8 or 9 lines of context on patches!

Now it updates $elapsed, but it still suffers the problem where you
always `sleep 1'. So, even for the case now where the timeout
functionality is not being utilized, you still delay a second if the
locking failed. Simply moving the sleep after the conditional would
fix that.

Thanks.

--
Dan