4
\$\begingroup\$

My hobby is Unix-like operating systems.

Recently I began writing my own sel4-based Posix compliant OS. Here's the code for internal signal handling functions which presents a kind of backend to help implement functions such as do_kill or do_sigsuspend.

I would like to know what you think about code quality, and any possible errors I've overlooked.

#include "process_definitions.hxx"
#include <signal.h>
#include <array>
#include "signal_definitions.hxx"
#include <sys/types.h>
#include <unistd.h>
#include <kernel_glue.hxx>
using namespace process_methods;


  std::array<int, 256> signal_num ;
  std::array<pid_t, 256> receiver ;      

    sigset_t signals_methods::signal_queue::dump_pending(pid_t pid)
    {
        sigset_t retval;
        for (int init = 256; init-- ;)
        {
            if (receiver[init] == pid )
            {
                int index = (signal_num[init] / sizeof(sigset_t));
                *(retval.__val) = *(retval.__val) & (1 << signal_num[init]);
            }
        }
        return retval;
    }

    void signals_methods::signal_queue::aggregate_signal_queue()
    {
        for (int init = 256 ; init--;)
        {
            send_signal(receiver[init], signal_num[init]);
            receiver[init] = 0 ;
            signal_num[init] = 0;
        }
    }

    void signals_methods::signal_queue::put_signal_to_queue(pid_t pid, int signal)
    {
        for (int init = 256; init--;)
        {
            if (signal_num[init] == 0 && receiver[init] == 0)
            {
                signal_num[init] = signal; 
                receiver[init] = pid;
            }
    }

    


};


signal_data_struct *signals_methods::get_signal_info(pid_t pid)
    {
        return &get_process_info(pid)->signal_data;
    }

signal_data_struct *signals_methods::get_signal_info()
{
    return &get_process_info()->signal_data;
}

 void  signals_methods::send_signal(pid_t pid, int signal)
    {
        if (!(get_signal_info(pid)->signal_flags[signal] & (1 << SA_NOMASK)))
        {   
            if (*(get_signal_info(pid)->blocked_mask.__val) & (1  << signal))
            {
                signals_methods::signal_queue::put_signal_to_queue(pid, signal); //signal is blocked so wait while it will be unblocked
                return;
            }
        }


        get_signal_info(pid)->blocked_mask = get_signal_info(pid)->signal_mask[signal];

        if (get_signal_info(pid)->handler[signal] == SIG_DFL )
        {
            exec_default_signal_handler( pid, signal);
            return;
        }

        if (get_signal_info(pid)->handler[signal] == SIG_IGN)
        {
            return;
        }

        if(get_signal_info(pid)->thread_suspend_lock)
        {
            thread_restore(pid);
        }

        if (get_signal_info(pid)->signal_flags[signal] & SA_SIGINFO)
        {
            
            exec_signal_handler_siginfo(pid, &get_signal_info(pid)->handler[signal], signal, &(get_signal_info(pid)->siginfo[signal]));
        } 
        else 
        {
            exec_signal_handler(pid, signal, &get_signal_info(pid)->handler[signal]);
        }
    }

    void signals_methods::exec_default_signal_handler(pid_t pid, int signal)
    {
    #define dump 0
    #define revoke 1
    #define stop 2
    #define cont 3
    #define ign 4
        int sigmode [] = {
                [SIGABRT] = dump,
                [SIGBUS] = dump,
                [SIGFPE] = dump,
                [SIGILL] = dump,
                [SIGQUIT] = dump,
                [SIGSEGV] = dump ,
                [SIGSYS] = dump ,
                [SIGTRAP] = dump ,
                [SIGXCPU] = dump ,
                [SIGXFSZ] = dump ,

                [SIGALRM] = revoke,
                [SIGHUP] = revoke,
                [SIGINT] = revoke,
                [SIGKILL] = revoke,
                [SIGPIPE] = revoke,
                [SIGTERM] = revoke,
                [SIGUSR1] = revoke,
                [SIGUSR2] = revoke,
                [SIGPOLL] = revoke,
                [SIGPROF] = revoke,
                [SIGVTALRM] = revoke,

                [SIGSTOP] = stop,
                [SIGTSTP] = stop ,
                [SIGTTIN] = stop,
                [SIGTTOU] = stop,

                [SIGCONT ] = cont,

                [SIGURG] = ign
                };


        if (!(get_signal_info(pid)->signal_flags[signal] & SA_NOCLDSTOP))
        {
            signals_methods::signal_queue::put_signal_to_queue(SIGCHLD, get_process_info(pid)->parent_pid);
            //get_signal_info(get_process_info(pid)->parent_pid)->siginfo[SIGCHLD]._sifields)
        }
           switch (sigmode[signal])
                {
                case dump:
                    thread_dump(pid);
                    break;
                case revoke:
                    thread_revoke(pid);
                    break;
                case stop:
                    thread_stop(pid);
                    break;
                case cont:
                    thread_restore(pid);
                default:
                    return;
                    break;
                }

        };
\$\endgroup\$
1
  • \$\begingroup\$ no, my code is working how it must. me simply interesting how i can improve alghroritm \$\endgroup\$ Commented Jun 27, 2021 at 22:22

2 Answers 2

Reset to default
3
\$\begingroup\$

What are the magic numbers 256 scattered all over the place? If they all mean the same thing (number of supported distinct signals?), then name a constant. If they have more than one meaning, then there's even more reason to use well-named constants.

I don't like these:

#define dump 0
#define revoke 1
#define stop 2
#define cont 3
#define ign 4

We normally use ALL_CAPS for macros (since they require particular care and attention). But really there's no need to use the preprocessor for this. An ordinary enum is much more appropriate - for one thing, it allows the compiler to ensure that all cases are handled in the switch further on:

       switch (sigmode[signal])
            {
            case dump:
                thread_dump(pid);
                break;
            case revoke:
                thread_revoke(pid);
                break;
            case stop:
                thread_stop(pid);
                break;
            case cont:
                thread_restore(pid);
                break;
            case ign:
                return;
            }

(I also removed the unreachable break statement that followed return there, and eliminated the fall-through that might catch out somebody adding another case in future.)

\$\endgroup\$
1
  • \$\begingroup\$ Thanks, that was very helpfully. And about "magic 256", is just a length for signal queue. \$\endgroup\$ Commented Jun 29, 2021 at 1:04
3
\$\begingroup\$

Toby's answer mentioned the high use of the magic constant 256.

I think you should at the very least use signal_num.size() and the like, so that the number only appears in the array definition. Even then, since you have multiple arrays that are the same size, you should name a constant like signal_count.

Where you are looping over the entire array, it would be better if you could use the range-based for statement. Combining the test and decrement with the test is odd enough. Since you seem to always be iterating backwards, perhaps you should store them in the opposite order? Then just transform the argument for the calls that get/set values by signal number.

signal_data_struct *signals_methods::get_signal_info(pid_t pid)
In C++, unlike C, it's orthodox to put the * with the type not with the variable (or function name in this case). Write signal_data_struct* foo. But why aren't you defining the functions to return references, rather than pointers?

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.