[Crash-utility] [PATCH] trace: Improve "trace show" command

[Lai Jiangshan]

On 03/23/2011 10:37 PM, Dave Anderson wrote:
> 
> 
> ----- Original Message -----
>> On 03/03/2011 11:25 PM, Dave Anderson wrote:
>>>
>>>
>>> ----- Original Message -----
>>>> The code were also applied to:
>>>> git://github.com/laijs/tracing-extension-module-for-crash.git
>>>>
>>>> Documents and man pages will/may be added in two weeks.
>>>>
>>>> Dave Anderson, could you add a "Requires" entry to its RPM.spec,
>>>> it requires trace-cmd RPM after this patch applied.
>>
>> Sorry, please omit this requirement of mine.
> 
> That actually would be a legitimate request for the RHEL6 crash-trace-command
> package, but I'd prefer not to add a Requires entry for the upstream src.rpm.
> 
> But I note in your new patch, that ftrace_show() is a void function, and
> if the popen("trace.cmd") fails, it just returns quietly.  I would think
> that there should be an explanatory error message in that case?  Also, 
> there's a buffer overflow error assignment to buf[4097]:
> 
> static void ftrace_show(int argc, char *argv[])
> {
>         char buf[4096];
>         char tmp[] = "/tmp/crash.trace_dat.XXXXXX";
>         char *trace_cmd = "trace-cmd", *env_trace_cmd = getenv("TRACE_CMD");
>         int fd;
>         FILE *file;
>         size_t ret;
> 
>         /* check trace-cmd */
>         if (env_trace_cmd)
>                 trace_cmd = env_trace_cmd;
>         if (!(file = popen(trace_cmd, "r")))
>                 return;
>         ret = fread(buf, 1, sizeof(buf), file);
>         buf[4097] = 0;
>         if (!strstr(buf, "trace-cmd version")) {
>                 if (env_trace_cmd)
>                         fprintf(fp, "Invalid environment TRACE_CMD: %s\n",
>                                         env_trace_cmd);
>                 else
>                         fprintf(fp, "\"trace show\" requires trace-cmd.\n"
>                                         "please set the environment TRACE_CMD "
>                                         "if you installed it a special path\n"
>                                         );
>                 return;
>         }
> 
> So if you don't mind, I'll fix it like this:
> 
>         buf[0] = 0;
>         if ((file = popen(trace_cmd, "r"))) {
>                 ret = fread(buf, 1, sizeof(buf), file);
>                 buf[4095] = 0;
>         }

It seems better:
	if (...) {
		ret = fread(buf, 1, sizeof(buf) - 1, file);
		buf[ret] = 0;
	}

>         if (!strstr(buf, "trace-cmd version")) {
>                 if (env_trace_cmd)
>                         fprintf(fp, "Invalid environment TRACE_CMD: %s\n",
>                                         env_trace_cmd);
>                 else
>                         fprintf(fp, "\"trace show\" requires trace-cmd.\n"
>                                         "please set the environment TRACE_CMD "
>                                         "if you installed it in a special path\n"
>                                         );
>                 return;
>         }
> 
> If that's OK with you, the patch is queued for crash 5.1.4.
> 

OK with me, thanks,

Lai.