2
\$\begingroup\$

It's been a while I program in ANSI C, and helping my brother with a school project I ended up revising my C gift (or lack of) :)

the idea was to create an inverted V process using fork() and I ended up with the code that you can find in GitHub

I was wondering if there's an easy way of accomplish it, as I see that I might be able to refactor it as there's code that is equal in some lines, but I could never got it to have the same output

This is only for curiosity as the work is already presented in school but I always thrive to learn better and I keep wonder if we can refactor the block below

pid = fork();
switch (pid) {
  case -1:
    printf("fork failed\n"); 
    break;
  case 0:
    printf("Process %s%d %d, from %d\n", colLetter, currentRow, getpid(), getppid());
    currentRow += 1;
    // process 1 fork and his child
    process_single_tree(colLetter, maxInteractions, currentRow);
    break;
  default:
    break;
}

to only have one iteration and not having to similar blocks in the code


Added

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/wait.h>

#define MIN 0
#define MAX 32

/**
 * Validates if the interaction number is between the acceptable range
 * @param int lines to process
 * @return nothing
 */
void validateInteractions(int interactions)
{
    if (interactions < MIN || interactions > MAX)
    {
        printf("Level must be between %d and %d", MIN, MAX);
        exit(1);
    }
}

/**
 * Validates if there are enough arguments
 * @param int number of arguments (argc)
 * @return nothing
 */
void validateArguments(int arguments) {
    if (arguments < 2)
    {
        printf("A decimal argument must be declared");
        exit(1);
    }
}

/**
 * Processes a single fork() and his child
 * @param char the column Letter that the column belongs
 * @param int the max interactions to process
 * @param int the current row id
 * @return nothing
 */
void process_single_tree(char *colLetter, int maxInteractions, int currentRow)
{
    int pid;
    int status;

    if (currentRow > maxInteractions)
    {
        exit(0);
    }

    pid = fork();
    switch (pid)
    {
    case -1:
        printf("fork failed\n");
        break;
    case 0:
        printf("Process %s%d %d, from %d\n", colLetter, currentRow, (int)getpid(), (int)getppid());
        currentRow += 1;
            // process 1 fork and his child
        process_single_tree(colLetter, maxInteractions, currentRow);
        break;

    default:
        break;
    }

    wait(&status);
}

/**
 * Processes double fork() and their childs
 * @param int the max interactions to process
 * @param int the current row id
 * @return nothing
 */
void process_double_tree(int maxInteractions, int currentRow)
{
    int i;
    int index = 2; // process twice
    int pid;
    int status;
    char *colLetter; // column letter

    for (i = 1; i <= index; i++)
    {
        colLetter = i == 1 ? "A" : "B";
        if (currentRow > 1)
            exit(0);

        pid = fork();
        switch (pid)
        {
        case -1:
            printf("fork failed\n");
            break;
        case 0:
            printf("Process %s%d %d, from %d\n", colLetter, currentRow, (int)getpid(), (int)getppid());
            currentRow += 1;
            // process 1 fork and his child
            process_single_tree(colLetter, maxInteractions, currentRow);
            break;

        default:
            break;
        }
    }
    for (i = 0; i < index; i++)
    {
        wait(&status);
    }
}

int main(int argc, char *argv[])
{
    int interactions;
    int currentRow = 1;

    // check if "argv[1]" is valid
    validateArguments(argc);

    interactions = atoi(argv[1]);

    // check if level is between MIN and MAX
    validateInteractions(interactions);

    printf("Inverted V process tree with n=%d\n", interactions);
    printf("Process AB has PID=%d\n", (int)getpid());

    // there are levels to process
    if (interactions > 0)
    {
        // create 2 forks and process each child
        process_double_tree(interactions, currentRow);
    }
}

edited added as suggested, as pid's should be cast

