change file attributes on a Linux file system with chattr

Question

I am reading the book The Linux Programming Interface and I have reached the exercise in which the implementation of the chattr command in C is required.

I would like to receive feedback on the implementation, to criticize my solution

#include <unistd.h>
#include <stdlib.h>
#include <stdio.h>
#include <fcntl.h>
#include <errno.h>
#include <string.h>
#include <sys/ioctl.h>

#include <linux/fs.h>

static inline void  
do_ioctl(int fd, unsigned long req, int *attr)
{
    if (ioctl(fd, req, attr) == -1) {
        fprintf(stderr, "ERROR: ioctl error - %s\n",
                strerror(errno));
        exit(EXIT_FAILURE);
    }
    
    return;
}


int
main(int argc, char *argv[])
{
    int i;
    int fd;
    int attr;
    int flag_count;

    if (argc < 3) {
        fprintf(stderr, "Usage: %s <flags> <file_name>\n", argv[0]);
        exit(EXIT_FAILURE);
    }
    
    if ((fd = open(argv[2], O_RDONLY | O_WRONLY)) == -1) {
        fprintf(stderr, "ERROR: Unable to open %s -- %s\n",
                argv[2], strerror(errno));
        exit(EXIT_FAILURE);
    }

    do_ioctl(fd, FS_IOC_GETFLAGS, &attr);

    flag_count = strlen(argv[1]);
    for (i = 1; i < flag_count; i++) {
        switch (argv[1][i]) {
            case 'a':
                attr |= FS_APPEND_FL;
                break;
            case 'c':
                attr |= FS_COMPR_FL;
                break;
            case 'D':
                attr |= FS_DIRSYNC_FL;
                break;
            case 'i':
                attr |= FS_IMMUTABLE_FL;
                break;
            case 'j':
                attr |= FS_JOURNAL_DATA_FL;
                break;
            case 'A':
                attr |= FS_NOATIME_FL;
                break;
            case 'd':
                attr |= FS_NODUMP_FL;
                break;
            case 't':
                attr |= FS_NOTAIL_FL;
                break;
            case 's':
                attr |= FS_SECRM_FL;
                break;
            case 'S':
                attr |= FS_SYNC_FL;
                break;
            case 'T':
                attr |= FS_TOPDIR_FL;
                break;
            case 'u':
                attr |= FS_UNRM_FL;
                break;
            default:
                fprintf(stderr, "ERROR: unknown flag %c\n", argv[1][i]);
                exit(EXIT_FAILURE);
        }
    }
    do_ioctl(fd, FS_IOC_SETFLAGS, &attr);
    
    exit(EXIT_SUCCESS);
}

This implementation of the chattr command is a more simplified option

Also, as the book states (studying system calls) I would like some suggestions for replacing standard functions with POSIX system calls, what suggestions would you have here? I have implemented with classic functions for the moment. I would use system calls from the book APUE, being very popular and very good for learning system calls rather than those in this book.

Answer 1

General Overview

The code is fairly well written and a good start. I'm sorry but I can't help out on the POSIX part of the question.

Possible Bug

The current parsing of the command line has been over simplified, the use of multiple switches may not work correctly or at all. The -R and other command line switches (-v, -p) are not checked for, and if used may break the implementation. It might be better to parse for all the switches and report to the user which ones are not implemented. The code assumes that all switches will be compacted together, and this is only sometimes true, especially where there are switches that have a second argument.

The exit(STATUS) Function Is Not Needed in main()

The normal way to return an exit status from main() is to return from main rather than calling the exit() function. The exit() function is only needed when exiting from a function other than main(). The reason for this is that main() is the entry point for the operating system into the program so anything it returns is returned to the operating system.

If main() doesn't return a value at the end of the program the program will return EXIT_SUCCESS by default.

The only place in this program the the exit() call is really necessary is in the do_ioctl() function, and if that function returned the exit status rather than being void there would be no reason to call exit() at all.

There are good reasons for calling the exit() function, and in a program executed by a shell it is okay, but if you are writing a shell or an operating system you don't really want to use it. Calling exit() from the operating system will shut down the system in an unfriendly manner, it can have serious side effects.

The Use of the inline Key Word

Generally the inline Key Word is for optimizing functions for performance reasons. The inline Key Word is a recommendation to the compiler that the code should be put in the cache memory. A good optimizing compiler will generally generate better optimization than human attempting optimization so the inline key word is sometimes ignored, especially if you are compiling -O3.

In this particular case there is no good reason to use the inline key word since the program calls do_ioctl() only twice, so it won't ever become a bottleneck.

Readability and Maintainability

The code is generally readable, but there are changes I would make:

Declare Variables as Necessary

In the original version of C all variables needed to be declared at the top of the function, this is no longer true. To make the code easier to read and maintain, declare the variables as necessary

int
main(int argc, char* argv[])
{
    int exit_status = EXIT_SUCCESS;
    if (argc < 3) {
        fprintf(stderr, "Usage: %s <flags> <file_name>\n", argv[0]);
        return EXIT_FAILURE;
    }

    int fd;
    if ((fd = open(argv[2], O_RDONLY | O_WRONLY)) == -1) {
        fprintf(stderr, "ERROR: Unable to open %s -- %s\n",
            argv[2], strerror(errno));
        return EXIT_FAILURE;
    }

    if (exit_status != EXIT_FAILURE)
    {
        int file_attributes = 0;
        exit_status = get_file_attributes(fd, &file_attributes);

        if (exit_status != EXIT_FAILURE)
        {
            exit_status = process_command_line(argc, argv, &file_attributes);
        }

        set_file_attributes(fd, &file_attributes);
    }

    close(fd);

    return exit_status;
}

Complexity

The function main() is too complex (does too much). As programs grow in size the use of main() should be limited to calling functions that parse the command line, calling functions that set up for processing, calling functions that execute the desired function of the program, and calling functions to clean up after the main portion of the program.

There is also a programming principle called the Single Responsibility Principle that applies here. The Single Responsibility Principle states:

that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

By isolating the command line processing in it's own function it is easier to maintain by adding or deleting switches.

static int get_file_attributes(int fd, int* file_attributes)
{
    int exit_status = EXIT_SUCCESS;

    if (ioctl(fd, FS_IOC_GETFLAGS, file_attributes) == -1) {
        fprintf(stderr, "ERROR: Failed to get file attributes : ioctl error - %s\n", strerror(errno));
        exit_status = EXIT_FAILURE;
    }

    return exit_status;
}

static int set_file_attributes(int fd, int* file_attributes)
{
    int exit_status = EXIT_SUCCESS;

    if (ioctl(fd, FS_IOC_SETFLAGS, file_attributes) == -1) {
        fprintf(stderr, "ERROR: Failed to set file attributes : ioctl error - %s\n", strerror(errno));
        exit_status = EXIT_FAILURE;
    }

    return exit_status;
}

static int process_command_line(int argc, char *argv[], int *file_attributes)
{
    int exit_status = EXIT_SUCCESS;

    int flag_count = strlen(argv[1]);
    for (int i = 1; i < flag_count; i++) {
        switch (argv[1][i]) {
        case 'a':
            *file_attributes |= FS_APPEND_FL;
            break;
        case 'c':
            *file_attributes |= FS_COMPR_FL;
            break;
        case 'D':
            *file_attributes |= FS_DIRSYNC_FL;
            break;
        case 'i':
            *file_attributes |= FS_IMMUTABLE_FL;
            break;
        case 'j':
            *file_attributes |= FS_JOURNAL_DATA_FL;
            break;
        case 'A':
            *file_attributes |= FS_NOATIME_FL;
            break;
        case 'd':
            *file_attributes |= FS_NODUMP_FL;
            break;
        case 't':
            *file_attributes |= FS_NOTAIL_FL;
            break;
        case 's':
            *file_attributes |= FS_SECRM_FL;
            break;
        case 'S':
            *file_attributes |= FS_SYNC_FL;
            break;
        case 'T':
            *file_attributes |= FS_TOPDIR_FL;
            break;
        case 'u':
            *file_attributes |= FS_UNRM_FL;
            break;
        default:
            fprintf(stderr, "ERROR: unknown flag %c\n", argv[1][i]);
            exit_status = EXIT_FAILURE;
        }
    }

    return exit_status;
}

Answer 2

Sentinels on troublesome output

When open fails, something is wrong. Sometimes a formed filename is amiss. Using sentinels like "\"%s\"" helps the user identify exactly where the filename starts/ends. Consider a filename name of " abc def ". Spaces in arguments are unusual but possible.

// fprintf(stderr, "ERROR: Unable to open %s -- %s\n", argv[2], strerror(errno));
fprintf(stderr, "ERROR: Unable to open \"%s\" -- %s\n", argv[2], strerror(errno));