FileSystem Utilities wrapper

Question

Related to my code I just put out for review.

I wrote some small helper functions for dealing with interactions with the file system. This is not meant to by any type of "File System" wrapper. I just wanted to put my code into named functions to make the code Column formatting code easy to read and the functions did not seem "Column File" centric so I placed them in a separate class.

FileSystem.h

#ifndef THORSANVIL_FILESYSTEM_FILESYSTEM_H
#define THORSANVIL_FILESYSTEM_FILESYSTEM_H

#include <sys/stat.h>
#include <sys/types.h>
#include <string>
#include <fstream>

// See: https://codereview.stackexchange.com/questions/81922/macro-to-build-type-declaration
// For details about the Traits type and how it is built.

namespace ThorsAnvil::FileSystem
{
    using iostate   = std::ios_base::iostate;
    static constexpr iostate const& goodbit   = std::ios_base::goodbit;
    static constexpr iostate const& badbit    = std::ios_base::badbit;
    static constexpr iostate const& failbit   = std::ios_base::failbit;
    static constexpr iostate const& eofbit    = std::ios_base::eofbit;

    using openmode = std::ios_base::openmode;
    static constexpr openmode const& app      = std::ios_base::app;
    static constexpr openmode const& binary   = std::ios_base::binary;
    static constexpr openmode const& in       = std::ios_base::in;
    static constexpr openmode const& out      = std::ios_base::out;
    static constexpr openmode const& trunc    = std::ios_base::trunc;
    static constexpr openmode const& ate      = std::ios_base::ate;

    // File System Functionality
    struct FileSystem
    {
        enum DirResult {DirAlreadyExists, DirCreated, DirFailedToCreate};
        static DirResult makeDirectory(std::string const& path, mode_t permissions);
        static bool      isFileOpenable(std::string const& path, openmode mode);
        static bool      removeFileOrDirectory(std::string const& path);
    };
}

#endif

FileSystem.cpp

// This header file is generated by autotools when it does all the compiler checks and stuff
// The only interesting thing it defines for this file is the macro HEADER_ONLY_INCLUDE
#include "ThorsStorageConfig.h"
//
// If this file is being used in a header only context this is `inline` otherwise it is blank.
//
// To use this code in a header only context you need to check out the special version:
// git clone --single-branch --branch header-only https://github.com/Loki-Astari/ThorsStorage.git
//
// This branch is auto generated after each successful build by travis.ci


#include "filesystem.h"
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>
#include <stdio.h>

using namespace ThorsAnvil::FileSystem;

// File System Stuff
HEADER_ONLY_INCLUDE
FileSystem::DirResult FileSystem::makeDirectory(std::string const& path, mode_t permissions)
{
    using StatusInfo = struct stat;

    StatusInfo        info;
    for (std::size_t pos = path.find('/'); pos != std::string::npos; pos = path.find(pos + 1, '/'))
    {
        std::string     subPath = path.substr(0, pos);
        if ((stat(subPath.c_str(), &info) != 0) && (mkdir(subPath.c_str(), permissions) != 0))
        {
            return DirFailedToCreate;
        }
    }
    if (stat(path.c_str(), &info) == 0)
    {
        return DirAlreadyExists;
    }

    if (mkdir(path.c_str(), permissions) == 0)
    {
        return DirCreated;
    }
    return DirFailedToCreate;
}

HEADER_ONLY_INCLUDE
bool FileSystem::isFileOpenable(std::string const& path, openmode mode)
{
    bool result = true;
    int accessFlag = ((mode & in) ? R_OK : 0)
                   | ((mode & out)? W_OK : 0);
    if (access(path.c_str(), accessFlag) != 0)
    {
        // This is still OK if we want to open a file for writing as we will be creating it.
        // But to make sure we have permission we have to check three things.
        //  1: The errors for accesses is because the file does not exist.
        //  2: We want to open the file for writing.
        //  3: The directory we want to open the file is writable by this processes.
        //
        //  Otherwise the file is not open-able for the mode we want.
        result = (errno == ENOENT) && (mode & out) && (access(path.substr(0, path.find_last_of('/')).c_str(), W_OK) == 0);
    }
    return result;
}

HEADER_ONLY_INCLUDE
bool FileSystem::removeFileOrDirectory(std::string const& path)
{
    int state = remove(path.c_str());
    return state == 0;
}

Answer 1

Here are some thoughts while I read through your code:

---

I just wanted to put my code into named functions to make the code Column formatting code easy to read and the functions did not seem "Column File" centric so I placed them in a separate class.

Good idea.

---

static constexpr iostate const& goodbit   = std::ios_base::goodbit;

Are all the lines that look like this really necessary? Do you really need to have this stuff in your own namespace?

---

struct FileSystem

This struct has no state. Could be a namespace.

---

using StatusInfo = struct stat;

This kind of thing is a common practice so maybe some readers will like it, but I think it's easier to just see struct stat. One day, someone may refactor your code and move the definition of StatusInfo away from some of the uses. Then any new developers will have to look it up. Also, nothing is stopping you from ending up with uses of struct stat AND StatusInfo mixed together which would be pointless.

Also, stat is an unfortunate name because it's short and generic, but StatusInfo is only longer -- it's still generic. If you have to alias it, how about FileStatusInfo?

---

for (std::size_t pos = path.find('/'); pos != std::string::npos; pos = path.find(pos + 1, '/'))

I see what you're going for. This is fine, but the repeated check for / is not ideal. How about something like:

for (std::size_t pos = -1; pos != std::string::npos; /*in loop*/) {
    pos = path.find(pos + 1, '/');

This might be controversial!

---

path.find('/')

This gives up portability and makes me worry about paths that have // or /./. Maybe these things don't matter for your use case?

---

stat(subPath.c_str(), &info)

What's this intended to do? You never read info. It sounds like you're using it to check if a file/dir exists? stat can fail for a lot of reasons...

---

((stat(subPath.c_str(), &info) != 0) && (mkdir(subPath.c_str(), permissions) != 0))

First off it's good you are checking API return values. But...

Suppose this happens:

1. stat(subPath.c_str(), &info) returns non-zero presumably to say "the file/dir doesn't exist"

2. Another process completely reorganizes the filesystem and permissions.

3. You call mkdir(subPath.c_str(), permissions)

mkdir will fail (and you check for that)... but then what was the point of stat? The idea of looping through parts of a directory has the same problem. You may be able to create one directory, but then you fail. Now the filesystem has a random directory in it.

That might seem like a rare, not-so-bad occurrence. But if you use this function enough times on enough computers, it becomes likely that someone will have a problem because of a bug like that. Weird environments can systematically expose "unlikely" bugs and lead to out-of-memory errors or extremely slow filesystems.

The common solution is to create files and if any of the creations fail, then delete the ones you already created. This is hard to get right but most operating systems have APIs to help.

---

It's a little suspicious that you have a loop of stat && mkdir and then after the loop make the same syscalls. Can those be part of the loop?

---

The idea of isFileOpenable suffers from the same problem as the loop above; the file might be openable when you asked, but it might become un-openable immediately after that.

It's much easier to handle filesystem failures than to try and lock files, check their status, do things, then unlock.

---

removeFileOrDirectory

This function looks good, but maybe you could simply expose remove from cstdlib?

---

Closing thoughts:

I think this is good code for a personal project or learning about the filesystem APIs, but you haven't handled a lot of the classic problems with filesystem libraries e.g. race conditions and weird failures. C++ has std::filesystem which can do most of this. std::filesystem's API will try to force you do reasonable things.