\$\endgroup\$
8
  • 1
    \$\begingroup\$ Welcome to code review where we review working code from projects to provide suggestions on how that code can be improved. This question is off-topic because it doesn't provide enough code to review. We would need to see at least the entire function this snippet is a part of, but preferably we would like to see the entire code of the project. For instance the code references a function called process_single_tree() but that function is not included for review. \$\endgroup\$
    – pacmaninbw
    Apr 12, 2020 at 22:29
  • 1
    \$\begingroup\$ The code to be reviewed must be included in the question, a link is fine for a full project. Snippets might be okay on stackoverflow, but they are not okay on code review. Please take a look at codereview.stackexchange.com/help/how-to-ask. \$\endgroup\$
    – pacmaninbw
    Apr 12, 2020 at 23:02
  • 2
    \$\begingroup\$ Unless you post your code here in your question then for legal reasons we cannot touch your code. Your code is not available under any copyright meaning that any review of your code we perform is potentially copyright infringement. For this reason we have the "authorship" close reason, so we can protect users that could be subjected to copyright trolling. \$\endgroup\$
    – Peilonrayz
    Apr 12, 2020 at 23:12
  • 3
    \$\begingroup\$ so, very diff from StackOverflow... thank you to point out the rules for me! I've added the code to the question :) \$\endgroup\$
    – balexandre
    Apr 12, 2020 at 23:19
  • 2
    \$\begingroup\$ Note: getpid() does not certainly return an int and so "%d" is not certainly the correct specifier. Suggest What is the correct printf specifier for printing pid_t. \$\endgroup\$ Apr 12, 2020 at 23:19

2 Answers 2

1
\$\begingroup\$

regarding:

printf("fork failed\n");

error messages should be output to stderr and when the error is from a C library function, should also output the text reason the system thinks the error occurred. Suggest:

perror( "fork failed" );

which handles both output activities

regarding:

interactions = atoi(argv[1]);  

Always check argc to assure the user actually entered the expected command line parameter (if not, then output to stderr a USAGE message) before accessing anything beyond argv[0]. Other wise, when the expected command line parameter has not been entered by the user, accessing beyond argv[0] will result in a seg fault event. without the user having any idea as to why

regarding:

printf("A decimal argument must be declared");

This outputs an error message to stdout rather than to stderr. Suggest:

fprintf( stderr, "USAGE: %s decimal argument\n", argv[0] );

regarding:

case 0:
    printf("Process %s%d %d, from %d\n", colLetter, currentRow, (int)getpid(), (int)getppid());
    currentRow += 1;
        // process 1 fork and his child
    process_single_tree(colLetter, maxInteractions, currentRow);
    break;

default:
    break;
}

wait(&status);

This will have the child process (along with the parent process) calling wait() Suggest:

case 0:
    printf("Process %s%d %d, from %d\n", colLetter, currentRow, (int)getpid(), (int)getppid());
    currentRow += 1;
        // process 1 fork and his child
    process_single_tree(colLetter, maxInteractions, currentRow);
    exit( EXIT_SUCCESS );   <<-- added statement
    break;

default:
    wait(&status);
    break;
}

regarding:

interactions = atoi(argv[1]);

The function: atoi() can fail and it will not notify you of this event. Suggest using the function: strtol()

\$\endgroup\$
1
\$\begingroup\$

stderr

Consider printing your errors to stderr; for example:

    printf("Level must be between %d and %d", MIN, MAX);

can be

    fprintf(stderr, "Level must be between %d and %d", MIN, MAX);

colLetter

If this is actually a letter, is there any reason you can't pass it around as a char instead of a char* string?

perror

printf("fork failed\n");

Based on the spec, this modifies errno, so you are best to use perror here instead of printf.

Casting prior to printf

getpid actually returns a pid_t, which might not be guaranteed to fit in an int. In fact, the spec says that

The implementation shall support one or more programming environments in which the widths of pid_t [...] are no greater than the width of type long.

So to avoid overflow, you're safer to cast this to a long and print with %l.

No-op default

This:

default:
    break;

can be omitted.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.