Hello Nick, (2013/08/16 1:29), Nick Bartos wrote: > You do bring up an interesting point. What do you think about just using > the buffer to handle writing the timestamp (which could actually be a smaller > buffer than is used now), then writing the rest of the message 1 character > at a time as it is read? That will make the process take a little bit longer > due to inefficient use of i/o, however in this case I do not believe it will > take much longer since dmesg is not that big. Additionally this would fix the > problem of possible message truncation (and of make the code simpler), > which I think is worth the small speed penalty. I prefer to use the buffer to reduce the system call count. Additionally, the current buffer size is only 1k bytes, so I don't think to reduce memory consumption smaller than now is meaningful. Thanks Atsushi Kumagai > > On Thu, Aug 15, 2013 at 12:56 AM, Atsushi Kumagai <kumagai-atsushi at mxc.nes.nec.co.jp <mailto:kumagai-atsushi at mxc.nes.nec.co.jp>> wrote: > > Sorry for late reply. > > On Tue, 6 Aug 2013 08:56:56 -0700 > Nick Bartos <nick at pistoncloud.com <mailto:nick at pistoncloud.com>> wrote: > > > Yes you are correct on changing DEBUG_MSG to ERRMSG. I didn't read enough > > of the code to see that was an option. > > > > Each buffer size check does something specific, which was intended to > > handle all failure cases. While it's probably possible to do the same > > checks in less actual code, simply removing any of the checks will make it > > fail in some cases. Also, if someone sets BUFSIZE to a very small number > > when nothing will work, it makes sense to exit with failure without trying > > to read anything. However if only a few lines are going to be truncated > > because they are too long, it makes sense to continue on and get as much > > debug output as possible (but with printing the error message about > > truncation). > > I also think it's good to output messages as much as possible even if they > are truncated. > > Now, I hit upon another idea. How about dividing the read and write processing > in multiple parts like below? This way can treat any length message with > fixed BUFSIZE without truncation. > > > message [ 112.919861] sd 2:1:2:0: [sdf] Unhandled error code\n\0 > > |---- BUFSIZE -----|---- BUFSIZE -----|---- BUFSIZE -----| > read/write 1 2 3 > times > > > If that is OK with you, could you post your v2 patch ? > > > Thanks > Atsushi Kumagai > > > Good catch on using a null character, that makes more sense as the former > > would likely result in a compiler warning. > > > > > > On Mon, Aug 5, 2013 at 9:15 PM, Atsushi Kumagai < > > kumagai-atsushi at mxc.nes.nec.co.jp <mailto:kumagai-atsushi at mxc.nes.nec.co.jp>> wrote: > > > > > Hello Nick, > > > > > > On Mon, 29 Jul 2013 09:27:57 -0700 > > > Nick Bartos <nick at pistoncloud.com <mailto:nick at pistoncloud.com>> wrote: > > > > > > > I am reporting this bug as requested in the "BUG REPORT" of > > > > the makedumpfile README. > > > > > > > > There are several times when sprintf is used to append to a buffer like > > > so: > > > > sprintf(buf, "%s.", buf); > > > > > > > > In the sprintf manpage, it describes that behavior is not consistent. I > > > > first noticed this as my hardned system would make all dmesg output lines > > > > empty. Here is the excerpt from the man page: > > > > > > > > C99 and POSIX.1-2001 specify that the results are undefined if a > > > call > > > > to sprintf(), snprintf(), vsprintf(), or vsnprintf() would cause > > > copying > > > > to take place between objects that overlap (e.g., if the target string > > > > array and one of the supplied input arguments refer to the same buffer). > > > > See NOTES. > > > > > > > > NOTES > > > > Some programs imprudently rely on code such as the following > > > > > > > > sprintf(buf, "%s some further text", buf); > > > > > > > > to append text to buf. However, the standards explicitly note > > > that > > > > the results are undefined if source and destination buffers overlap > > > > when calling sprintf(), snprintf(), vsprintf(), and vsnprintf(). > > > > Depending on the version of gcc(1) used, and the compiler options > > > employed, > > > > calls such as the above will not produce the expected results. > > > > > > > > The glibc implementation of the functions snprintf() and > > > vsnprintf() > > > > conforms to the C99 standard, that is, behaves as described above, > > > since > > > > glibc version 2.1. Until glibc 2.0.6 they would return -1 when the > > > output > > > > was truncated. > > > > > > > > > > > > In addition to using sprintf improperly, it appears that there may be > > > > buffer overflow potential in the current code, since the loop does not > > > stop > > > > if the size of buf is exhausted. > > > > > > > > I have attached a patch which modifies this behavior. > > > > > > I have some comments on this patch, please see below. > > > > > > > diff -U3 -r makedumpfile-1.5.4.orig/makedumpfile.c > > > makedumpfile-1.5.4/makedumpfile.c > > > > --- makedumpfile-1.5.4.orig/makedumpfile.c 2013-07-01 > > > 08:08:49.000000000 +0000 > > > > +++ makedumpfile-1.5.4/makedumpfile.c 2013-07-22 22:47:34.195721706 +0000 > > > > @@ -3701,6 +3701,7 @@ > > > > char *msg, *p; > > > > unsigned int i, text_len; > > > > unsigned long long ts_nsec; > > > > + int buf_s; > > > > char buf[BUFSIZE]; > > > > ulonglong nanos; > > > > ulong rem; > > > > @@ -3713,21 +3714,49 @@ > > > > > > > > msg = logptr + SIZE(log); > > > > > > > > - sprintf(buf, "[%5lld.%06ld] ", nanos, rem/1000); > > > > + buf_s = snprintf(buf, BUFSIZE, "[%5lld.%06ld] ", nanos, rem/1000); > > > > + if (buf_s < 0) { > > > > + DEBUG_MSG("snprintf returned an error in > > > dump_log_entry.\n"); > > > > + return FALSE; > > > > + } > > > > > > This is the error case, so you should use ERRMSG instead of DEBUG_MSG. > > > The same can be said about all other messages. > > > > > > > + if (buf_s >= BUFSIZE) { > > > > + DEBUG_MSG("snprintf truncated output in > > > dump_log_entry.\n"); > > > > + DEBUG_MSG(" increase BUFSIZE and recompile.\n"); > > > > + /* No point in continuing, we can't append anything useful > > > */ > > > > + return FALSE; > > > > + } > > > > + > > > > + /* It's unlikely but possible we may not have enough to terminate > > > the > > > > + string */ > > > > + if (buf_s >= BUFSIZE - 1) { > > > > + DEBUG_MSG("not enough buffer space in dump_log_entry.\n"); > > > > + DEBUG_MSG(" increase BUFSIZE and recompile.\n"); > > > > + return FALSE; > > > > + } > > > > + > > > > + for (i = 0, p = msg; i < text_len; i++, p++, buf_s++) { > > > > + if (buf_s >= BUFSIZE - 2) { > > > > + DEBUG_MSG("output truncated in dump_log_entry.\n"); > > > > + DEBUG_MSG(" increase BUFSIZE and recompile.\n"); > > > > + break; > > > > + } > > > > > > I don't think to check the remaining buffer space 3 times is meaningful, > > > they indicate just "lack of buffer space". > > > Is the last one enough to check the remaining buffer space ? > > > > > > > > > > > - for (i = 0, p = msg; i < text_len; i++, p++) { > > > > if (*p == '\n') > > > > - sprintf(buf, "%s.", buf); > > > > + buf[buf_s] = '.'; > > > > else if (isprint(*p) || isspace(*p)) > > > > - sprintf(buf, "%s%c", buf, *p); > > > > + buf[buf_s] = *p; > > > > else > > > > - sprintf(buf, "%s.", buf); > > > > + buf[buf_s] = '.'; > > > > } > > > > > > > > - sprintf(buf, "%s\n", buf); > > > > + buf[buf_s] = '\n'; > > > > + buf_s++; > > > > + buf[buf_s] = NULL; > > > > > > Null character(\0) should be used to terminate strings instead of > > > null pointer. > > > > > > > > > > > - if (write(info->fd_dumpfile, buf, strlen(buf)) < 0) > > > > + if (write(info->fd_dumpfile, buf, strlen(buf)) < 0) { > > > > + DEBUG_MSG("Problem writing to dump file.\n"); > > > > return FALSE; > > > > + } > > > > else > > > > return TRUE; > > > > } > > > > > > > > > Thanks > > > Atsushi Kumagai > > > > >
