Heitor Ricardo Alves de Siqueira <hal...@linux.vnet.ibm.com> writes:
> This commit rewrites some functions in iprconfig that are used to
> display log information to the user. We now use standard C code instead
> of system() calls and other OS utilities like grep. To view the
> requested log info, the less pager is invoked with the environment
> variable LESSSECURE set to 1 to prevent arbitrary command execution.
>
> This commit also introduces a zlib depency to iprutils. This is needed
> so that we can seamlessly work with compressed and uncompressed log files.
Thanks for the patch! A few stylish issues below:
> diff --git a/iprconfig.c b/iprconfig.c
> index fadf562062ea..644ba1de37da 100644
> --- a/iprconfig.c
> +++ b/iprconfig.c
> @@ -94,8 +94,7 @@ static int use_curses;
> #define for_each_raid_cmd(cmd) for (cmd = raid_cmd_head; cmd; cmd =
> cmd->next)
>
> #define DEFAULT_LOG_DIR "/var/log"
> -#define DEFAULT_EDITOR "vi -R"
> -#define FAILSAFE_EDITOR "vi -R -"
> +#define DEFAULT_EDITOR "/usr/bin/less"
I like moving the default editor from vi to less. But I guess we can
avoid using the absolute path here... you know the drill :)
>
> #define IS_CANCEL_KEY(c) ((c == KEY_F(12)) || (c == 'q') || (c == 'Q'))
> #define CANCEL_KEY_LABEL "q=Cancel "
> @@ -11935,44 +11934,69 @@ int ibm_storage_log_tail(i_container *i_con)
> int rc, len;
> int log_fd;
> char *logfile;
> + FILE *logsource;
>
> logfile = strdup(_PATH_TMP"iprerror-XXXXXX");
> log_fd = mkstemp(logfile);
> + if (log_fd < 0) {
> + closelog();
> + openlog(tool_name, LOG_PERROR | LOG_PID | LOG_CONS, LOG_USER);
> + syslog(LOG_ERR, "Could not create temp log file: %m\n");
> + closelog();
> + openlog(tool_name, LOG_PID | LOG_CONS, LOG_USER);
Why do you invoke closelog may times? Is that required? From the
manpage, closelog seems to be optional..
> + return 65;
Why 65? Is that expected by the caller? Can we change this to use a
constant or something more meaningful?
> + }
>
> def_prog_mode();
> + endwin();
>
> - if (log_fd == -1) {
> - len = sprintf(cmnd, "cd %s; zcat -f messages", log_root_dir);
> -
> - len += sprintf(cmnd + len," | grep ipr | ");
> - len += sprintf(cmnd + len, "sed \"s/ `hostname -s` kernel:
> ipr//g\" | ");
> - len += sprintf(cmnd + len, "sed \"s/ localhost kernel: ipr//g\"
> | ");
> - len += sprintf(cmnd + len, "sed \"s/ kernel: ipr//g\" | ");
> - len += sprintf(cmnd + len, "sed \"s/\\^M//g\" | ");
> - len += sprintf(cmnd + len, FAILSAFE_EDITOR);
> + snprintf(cmnd, sizeof(cmnd), "%s/messages", log_root_dir);
Guess we could use a more meaningful name than cmnd for this variable,
since it doesn't holds a command anymore, but a file name.
> + logsource = fopen(cmnd, "r");
> + if (logsource < 0) {
> closelog();
> - openlog("iprconfig", LOG_PERROR | LOG_PID | LOG_CONS, LOG_USER);
> - syslog(LOG_ERR, "Error encountered concatenating log
> files...\n");
> - syslog(LOG_ERR, "Using failsafe editor...\n");
> + openlog(tool_name, LOG_PERROR | LOG_PID | LOG_CONS, LOG_USER);
> + syslog(LOG_ERR, "Could not open %s: %m\n", cmnd);
> closelog();
> - openlog("iprconfig", LOG_PID | LOG_CONS, LOG_USER);
> - sleep(3);
> - } else {
> - len = sprintf(cmnd,"cd %s; zcat -f messages | grep ipr |",
> log_root_dir);
> - len += sprintf(cmnd + len, "sed \"s/ `hostname -s` kernel:
> ipr//g\"");
> - len += sprintf(cmnd + len, " | sed \"s/ localhost kernel:
> ipr//g\"");
> - len += sprintf(cmnd + len, " | sed \"s/ kernel: ipr//g\"");
> - len += sprintf(cmnd + len, " | sed \"s/\\^M//g\" ");
> - len += sprintf(cmnd + len, ">> %s", logfile);
> - system(cmnd);
> -
> - sprintf(cmnd, "%s %s", editor, logfile);
> + openlog(tool_name, LOG_PID | LOG_CONS, LOG_USER);
> + close(log_fd);
> + return 65;
> }
>
> - rc = system(cmnd);
> + while (fgets(cmnd, sizeof(cmnd), logsource)) {
> + /* ignore lines that dont contain 'ipr' */
> + if (strstr(cmnd, "ipr") == NULL)
> + continue;
>
> - if (log_fd != -1)
> - close(log_fd);
> + const char *local_s = "localhost kernel: ipr";
> + const char *kernel_s = "kernel: ipr";
> + char regex[256];
> + char host[256];
> + char *dot;
Please move the declarations to the top of the function or to the top of
the current code block. I find it bad to mix declarations with actual
code.
> +
> + gethostname(host, sizeof(host));
> + dot = strchr(host, '.');
> + if (dot)
> + *dot = '\0';
> + snprintf(regex, sizeof(regex), "%s kernel: ipr", host);
> +
> + /* erase regex from cmnd */
> + if (dot = strstr(cmnd, regex)) {
I was confused by the usage of cmnd here. Wasn't it pointing to the log
file name? Let's use another variable here, with a better name.
> + memmove(dot, dot + strlen(regex),
> + strlen(dot) - strlen(regex) + 1);
> + } else if (dot = strstr(cmnd, local_s)) {
> + memmove(dot, dot+strlen(local_s),
> + strlen(dot) - strlen(local_s) + 1);
> + } else if (dot = strstr(cmnd, kernel_s)) {
> + memmove(dot, dot+strlen(kernel_s),
> + strlen(dot) - strlen(kernel_s) + 1);
> + }
> +
> + write(log_fd, cmnd, strlen(cmnd));
I had a hard time understanding this block. Would please add a comment
in the code? :)
> int log_fd;
> char *logfile;
> -
> + FILE *logsource;
>
> sprintf(cmnd,"%s/boot.msg",log_root_dir);
> rc = stat(cmnd, &file_stat);
> @@ -12488,35 +12534,41 @@ int ibm_boot_log(i_container *i_con)
>
> logfile = strdup(_PATH_TMP"iprerror-XXXXXX");
> log_fd = mkstemp(logfile);
> + if (log_fd < 0) {
> + closelog();
> + openlog(tool_name, LOG_PERROR | LOG_PID | LOG_CONS, LOG_USER);
> + syslog(LOG_ERR, "Could not create temp log file: %m\n");
> + closelog();
> + openlog(tool_name, LOG_PID | LOG_CONS, LOG_USER);
> + return 65;
> + }
>
> def_prog_mode();
> + endwin();
>
> - if (log_fd == -1) {
> - len = sprintf(cmnd, "cd %s; zcat -f ", log_root_dir);
> -
> - /* Probably have a read-only root file system */
Do we still handle the case where the rootfs is RO?
> - len += sprintf(cmnd + len, "boot.msg");
> - len += sprintf(cmnd + len," | grep ipr |");
> - len += sprintf(cmnd + len, FAILSAFE_EDITOR);
> + snprintf(cmnd, sizeof(cmnd), "%s/boot.msg", log_root_dir);
> + logsource = fopen(cmnd, "r");
> + if (logsource < 0) {
> closelog();
> - openlog("iprconfig", LOG_PERROR | LOG_PID | LOG_CONS, LOG_USER);
> - syslog(LOG_ERR, "Error encountered concatenating log
> files...\n");
> - syslog(LOG_ERR, "Using failsafe editor...\n");
> + openlog(tool_name, LOG_PERROR | LOG_PID | LOG_CONS, LOG_USER);
> + syslog(LOG_ERR, "Could not open %s: %m\n", cmnd);
> closelog();
> - openlog("iprconfig", LOG_PID | LOG_CONS, LOG_USER);
> - sleep(3);
> - } else {
> - len = sprintf(cmnd,"cd %s; zcat -f boot.msg | grep ipr | "
> - "sed 's/<[0-9]>ipr-err: //g' | sed 's/<[0-9]>ipr:
> //g'",
> - log_root_dir);
> - len += sprintf(cmnd + len, ">> %s", logfile);
> - system(cmnd);
> - sprintf(cmnd, "%s %s", editor, logfile);
> + openlog(tool_name, LOG_PID | LOG_CONS, LOG_USER);
> + close(log_fd);
> + return 65;
> }
> - rc = system(cmnd);
>
> - if (log_fd != -1)
> - close(log_fd);
> + while (fgets(cmnd, sizeof(cmnd), logsource)) {
> + /* ignore lines that dont contain 'ipr' */
> + if (strstr(cmnd, "ipr") == NULL)
> + continue;
> +
> + write(log_fd, cmnd, strlen(cmnd));
> + }
> +
> + fclose(logsource);
> + close(log_fd);
> + rc = invoke_pager(logfile);
>
> if ((rc != 0) && (rc != 127)) {
> s_status.num = rc;
> diff --git a/iprlib.c b/iprlib.c
> index c5dfd8bb50c0..26c7a500d29b 100644
> --- a/iprlib.c
> +++ b/iprlib.c
> @@ -10079,3 +10079,23 @@ int ipr_jbod_sysfs_bind(struct ipr_dev *dev, u8 op)
> close(fd);
> return 0;
> }
> +
> +int invoke_pager(char *filename)
> +{
> + int pid, status;
> + char *argv[] = {
> + "/usr/bin/less", "-c",
Replace with macro DEFAULT_EDITOR.
> + filename, NULL
> + };
> +
> + pid = fork();
> + if (pid) {
> + waitpid(pid, &status, 0);
> + } else {
> + /* disable insecure pager features */
> + putenv("LESSSECURE=1");
> + execv(argv[0], argv);
> + _exit(errno);
> + }
> + return WEXITSTATUS(status);
> +}
> diff --git a/iprlib.h b/iprlib.h
> index 16fe1e162650..cec70cd976a7 100644
> --- a/iprlib.h
> +++ b/iprlib.h
> @@ -38,6 +38,7 @@
> #include <sys/stat.h>
> #include <sys/types.h>
> #include <sys/socket.h>
> +#include <sys/wait.h>
> #include <ctype.h>
> #include <syslog.h>
> #include <term.h>
> @@ -49,6 +50,7 @@
> #include <bits/sockaddr.h>
> #include <linux/netlink.h>
> #include <time.h>
> +#include <zlib.h>
>
> typedef uint8_t u8;
> typedef uint16_t u16;
> @@ -2906,6 +2908,7 @@ u32 get_ioa_ucode_version(char *);
> int ipr_improper_device_type(struct ipr_dev *);
> int ipr_get_fw_version(struct ipr_dev *, u8 release_level[4]);
> int ipr_get_live_dump(struct ipr_ioa *);
> +int invoke_pager(char *filename);
>
> static inline u32 ipr_get_dev_res_handle(struct ipr_ioa *ioa, struct
> ipr_dev_record *dev_rcd)
> {
> diff --git a/spec/iprutils.spec b/spec/iprutils.spec
> index 461d97799786..fe246e4f7b2c 100644
> --- a/spec/iprutils.spec
> +++ b/spec/iprutils.spec
> @@ -8,7 +8,7 @@ Vendor: IBM
> URL: http://sourceforge.net/projects/iprdd/
> Source0: iprutils-%{version}.tar.gz
> BuildRoot: %{_tmppath}/%{name}-root
> -BuildRequires: ncurses-devel
> +BuildRequires: ncurses-devel zlib-devel
>
> # Predicate whether we want to build -static package.
> %bcond_with static
This breaks static build. In a RHEL system, we have the zlib-static
package, other distros will have similar packages. Please add them as
proper build dependencies for the static subpackage.
Thanks,
--
Gabriel Krisman Bertazi
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=267308311&iu=/4140
_______________________________________________
Iprdd-devel mailing list
Iprdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/iprdd-devel
