On 01/13/2016 11:08 AM, Gabriel Krisman Bertazi wrote:
> 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:
>
Hi Gabriel,
Thanks for taking the time to sift through the patch! A few comments to
your review 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 :)
>
Done. :)
>>
>> #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..
>
You're right, we don't really need this. I think this was a workaround
to print the message to syslog and stdout at the same time, but I've
changed this to use the return code messages.
>> + return 65;
>
> Why 65? Is that expected by the caller? Can we change this to use a
> constant or something more meaningful?
>
Yes, turns out that it is an expected value. Depending on this
function's return value, a different error message will be displayed to
the user. I've changed the code to use some defined constants for this,
should make it a bit more clear.
>> + }
>>
>> 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.
>
Done.
>> + 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.
>
You're right, that was sloppy from me. I've fixed this in v2.
>> +
>> + 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.
>
Also done.
>> + 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? :)
>
I can see that this block is confusing, sorry for that. That is usually
not a good sign, so if you have any suggestions for this, please let me
know. For now, I added some clarification to 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?
>
Not directly, no. This will probably fail in that case, because we may
not be able to create a new file with mkstemp. I don't see a very good
way to handle a RO rootfs with the current code, since we need a temp.
file to filter log info before we show it to the user. Do you have any
ideas for this, or should we just fail and inform the user?
>
>> - 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.
Done.
>> + 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.
>
Right. I had some difficulties with that, but as we talked in private
these should be sorted out now. I did some tests on building and it
looks alright now. If I missed anything again, please let me know.
I'll send v2 to the list shortly.
> Thanks,
>
Thank you,
--
Heitor Ricardo Alves de Siqueira
IBM Linux Technology Center
------------------------------------------------------------------------------
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
