Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement the signal command #39 #43

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

irevoire
Copy link

No description provided.

@irevoire irevoire requested a review from jfsmig January 21, 2020 16:15
Copy link
Author

@irevoire irevoire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not able to test anything, so you should not merge this.

errno = 0; // this should not be needed

if( argc <= 0) {
g_string_append_printf(out, "No signal were specified\n");
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should return an error here, but I have no idea on how to do this.

main/gridinit.c Outdated
str_signal = argv[0];
argv++;
argc--;
g_string_append_printf(out, "START signal\n");
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ll remove this

g_string_append_printf(out, "START signal\n");

// first we try to convert the string to an integer
signal = strtol(str_signal, NULL, 10); // TODO should we autodetect the base?
Copy link
Author

@irevoire irevoire Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to allow hexadecimal signals and things like that?

// we look into the of known signals
for (struct signal_mapping_s* s = SIGNAL;; s++) {
if (s->name == NULL) { // we did not find any signals
g_string_append_printf(out, "Unable to decode the signal %s\n", str_signal);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

send an error

{ "SIGSTKFLT", SIGSTKFLT },
{ "SIGUNUSED", SIGUNUSED },
{ NULL, 0 },
#endif
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think we should put this thing here. It’s not good for code readability.
It would probably be best to create a plateform file or a signal file for this.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 to have a dedicated file

@irevoire
Copy link
Author

It’s a funny thing that travis do not know SIGEMT, SIGINFO and SIGLOST.
Since travis tried to use SIGLOST I can deduce that it run on linux.
And all the flags I used obviously do exists on linux: http://man7.org/linux/man-pages/man7/signal.7.html

But I guess travis uses some old linux.
To support that we could either remove some flag or add some #if to check wich version of linux is being used.

char *name;
int value;
} SIGNAL [] = {
#ifdef __unix__ // the base signals
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, lot of copy and paste, you should use a macro

#define ITEM(x) {x, #x}

and have

] SIGNAL [] = {
ITEM(SIGABRT),
ITEM(SIGHUP),
...
}

{ "SIGSTKFLT", SIGSTKFLT },
{ "SIGUNUSED", SIGUNUSED },
{ NULL, 0 },
#endif
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 to have a dedicated file

{ "SIGXCPU", SIGXCPU },
{ "SIGXFSZ", SIGXFSZ },
#else
#error Your plateform is not supported
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to have #error at top

}

void signal_process(void *u UNUSED, struct child_info_s *ci) {
kill(ci->pid, signal);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we should emit a message with kill return an error

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants