Moving an object with a file descriptor

Question

I have a class, say Foo, defined like this:

class Foo {
public:
  Foo(int i) {
    valid = false;
    char devname[] = "/dev/device0";
    sprintf(devname, "/dev/device%d", i);
    fd = open(devname, O_RDWR);
    if (fd > 0) {
      valid = true;
    }

  ~Foo() {
    if (valid) {
      close(fd);
    }
  }

  bool valid;
private:
  int fd;
};

I have another class, say Bar, defined like this:

class Bar {
public:
  Bar() {
    for (int i = 0; i < 4; i++) {
      Foo foo(i);
      if (foo.valid) {
        vector_of_foos.push_back(foo);
      }
    }
  }

  std::vector<Foo> vector_of_foos;
};

The problem with this is that push_back makes a copy of the Foo object, which copies the fd property. Then the destructor for the original Foo object is called, which closes the file that fd points to, rendering fd invalid.

Unfortunately I cannot use emplace_back since I need to instantiate my Foo object before adding it to the vector_of_foos vector so that I can check the valid property.

I have also tried using std::move but this still calls the destructor for the original Foo object once it goes out of scope which closes the file.

What is the recommended way of managing a resource like this? Should I use an array of smart pointers? Should my vector_of_foos be instead a std::vector<Foo *> where I maintain a vector of pointers to Foo which I allocate dynamically?

Answer 1

Foo needs a copy constructor and copy assignment operator so it can dup() the fd of the copied-from object (or, you need to delete them so Foo objects can't be copied at all, only moved).

When implementing move semantics, after moving the fd value from the moved-from object to the moved-to object, you need to update the moved-from object so its fd no longer refers to a valid file descriptor. Simply set it to -1, which is what open() and dup() return on error.

You don't need your valid member at all. That is a source of error waiting to happen if it ever gets out of sync with your fd.

Try something more like this:

class Foo {
public:
  Foo(int i) {
    std::string devname = "/dev/device" + std::to_string(i);
    fd = open(devname.c_str(), O_RDWR);
  }

  Foo(const Foo &src) {
    fd = dup(src.fd);
  }
  // or: Foo(const Foo &) = delete;

  Foo(Foo &&src) : fd(-1) {
    src.swap(*this);
  }

  ~Foo() {
    if (fd != -1) {
      close(fd);
    }
  }

  bool valid() const {
    return (fd != -1); 
  }

  Foo& operator=(Foo rhs) {
    rhs.swap(*this);
    return *this;
  }
  // optional: Foo& operator=(const Foo &) = delete;

  void swap(Foo &other) {
    std::swap(fd, other.fd);
  }

private:
  int fd;
};

Answer 2

n.m beat me to the punch, but you need to define a move constructor that 'forgets' about your file descriptor in the moved-from object. Something like:

Foo (Foo &&move_from)
{
    valid = move_from.valid;
    fd = move_from.fd;
    move_from.valid = false;
}

Also, delete your copy constructor and copy assignment operator and implement a move-assignment operator. You then have an object that can be moved but not copied, rather like std::unique_ptr.

Answer 3

You probably want to delete copy constructors altogether (or provide some logic to have two valid instances with different descriptors). If you delete them, compiler will make sure you use move semantics always.

Then, you can declare move constructors, that will perform valid moving:

class Foo {
public:
  Foo(int i) {
    valid = false;
    char devname[] = "/dev/device0";
    sprintf(devname, "/dev/device%d", i);
    fd = open(devname, O_RDWR);
    if (fd > 0) {
      valid = true;
    }

  ~Foo() {
    if (valid) {
      close(fd);
    }
  }

  Foo(const Foo&) = delete;
  Foo& operator=(const Foo&) = delete;
  Foo(Foo&& other) {
    valid = other.valid;
    fd = other.fd;
    other.valid = false;
  };
  Foo& operator=(Foo&& other) {
    valid = other.valid;
    fd = other.fd;
    other.valid = false;
  };

  bool valid;
private:
  int fd;
};

A bit of explanation:

You violated the Rule of Three/Five/Zero. In short, it says "whenever your class needs a user defined destructor/copy constructor/move constructor, it will most likely the rest of them". Your class does require a destructor, which you provided, but you didn't provide neither copy or move operators, which is a mistake. You have to provide both copy constructor/copy assignment operator (here deleted) and move constructor/move assignment operator.

As to why: compiler is not smart enough to guess what do you need. Ignoring the fact that your class doesn't support move semantics (user-defined destructor prevents implicit creation of move constructors), implicit move constructor is very, very simple. It just calls std::move on every class member. And std::move for primitive types is also very simple - it just copies that variable, as it's the fastest operation.
It could possibly zero the other variable, but that's not necessary. As per definition, the variable must be left in "unknown, but valid state". No change to that variable fits this definition.