Merge lp://staging/~sylvain-pineau/checkbox-core/bug983035 into lp://staging/checkbox-core

Excellent work looking info fileobject.c! I just commented on the bug linked to this branch a couple minutes ago, do you think we should add more methods to this story?

A new proposal based this time on io.FileIO

io.FileIO is now used as a base class to checkbox.lib.file

I've fixed classes that used the old file interface to match the new way we built file objects.

Nice work, I'm very pleased about removing the create_from_* class methods and using None instead of an empty File instance! First, I don't think this logic is the same as before:

324 - if (self._user_enable
325 - and not self._openPath(path, self._file, lock=self._lock)):
326 + self._file = self._openPath(path, lock=self._lock)
327 + if (self._user_enable and not self._file):

Before, _openPath was only called if _user_enable is True. Now, _openPath is always called. Second, I think this code is a bit ambiguous because self._writer is not expected to be boolean:

141 + return self._writer == True

Even though your code probably works, I think it's more obvious to say something like: return self._writer is not None. Last, but not least, what would you think about changing Fifo.create_fifo to just be a function, instead of a class method, that would return a reader and writer tuple?

Thanks for the review, I've made the suggested changes, both in code and tests.

This code will raise an AttributeError when calling the close() method on a None object:

394 + file = create_file(fd, mode)
395 + if not file:
396 + file.close()

By removing the decorators in checkbox.lib.file, the file object no longer contains the errno attribute used like this in checkbox.daemon.pidentry for example:

  elif buffer is None \
       and file.errno != errno.EWOULDBLOCK \
       and file.errno != errno.EAGAIN:
      logging.info("Read failed for pid %d: %d", pipe_fd, file.errno)
      return False

A quick grep returned other places where file.errno is used. We might either want to reintroduce the previous decorators, but that still won't work in places like in checkbox.journal.multi where errno will be called on a None object. How would you address this problem?

By the way, I really like this unit test which clearly shows that the new behavior of fifo_create is much more elegant:

- read_file = File()
- write_file = File()
-
- self.assertTrue(fifo_create(self.path, read_file, write_file))
- self.assertNotEquals(read_file.fileno(), -1)
- self.assertNotEquals(write_file.fileno(), -1)
+ reader, writer = fifo_create(self.path)
+ self.assertNotEquals(reader.fileno(), -1)
+ self.assertNotEquals(writer.fileno(), -1)

I just thought of a solution to solve the problem of having create_file somehow return an errno: instead of returning None when an error occurs, we could return the negative errno:

  file = create_file(path)
  if file < 0:
    logging.error("Failed to open file '%s'; errno = %d", path, -file)

This will work when create_file returns an actual object, so the code is safe. The only problem is that the value assigned to file is either an object or a negative integer. This might sound strange but it's not that much diffferent from returning None. For example, both a negative integer and None will raise an AttributeError when attempting to call file.read() when create_file fails. What do you think?

It's probably the best we can do to keep the calling code quite simple.

I was thinking to reintroduce the errno attribute to the File class, if the file object creation fails, returns the class and check for the errno. Given that the application if not threaded, it could be reliable:

class File(io.FileIO):

    __slots__ = (
        "errno",
        )

    def __init__(self, *args, **kwargs):
        super(File, self).__init__(*args, **kwargs)
        self.errno = 0

[...]

def create_file(*args, **kwargs):
    try:
        return File(*args, **kwargs)
    except (EnvironmentError, ValueError), error:
        f = File
        f.errno = error.errno
        return f

Even if I prefer your proposal, what do you think of mine ?