alive.chmod(Time.now.to_i) raises Errno::EINVAL on OpenBSD

alive.chmod(Time.now.to_i) raises Errno::EINVAL on OpenBSD

[Jeremy Evans]

On OpenBSD:

$ ruby -e "File.new('TODO').chmod(Time.now.to_i)"
-e:1:in `chmod': Invalid argument - TODO (Errno::EINVAL)
        from -e:1

This is explained in the man page:

     int
     fchmod(int fd, mode_t mode);

    ...

     [EINVAL]      mode contains bits other than the file type and those de-
                   scribed above.

I think 04777 is the highest allowed mode on OpenBSD.  Time.now.to_i
is obviously higher than that.

Here's a diff that should fix the problem.  At the very least it
allows the workers to start without crashing:

diff --git a/lib/unicorn.rb b/lib/unicorn.rb
index ddec8e9..092f500 100644
--- a/lib/unicorn.rb
+++ b/lib/unicorn.rb
@@ -579,13 +579,13 @@ module Unicorn
         # changes with chmod doesn't update ctime on all filesystems; so
         # we change our counter each and every time (after process_client
         # and before IO.select).
-        t == (ti = Time.now.to_i) or alive.chmod(t = ti)
+        t == (ti = Time.now.to_i) or (t = ti;
alive.chmod(alive.stat.mode ^ 0100))

         ready.each do |sock|
           begin
             process_client(sock.accept_nonblock)
             nr += 1
-            t == (ti = Time.now.to_i) or alive.chmod(t = ti)
+            t == (ti = Time.now.to_i) or (t = ti;
alive.chmod(alive.stat.mode ^ 0100))
           rescue Errno::EAGAIN, Errno::ECONNABORTED
           end
           break if nr < 0

There are definitely other ways that will work, as long as the mode is
kept between 0 and 04777.

Jeremy

Re: alive.chmod(Time.now.to_i) raises Errno::EINVAL on OpenBSD

[Eric Wong]

Jeremy Evans <jeremyevans0@gmail.com> wrote:
> On OpenBSD:
> 
> $ ruby -e "File.new('TODO').chmod(Time.now.to_i)"
> -e:1:in `chmod': Invalid argument - TODO (Errno::EINVAL)
>         from -e:1
> 
> This is explained in the man page:
> 
>      int
>      fchmod(int fd, mode_t mode);
> 
>     ...
> 
>      [EINVAL]      mode contains bits other than the file type and those de-
>                    scribed above.
> 
> I think 04777 is the highest allowed mode on OpenBSD.  Time.now.to_i
> is obviously higher than that.

Yikes.  I was worried about the portability of this.  I actually redid
this in the Revactor model of Rainbows! to flip between 0 and 1, I'll do
that in Unicorn, too.

> Here's a diff that should fix the problem.  At the very least it
> allows the workers to start without crashing:
> 
> diff --git a/lib/unicorn.rb b/lib/unicorn.rb
> index ddec8e9..092f500 100644
> --- a/lib/unicorn.rb
> +++ b/lib/unicorn.rb
> @@ -579,13 +579,13 @@ module Unicorn
>          # changes with chmod doesn't update ctime on all filesystems; so
>          # we change our counter each and every time (after process_client
>          # and before IO.select).
> -        t == (ti = Time.now.to_i) or alive.chmod(t = ti)
> +        t == (ti = Time.now.to_i) or (t = ti;
> alive.chmod(alive.stat.mode ^ 0100))

I think the Time.now.to_i bit is overkill (and probably detrimental to
performance on non-x86_64, non-VDSO-enabled, non-GNU/Linux machines).

> There are definitely other ways that will work, as long as the mode is
> kept between 0 and 04777.

Here's what I committed and pushed out, it should work for all *nix
now since it only alternates between 0 and 1, but be sure to let
me know if it doesn't for some reason.  Thanks!

>From 24a1b4c6b5fcb948e4fdee04e286c044d6d45f98 Mon Sep 17 00:00:00 2001
From: Eric Wong <normalperson@yhbt.net>
Date: Thu, 8 Oct 2009 19:56:29 -0700
Subject: [PATCH] fchmod heartbeat flips between 0/1 for compatibility

This removes the Time.now.to_i comparison that was used to avoid
multiple, no-op fchmod() syscalls[1] within the same second.

This should allow us to run on OpenBSD where it can raise EINVAL
when Time.now.to_i is passed to it.

Reported-by: Jeremy Evans <jeremyevans0@gmail.com>

[1] - gettimeofday() from Time.now is not a real syscall on
VDSO-enabled x86_64 GNU/Linux systems where Unicorn is primarily
developed.
---
 lib/unicorn.rb |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/unicorn.rb b/lib/unicorn.rb
index d63567f..13c203a 100644
--- a/lib/unicorn.rb
+++ b/lib/unicorn.rb
@@ -575,13 +575,13 @@ module Unicorn
       nr = 0 # this becomes negative if we need to reopen logs
       alive = worker.tmp # tmp is our lifeline to the master process
       ready = LISTENERS
-      t = ti = 0
 
       # closing anything we IO.select on will raise EBADF
       trap(:USR1) { nr = -65536; SELF_PIPE.first.close rescue nil }
       trap(:QUIT) { alive = nil; LISTENERS.each { |s| s.close rescue nil } }
       [:TERM, :INT].each { |sig| trap(sig) { exit!(0) } } # instant shutdown
       logger.info "worker=#{worker.nr} ready"
+      m = 0
 
       begin
         nr < 0 and reopen_worker_logs(worker.nr)
@@ -595,13 +595,13 @@ module Unicorn
         # changes with chmod doesn't update ctime on all filesystems; so
         # we change our counter each and every time (after process_client
         # and before IO.select).
-        t == (ti = Time.now.to_i) or alive.chmod(t = ti)
+        alive.chmod(m = 0 == m ? 1 : 0)
 
         ready.each do |sock|
           begin
             process_client(sock.accept_nonblock)
             nr += 1
-            t == (ti = Time.now.to_i) or alive.chmod(t = ti)
+            alive.chmod(m = 0 == m ? 1 : 0)
           rescue Errno::EAGAIN, Errno::ECONNABORTED
           end
           break if nr < 0
@@ -614,7 +614,7 @@ module Unicorn
         redo unless nr == 0 # (nr < 0) => reopen logs
 
         ppid == Process.ppid or return
-        alive.chmod(t = 0)
+        alive.chmod(m = 0 == m ? 1 : 0)
         begin
           # timeout used so we can detect parent death:
           ret = IO.select(LISTENERS, nil, SELF_PIPE, timeout) or redo
-- 
Eric Wong