Re: [PATCHSET] printk: implement printk_header() and merging printk, take #3
Tejun Heo wrote: Andrew Morton wrote: So, I guess it's NACK w/o suggested alternatives, right? I wouldn't nack without good reasons, and I have none here. I don't have very strong opinions either way. I was just wondering whether I should just go with snprintf dancing in eh_link_report, which does make sense if not many need merging printk. .. Any chance you could poke through snprintf() and look for the off-by-one bug on the return result? (I think it happens when n is exceeded). :) - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #3
Tejun Heo wrote: .. * For timeouts, result TF isn't available and thus res printout is misleading. res shouldn't be printed after timeouts. This would require allocating yest another temp buf and separating out res printing into separate snprintf. .. And snprintf() is buggy, by the way. It does not always seem to return the correct character fill counts. I've given up trying to use it here for anything in kernelspace that *must* work. Someday I'll go back and try to figure out where it's screwing up. I think it is doing so in the cases where it runs out of space in the buffer. -ml - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #3
Tejun Heo wrote: Hello, Andrew Morton wrote: On Thu, 14 Feb 2008 09:40:51 +0900 Tejun Heo [EMAIL PROTECTED] wrote: Can you please take a look at ata_eh_link_report() in drivers/ata/libata-eh.c? I did. Punishment? Heh.. :-) Currently, it has some problems. Yes, and the patches do clean that up. Yeap, it does. ho hum. What tends to happen with this sort of thing is that fi we merge it, it ends up getting used more often than one expected... Hmmm... Okay. mprintk being printk, I'm not too sure how it can go over usual expectations but well, yeah, that actually is my expectation. If you stand back and squint at it, there are quite a few places where we do this sort of thing: allocate a buffer, squirt characters into it, reallocating and/or flushing as we proceed. All sysfs and procfs read-side code, for a start... printk is a special case, I think. It's the primary logging/debugging method which can't fail and as it's mostly interpreted by human beings (and developers in problematic cases), it has different maneuvering room on errors - ie. it's far better to print messages w/o header or proper log level than failing to print, which is quite different requirements from other components. Andrew, any more comments or suggestions on how to proceed on this? One way or the other, I think this is a problem worth solving. Thanks. -- tejun - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #3
On Fri, 15 Feb 2008 10:49:31 +0900 Tejun Heo [EMAIL PROTECTED] wrote: printk is a special case, I think. It's the primary logging/debugging method which can't fail and as it's mostly interpreted by human beings (and developers in problematic cases), it has different maneuvering room on errors - ie. it's far better to print messages w/o header or proper log level than failing to print, which is quite different requirements from other components. Andrew, any more comments or suggestions on how to proceed on this? Nope. One way or the other, I think this is a problem worth solving. There are a lot of such problems ;) - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #3
On Fri, 15 Feb 2008 11:36:12 +0900 Tejun Heo [EMAIL PROTECTED] wrote: Andrew Morton wrote: printk is a special case, I think. It's the primary logging/debugging method which can't fail and as it's mostly interpreted by human beings (and developers in problematic cases), it has different maneuvering room on errors - ie. it's far better to print messages w/o header or proper log level than failing to print, which is quite different requirements from other components. Andrew, any more comments or suggestions on how to proceed on this? Nope. One way or the other, I think this is a problem worth solving. There are a lot of such problems ;) So, I guess it's NACK w/o suggested alternatives, right? I wouldn't nack without good reasons, and I have none here. I don't have very strong opinions either way. As a seat-of-the-pants thing, it does seem to be a lot of core code to solve a fairly minor problem in (afaik) one remote place. But I haven't looked - perhaps there are other places which could be improved if such facilities were available. - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #3
On Wed, 13 Feb 2008 18:09:28 +0900 Tejun Heo [EMAIL PROTECTED] wrote: This is the third take of implement-printk_header-and-mprintk patchset. Changes from the last take[L] are... * Now header is printed on every line of a multiline message. If the header ends with ':' followed by spaces. The colon is replaced with space from the second line. * s/mprintk/mprintk_flush/ and s/mprintk_add/mprintk/ as suggested. * mprintk_init_alloc() and mprintk_free() added to ease malloc'd buffer handling. No need to specify buffer size. Sizing is left to mprintk. This patchset aims to make printing multiline messages and assembling message piece-by-piece easier. In a nutshell, printk_header() lets you do the following atomically (against other messages). code: printk(KERN_INFO ata1.00: , line0\nline1\nline2\n); output: 6ata1.00: line0 6ata1.00 line1 6ata1.00 line2 That seems sensible. And mprintk the following. code: DEFINE_MPRINTK(mp, 2 * 80); mprintk_set_header(mp, KERN_INFO ata%u.%2u: , 1, 0); mprintk_push(mp, ATA %d, 7); mprintk_push(mp, , %u sectors\n, 1024); mprintk(mp, everything seems dandy\n); output: 6ata1.00: ATA 7, 1024 sectors 6ata1.00 everything seems dandy And that looks like an awful lot of fuss. Is it really worth doing? - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #3
Andrew Morton wrote: And mprintk the following. code: DEFINE_MPRINTK(mp, 2 * 80); mprintk_set_header(mp, KERN_INFO ata%u.%2u: , 1, 0); mprintk_push(mp, ATA %d, 7); mprintk_push(mp, , %u sectors\n, 1024); mprintk(mp, everything seems dandy\n); output: 6ata1.00: ATA 7, 1024 sectors 6ata1.00 everything seems dandy And that looks like an awful lot of fuss. Is it really worth doing? In the above example, s/mprintk(/mprintk_flush(/ and s/mprintk_push(/mprintk(/ Can you please take a look at ata_eh_link_report() in drivers/ata/libata-eh.c? Currently, it has some problems. * All that it wants to do is printing some messages but it's awfully complex with temp bufs and super-long printk calls containing optional %s's. * status / error decode print outs are printed separately from cmd/res making it difficult to tell how they are grouped. Putting them together would require allocating another temp buf(s) and adding extra %s's to cmd/res printout. * For timeouts, result TF isn't available and thus res printout is misleading. res shouldn't be printed after timeouts. This would require allocating yest another temp buf and separating out res printing into separate snprintf. I was trying to do this and got fed up with all the tricks in the function. The only sane way out is to build messages piece-by-piece into a buffer and print it at once. The eh message is gigantic and I needed to allocate the buffer elsewhere than stack. ata_eh_link_report() fortunately has fixed place for that - ap-sector_buf - but let's assume there was no such buffer for the discussion. I'm still not too sure whether it's wise to use sector_buf for message building anyway. The only way is to malloc the buffer. Once the buffer is available, building message using snprintf is a bit cumbersome but is okay. The problem is that malloc can fail. To handle that case, we basically need to do if (buf) printed += snprintf(buf + printed, len - printed, ...); else printk(...); which is very cumbersome, so we need a wrapper around the above. As the wrapper needs to control when the message goes out, a flush function is necessary too. Combine those with overflow handling - mprintk. Similar problem exists for ata_dev_configure() in drivers/ata/libata-core.c although it's a bit better there. Please take a look at the fifth patch. It doesn't remove a lot of lines but it streamlines both functions significantly. For ata_dev_configure(), message reporting becomes what the function does secondarily while configuring the device, not something it's structured around. Thanks. -- tejun - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #3
On Thu, 14 Feb 2008 09:40:51 +0900 Tejun Heo [EMAIL PROTECTED] wrote: Can you please take a look at ata_eh_link_report() in drivers/ata/libata-eh.c? I did. Punishment? Currently, it has some problems. Yes, and the patches do clean that up. ho hum. What tends to happen with this sort of thing is that fi we merge it, it ends up getting used more often than one expected... If you stand back and squint at it, there are quite a few places where we do this sort of thing: allocate a buffer, squirt characters into it, reallocating and/or flushing as we proceed. All sysfs and procfs read-side code, for a start... - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHSET] printk: implement printk_header() and merging printk, take #3
Hello, Andrew Morton wrote: On Thu, 14 Feb 2008 09:40:51 +0900 Tejun Heo [EMAIL PROTECTED] wrote: Can you please take a look at ata_eh_link_report() in drivers/ata/libata-eh.c? I did. Punishment? Heh.. :-) Currently, it has some problems. Yes, and the patches do clean that up. Yeap, it does. ho hum. What tends to happen with this sort of thing is that fi we merge it, it ends up getting used more often than one expected... Hmmm... Okay. mprintk being printk, I'm not too sure how it can go over usual expectations but well, yeah, that actually is my expectation. If you stand back and squint at it, there are quite a few places where we do this sort of thing: allocate a buffer, squirt characters into it, reallocating and/or flushing as we proceed. All sysfs and procfs read-side code, for a start... printk is a special case, I think. It's the primary logging/debugging method which can't fail and as it's mostly interpreted by human beings (and developers in problematic cases), it has different maneuvering room on errors - ie. it's far better to print messages w/o header or proper log level than failing to print, which is quite different requirements from other components. Thanks. -- tejun - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
