Allow the option of parsing multiple post-connection commands.

[criscanedo]

Since kirc is my IRC client of choice I have a shell script to start a connection from any shell. I run an IRC channel and thus run identify with ChanServ in addition to identify with NickServ upon connection. It didn't look like there was a way for kirc to accept multiple post-connection commands so I added a small bit of code to allow that type of functionality at the most basic level for myself. Perhaps this could also be useful to future kirc users.

Using my shell script as an example, this change would allow the 2 -x options below to be processed:

kirc -s irc.dal.net -p 6667 -o $logfile -n $IRCNICK -x "$identnick" -x "$identchan"

Alternatively, we could allow for multiple commands to be processed using one -x option instead by specifying a delimiter thus resulting in processing each token in just one string.

Let me know what you all think. Also, great work on kirc. To me it's an absolutely phenomenal client. It didn't take long for me to choose kirc over a handful of other clients that I was simultaneously comparing the other day.

[mcpcpc]

Thanks for the PR and your kind words =).

Regarding the scope of this PR, I can definitely understand the need for passing multiple initial commands. With the current PR implementation, however, there may be instances where more than the default of 5 (or however many are specified) initial commands would need to be specified and I would hate to impose a set limit. Of course, the user could change this value their liking and rebuild kirc, but i know from experience that most users are just installing from existing distribution repositories. Using a delimiter character would also work, but it means that the delimited char can no longer be used as part of a startup command (e.g. what. if I wanted to send a PM containing that delimiting char?). The only safe delimited char, imho, to use is '\n', but that is hard to use unless you are dealing with a text editor. With that said, I wonder if it would make more sense to read from STDIN?

Here are three alternative proposals/solutions:

1. Use the current PR implementation, but increase the number of arguments in the proposed solution from 5 to some unreasonable number.
2. Introduce a new switch (-X) which tells kirc to read from STDIN. Treat each '\n' char of the input string as IRC new command and read until '\0' is reached (or some definitive buffer size). This achieves the desired functionality without impacting any existing functionality.
3. Replace the current -x switch behavior with that of the Solution 1. This enhances the existing capability of the initial command switch without introducing a new argument. The consequence is that this may break some user's startup scripts that upgrade (due to the inherent change in the argument behavior).

As an example for Option 3, a single startup command would look like this:

$ echo "privmsg NickServ :identify <username> <password>" | kirc  -s irc.dal.net -p 6667 -x

Then using a startup command script:

$ cat commandfile.txt | kirc  -s irc.dal.net -p 6667 -x

Personally, I am in favor of Option 3. Per usual, I am open to any other proposals or counter arguments.

[criscanedo]

Thanks for the PR and your kind words =).

Regarding the scope of this PR, I can definitely understand the need for passing multiple initial commands. With the current PR implementation, however, there may be instances where more than the default of 5 (or however many are specified) initial commands would need to be specified and I would hate to impose a set limit. Of course, the user could change this value their liking and rebuild kirc, but i know from experience that most users are just installing from existing distribution repositories. Using a delimiter character would also work, but it means that the delimited char can no longer be used as part of a startup command (e.g. what. if I wanted to send a PM containing that delimiting char?). The only safe delimited char, imho, to use is '\n', but that is hard to use unless you are dealing with a text editor. With that said, I wonder if it would make more sense to read from STDIN?

Here are three alternative proposals/solutions:

1. Use the current PR implementation, but increase the number of arguments in the proposed solution from 5 to some unreasonable number.

2. Introduce a new switch (-X) which tells kirc to read from STDIN. Treat each '\n' char of the input string as IRC new command and read until '\0' is reached (or some definitive buffer size). This achieves the desired functionality without impacting any existing functionality.

3. Replace the current -x switch behavior with that of the Solution 1.  This enhances the existing capability of the initial command switch without introducing a new argument. The consequence is that this may break some user's startup scripts that upgrade (due to the inherent change in the argument behavior).

As an example for Option 3, a single startup command would look like this:

$ echo "privmsg NickServ :identify <username> <password>" | kirc  -s irc.dal.net -p 6667 -x

Then using a startup command script:

$ cat commandfile.txt | kirc  -s irc.dal.net -p 6667 -x

Personally, I am in favor of Option 3. Per usual, I am open to any other proposals or counter arguments.

I did consider the implication of having a delimiter be a part of the start up command string and so also considered the possibility of an escape character; but I don't want to offload extra information to the user if not truly necessary.

However, I do like the idea of reading from STDIN when the -x option is specified. As it turns out, when redirecting to STDIN the call to isatty(STDIN_FILENO) in enableRawMode() fails because the file descriptor for STDIN is no longer associated with terminal of the calling process; it now refers to whatever was piped/redirected in. I couldn't find a function call to deal with this situation directly other than to create a variable to hold the STDIN file descriptor, with an initial default value of STDIN_FILENO static int ttyinfd = STDIN_FILENO, perform an easy search and replace to replace all instances of STDIN_FILENO with ttyinfd, and upon program execution set the value of ttyinfd to the return value of open("/dev/tty", 0) which returns a file descriptor for the terminal associated with the current process.

