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.
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility
