HiveBrain v1.2.0
Get Started
← Back to all entries
debugrubyMinor

Need reviews for impacts of dirty bug fix

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
fixbugreviewsneedforimpactsdirty

Problem

I was having a hard time with bug logstash#665. Hope they will fix it some day, but sparc/solaris support is probably not on top of their priority list. For further reference, original error is:

NotImplementedError: stat.st_dev unsupported or native support failed to load dev_major at org/jruby/RubyFileStat.java:190
_discover_file at /tmp/aaaa/logstash-1.4.1.dev/vendor/bundle/jruby/1.9/gems/filewatch-0.5.1/lib/filewatch/watch.rb:140


After digging a lot I found out that the problem here is the usage of a native library which is not correclty retrieved on Solaris. So when ruby fails, it is trying a fallback to java, which fails as well. Now I have retrieved the original watch.rb source file and isolated the problematic function:

private
def _discover_file(path, initial=false)
  globbed_dirs = Dir.glob(path)
  @logger.debug("_discover_file_glob: #{path}: glob is: #{globbed_dirs}")
  if globbed_dirs.empty? && File.file?(path)
    globbed_dirs = [path]
    @logger.debug("_discover_file_glob: #{path}: glob is: #{globbed_dirs} because glob did not work")
  end
  globbed_dirs.each do |file|
    next if @files.member?(file)
    next unless File.file?(file)

    @logger.debug("_discover_file: #{path}: new: #{file} (exclude is #{@exclude.inspect})")

    skip = false
    @exclude.each do |pattern|
      if File.fnmatch?(pattern, File.basename(file))
        @logger.debug("_discover_file: #{file}: skipping because it " +
                      "matches exclude #{pattern}")
        skip = true
        break
      end
    end
    next if skip

    stat = File::Stat.new(file)
    @files[file] = {
      :size => 0,
      :inode => [stat.ino, stat.dev_major, stat.dev_minor],
      :create_sent => false,
    }
    if initial
      @files[file][:initial] = true
    end
  end
end # def _discover_file


More precisely, the bug was happening there:

```
stat = File::Stat.new(file)
@files[file] = {
:size => 0,
:inode => [stat.ino, stat.dev_major, stat.dev_

Solution

In Unix filesystems, an inode is a number that uniquely identifies a file within the filesystem. (More strictly speaking, an inode is a data structure that holds metadata about a file within the filesystem; each inode is assigned an inode number, and the number is all you care about unless you're a kernel developer.)

However, the inode number only uniquely identifies a file within its filesystem. If /tmp and /usr are separate filesystems, inode 3461 of the /tmp filesystem might be /tmp/.ICE-unix while inode 3461 on /usr might be /usr/share/dict/words. Therefore, to uniquely identify a file on the whole computer, a tuple of (inode number, device major number, device minor number) is needed.

By ignoring the device number, you might end up acting on events on a file with a coincidentally equal inode number on a different filesystem — though the chance is miniscule.

If you decide to keep this workaround, then I think you should make a corresponding change on line 71 as well.

Context

StackExchange Code Review Q#46922, answer score: 4

Revisions (0)

No revisions yet.