With this change even if the -x option is not specified but a user attempts to redirect to STDIN, the program will not break during the call to isatty(). Though if this scenario does happen we might want to clear the buffered STDIN that the program has no use for. (I wonder if there is an easier way to go about this. It would be nice to instead reuse STDIN_FILENO by simply pointing it back to the terminal associated with the current process. Any suggestions?)

When sendCommands() is called if the -x option is detected, it reads from STDIN (MSG_MAX + 1)* CMD_MAX bytes. CMD_MAX is set to 100 instead of the previous 5 and I've reused MSG_MAX as the max length of an additional command with + 1 to account for newline characters. The program will now process up to 100 additional commands with each command having a max length of 512 bytes. sendCommands() makes use of strtok() to split the data read from STDIN, delimited by the newline character, and verifies the length of the token before sending it to raw() and is skipped otherwise with a message written to STDERR. (I figured commands exceeding set limits is a user error and benign enough to simply skip over and log instead of terminating the entire process. Any thoughts here?)

That covers the two changes introduced in the recent commits. My main concern is the same as yours: breaking users' start up scripts who were utilizing -x "". Before introducing the change in these commits I tried my best to keep the same functionality as before in allowing an argument to -x but also reading from STDIN. Ultimately I ran into an issue with the string argument to the -x option being ignored when STDIN is redirected to. optarg for the option is set to NULL although the argument string is present in argv. My guess is it has something to do with STDIN_FILENO no longer being associated with the terminal on program start up when redirecting STDIN. I am still looking into this in case you have any suggestions.

[criscanedo]

Ultimately I ran into an issue with the string argument to the -x option being ignored when STDIN is redirected to. optarg for the option is set to NULL although the argument string is present in argv. My guess is it has something to do with STDIN_FILENO no longer being associated with the terminal on program start up when redirecting STDIN. I am still looking into this in case you have any suggestions.

Correction: optarg is NULL because getopt() only places a pointer in optarg if an option requires an argument. Since we now consider STDIN, we can either set -x to expect no argument or expect an optional argument with :: (GNU extension).

Regardless of what we choose, luckily we can use optind to index argv and retrieve the argument of -x, argv[optind], add a bit of error handling, modify sendCommands() to account for an argument to -x, if any, and ultimately retain backwards compatibility with this new change so that it doesn't break current users' scripts and others can still take advantage of multiple commands via STDIN.

[criscanedo]

As an example in case you wanted to get a visual, the delimiter suggestion would look something like this - criscanedo@f8db275

It is a working copy so you can pull it and test it out.

[mcpcpc]

Maybe i am overlooking something, but couldn't we simplify this significantly by capturing STDIN to a buffer before we enable raw mode? For example, the following could be declared before initConnection() :

// declared at the beginning, along with 
// #define CBUF_SIZ 1024
// static char cbuf[1024];
// static int cmds = 0;

if (cmds > 0) {
		int flag = 0;
                // read STDIN until CBUF_SIZ reached or error -1 returned
		for (int i = 0; (i < CBUF_SIZ) && (flag > -1); i++) {
			flag = read(STDIN_FILENO, &cbuf[i], 1);
		}
}

We could then use a tokanizer to split the buffer at the '\n' chars and write each command char after initConnection() . For example, something like this:

// see the messageWrap() function as an example
if (cmds > 0) {
		for (char *tok = strtok(cbuf, "\n"); tok != NULL; tok = strtok(NULL, "\n")) {
			raw("%s\r\n", tok);
		}
}

[criscanedo]

Indeed that is what I tried initially and is what led me to discover the file descriptor STDIN_FILENO will no longer be associated with the terminal if STDIN is redirected to. That means the program is no longer able to read input from the terminal through STDIN_FILENO and was the reason behind creating a file descriptor for the associated terminal of the process to read input regardless of that happening:

static void opentty()
{
	if ((ttyinfd = open("/dev/tty", 0)) == -1) {
		perror("failed to open /dev/tty");
		exit(1);
	}
}

Additionally because STDIN_FILENO is no longer associated to a terminal with redirected STDIN, the following call to isatty() will cause the application to exit:

static int enableRawMode(int fd) {
	if (!isatty(STDIN_FILENO)) {
		goto fatal;
	}
        ...
 }

Though using the file descriptor for the terminal to read input (ttyinfd) will keep the program running. The code you suggested will indeed work with ttyinfd, setting the -x switch to require no argument, and ignoring an argument to -x if any.

The call to isatty() in readcmds() was to check if STDIN had buffered input and only then read from STDIN so as to prevent the program from waiting for user input if -x is specified with no input:

static void readcmds(char *cmd) {
        if (!isatty(STDIN_FILENO)) {
                ...
        }
        ...
}

Your changes certainly cover the required functionality so I'll go ahead and incorporate them to the branch and push it up shortly. Would you still want to consider an argument to -x if any?

[criscanedo]

Including this into your change you should now be able to send commands read from both STDIN and as an argument:

if (cmds > 0) {
	for (char *tok = strtok(cbuf, "\n"); tok; tok = strtok(NULL, "\n")) {
		raw("%s\r\n", tok);
	}
	if (inic) {
		raw("%s\r\n", inic);
	}
}

[mcpcpc]

@criscanedo the latest commit looks good. I will do some testing in a few hours and then merge (+version bump and new release tarballs shortly after).

[criscanedo]

Awesome, thanks. Likewise I will also continue testing and will be on stand by if anything comes up.