Hi Vladimir,

On Monday 11 February 2008 16:59, Vladimir Dronnikov wrote:
> This is the first draft of chat applet. No optimization yet!

--- busybox.orig/miscutils/chat.c       1970-01-01 03:00:00.000000000 +0300
+++ busybox/miscutils/chat.c    2008-02-11 18:41:01.000000000 +0300
@@ -0,0 +1,276 @@
+/* vi: set sw=4 ts=4: */
+/*
+ * bare bones chat utility
+ * inspired by ppp's chat
+ *
+ * Copyright (C) 2008 by Vladimir Dronnikov <[EMAIL PROTECTED]>
+ *
+ * Licensed under GPLv2, see file LICENSE in this tarball for details.
+ */
+#include "libbb.h"
+
+#define        DEFAULT_CHAT_TIMEOUT    45 * 1000
+
+/* All known arches use small ints for signals */
+static volatile smallint signalled;
+
+static int timeout = DEFAULT_CHAT_TIMEOUT;
+
+static void signal_handler(int signo)
+{
+       signalled = 3 + (SIGALRM == signo);
+}

But you forgot to set up the trap on SIGALRM...

+
+static llist_t *aborts;
+
+static int chat_expect(char *answer)
+{
+       //bb_error_msg("EXPECT: [%s]", answer);
+
+       // shall we wait for answer?
+       if (!answer || !*answer)
+               return 0;
+
+       // reset input buffer
+       int nbuf = sizeof(bb_common_bufsiz1);
+       char *buf = bb_common_bufsiz1; memset(buf, 0, nbuf--);
+       int i;

Declarations should not be mixed with code.

+       // get answer
+       struct pollfd pfd;
+       pfd.fd = STDIN_FILENO;
+       pfd.events = POLLIN;
+       while (!signalled && safe_poll(&pfd, 1, timeout) > 0) {
+               llist_t *l;
+               // read next char(s) from device
+               if (!(pfd.revents & POLLIN))
+                       break;
+               if (safe_read(STDIN_FILENO, buf, 1) > 0) {
+                       buf++;
+                       if (!--nbuf) {
+                               bb_error_msg("LINE TOO LONG");
+                               return 2;
+                       }
+               }
+               // TODO: escape substitutions in received answer!!!
+               // abort matched?
+               for (l = aborts, i = 5; l; l = l->link, ++i)
+                       if (strstr(bb_common_bufsiz1, l->data)) {
+                               bb_error_msg("ABORTED: [%s]", l->data);
+                               return i;
+                       }
+               // answer matched?
+               if (strstr(bb_common_bufsiz1, answer)) {
+                       // match found!
+                       //if (opts & OPT_v) {
+                       //      bb_error_msg("GOT: [%s]", bb_common_bufsiz1);
+                       //}
+                       return 0;
+               }
+       }
+
+       // answer timed out or invalid answer received
+       bb_error_msg("FAILED: [%s]", bb_common_bufsiz1);
+       return 4;
+}
+
+//static void chat_send(char *s, int timeout)
+static void chat_send(char *s)
+{
+       char *data = NULL;
+       int cr = 1;
+
+//     bb_error_msg("SEND: [%s]", s);
+
+       if ('@' == s[0]) {
+               // skip the @ and any following white-space
+               trim(++s);
+               s = data = xmalloc_open_read_close(s, NULL);
+       }
+
+//     if (opts & OPT_v) {
+//             bb_error_msg("send (%s)", s);
+//     }
+
+       alarm(timeout);
+       while (*s) {
+               // do we need special processing?
+               if ('\\' == *s) {
+                       char c = *++s;
+                       if (c) {
+                               if ('d' == c) {
+                                       sleep(1);
+                               } else if ('p' == c) {
+                                       usleep(100000);
+                               } else if ('K' == c) {
+                                       tcsendbreak(STDIN_FILENO, 0);
+                               } else {
+                                       if ('r' == c) {
+                                               c = '\r';
+                                       } else if ('n' == c) {
+                                               c = '\n';
+                                       } else if ('s' == c) {
+                                               c = ' ';
+                                       } else if ('t' == c) {
+                                               c = '\t';
+                                       } else if ('\\' == c) {
+                                               c = '\\';
+                                       } else if ('N' == c) {
+                                               c = '\0';
+                                       } else if ('b' == c) {
+                                               c = '\b';
+                                       } else if ('c' == c) {
+                                               cr = 0;
+                                               continue;
+                                       } else if (isdigit(c)) {
+                                               char *e;
+                                               c = strtoul(s, &e, 8);
+                                               if (!errno)
+                                                       s = e-1;
+                                               else ; //???
+                                       }
+                                       xwrite(STDOUT_FILENO, &c, 1);
+                               }
+                       }
+               } else if ('^' == *s) {
+                       char c = *++s-'@';
+                       xwrite(STDOUT_FILENO, &c, 1);
+               } else {
+                       xwrite(STDOUT_FILENO, s, 1);
+               }
+               s++;
+       }
+       if (cr)
+               xwrite(STDOUT_FILENO, "\r", 1);
+       alarm(0);
+
+       if (data)
+               free(data);
+}
+
+int chat_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
+int chat_main(int argc, char **argv)
+{
+       struct termios tio0, tio;
+       enum {
+               OPT_e = 1 << 0, // echo
+               OPT_E = 1 << 1, // use env      IGNORED
+               OPT_v = 1 << 2, // verbose      IGNORED
+               OPT_V = 1 << 3, // verbose2     IGNORED
+               OPT_s = 1 << 4, // to_stderr    IGNORED
+               OPT_S = 1 << 5, // no syslog    IGNORED
+               OPT_f = 1 << 6, // script file
+               OPT_t = 1 << 7, // timeout
+               OPT_r = 1 << 8, // report       IGNORED
+               OPT_T = 1 << 9, // phone 1      IGNORED
+               OPT_U = 1 << 10, // phone 2     IGNORED
+       };
+
+       enum {
+               DIR_HANGUP = 0,
+               DIR_ABORT,
+               DIR_CLR_ABORT,
+               DIR_REPORT,
+               DIR_CLR_REPORT,
+               DIR_TIMEOUT,
+               DIR_ECHO,
+               DIR_SAY,
+       };
+
+       // fetch options
+       unsigned opts;
+       char *file, *dummy;
+       opt_complementary = "t+";
+       opts = getopt32(argv, "eEvVsSf:t:r:T:U:", &file, &timeout, &dummy, 
&dummy, &dummy);
+       // process external script
+       if (opts & OPT_f) {
+               // read script
+//             char *script = xmalloc_open_read_close(file, NULL);
+               // parse script into argv?
+               // or may be rerun chat via execlp()?
+               //run_applet_and_exit(applet_name, argv);
+               // we never return from here...
+       }
+//     argc -= optind;
+       argv += optind;
+
+       // setup signals
+       sig_catch(SIGHUP,  signal_handler);
+       sig_catch(SIGINT,  signal_handler);
+       sig_catch(SIGTERM, signal_handler);
+       sig_catch(SIGPIPE, signal_handler);

This is an unfair question, but:
And why not SIGQUIT? SIGABRT? SIGALRM? SIGXCPU?
Why do you need to catch signals? To ensure that you reset back
the tcsetattr? Why do you need tcsetattr in the first place?

Readers of your code will benefit greatly from you
writing comments which explain above points.
"// setup signals" is a bad comment. It's obvious that signal
handlers are set up. It's unclear WHY they are set up, and that's
the thing you need to comment on.

+       // put stdin to "raw mode" (if stdin is a TTY),
+       // handle one character at a time
+       tcgetattr(STDIN_FILENO, &tio);
+       tio0 = tio;
+       cfmakeraw(&tio);
+       if (tcsetattr(STDIN_FILENO, TCSANOW, &tio))
+               bb_perror_msg_and_die("can't tcsetattr for STDIN");

I am not sure, but if stdin is not a _controlling tty_,
it will be unbuffered already, no?

+       // handle chat pairs
+       while (*argv) {
+//             bb_error_msg("PARAM: [%s]", *argv);
+               // directive given? process it
+               int key = index_in_strings(
+                       "HANGUP\0ABORT\0CLR_ABORT\0REPORT\0CLR_REPORT\0TIMEOUT\0ECHO\0SAY\0",
+                       *argv
+               );
+               if (key >= 0) {
+                       char *arg = *++argv;
+                       bool onoff = strcmp("OFF", arg);
+       //              bb_error_msg("DIRECTIVE: [%d][%s]", key, arg);
+                       if (DIR_HANGUP == key) {
+                               // turn SIGHUP on/off
+                               sig_catch(SIGHUP, onoff ? signal_handler : 
SIG_IGN);
+                       } else if (DIR_ABORT == key) {
+                               // set abort on string
+                               llist_add_to_end(&aborts, arg);
+                               if (opts & OPT_v)
+                                       bb_error_msg("abort on (%s)", arg);
+                       } else if (DIR_CLR_ABORT == key) {
+                               // clear previously set abort string
+                               for (llist_t *l = aborts; l; l = l->link) {
+                                       if (!strcmp(l->data, arg)) {
+                                               llist_unlink(&aborts, l);
+                                               if (opts & OPT_v)
+                                                       bb_error_msg("clear 
abort on (%s)", arg);
+                                               break;
+                                       }
+                               }
+/*                     } else if (DIR_REPORT == key) {
+                               // IGNORED
+                       } else if (DIR_CLR_REPORT == key) {
+                               // IGNORED
+*/                     } else if (DIR_TIMEOUT == key) {
+                               // set new timeout
+                               timeout = atoi(arg) * 1000;

You will miss bogus "numbers" like "qwe345". xatou?

+                               if (timeout <= 0)
+                                       timeout = DEFAULT_CHAT_TIMEOUT;
+                               if (opts & OPT_v)
+                                       bb_error_msg("timeout set to %u 
milliseconds", timeout);
+/*                     } else if (DIR_ECHO == key) {
+                               // turn echo on/off
+                               if (onoff)
+                                       opts |= OPT_e;
+                               else
+                                       opts &= ~OPT_e;
+*/                     } else if (DIR_SAY == key) {
+                               // just print argument
+                               bb_error_msg(arg);
+                       }
+                       argv++;
+               // wait for specific reply
+               } else {
+                       if ((signalled=chat_expect(*argv++)))

Please move assignment out of if(). Make it easy to read.

+                               break;
+                       if (*argv)
+                               // send command
+                               chat_send(*argv++);
+               }
+       }
+
+       tcsetattr(STDIN_FILENO, TCSANOW, &tio0);
+
+       return signalled;
+}

--
vda
_______________________________________________
busybox mailing list
[email protected]
http://busybox.net/cgi-bin/mailman/listinfo/busybox

Reply via email to