Hello,
Since I have started this thread, I feel I have to do something real.
Dummy technical work, but someone have to do it, right?
I patched USB subsystem, it uses __FUNCTION__ in deprecated way no more.
What is changed?
- in usb.h, I modified dbg(), warn(), err(), info() macros to contain
function name in the prefix. These macros are for simple one-line messages.
- in all files under drivers/usb, all macros mentioned fixed.
- all __FUNCTION__ occurencies in drivers/usb revised.
I compiled kednel with all USB modules enabled. Since everything
compiles OK and all changes are in message formats, this patch should
not corrupt anything. In the worst case you will get badly formatted
message.
Patch is against 2.4.17
Comments?
Please, when replying, CC me: mailto:[email protected]
On Thu, Jan 10, 2002 at 12:12:29AM +0200, Vladimir Kondratiev wrote:
> Hello,
> Since I have started this thread, I feel I have to do something real.
> Dummy technical work, but someone have to do it, right?
> I patched USB subsystem, it uses __FUNCTION__ in deprecated way no more.
> What is changed?
> - in usb.h, I modified dbg(), warn(), err(), info() macros to contain
> function name in the prefix. These macros are for simple one-line messages.
> - in all files under drivers/usb, all macros mentioned fixed.
> - all __FUNCTION__ occurencies in drivers/usb revised.
>
> I compiled kednel with all USB modules enabled. Since everything
> compiles OK and all changes are in message formats, this patch should
> not corrupt anything. In the worst case you will get badly formatted
> message.
Your patch makes whitespace changes to a lot of dbg() statements, but
does not modify their contents. Can you please change this, as this
change does not need to happen.
> Patch is against 2.4.17
2.4.18-pre2 has a _lot_ of usb changes and this patch misses a number of
places.
I'd also like to see this against the 2.5.x tree first, as the
recommended compiler for the 2.4.x tree is still 2.95.3, and I don't
think that will change anytime soon.
thanks,
greg k-h
On Thu, Jan 10, 2002 at 12:12:29AM +0200, Vladimir Kondratiev wrote:
> Since I have started this thread, I feel I have to do something real.
> Dummy technical work, but someone have to do it, right?
> I patched USB subsystem, it uses __FUNCTION__ in deprecated way no more.
#ifdef DEBUG
-#define dbg(format, arg...) printk(KERN_DEBUG __FILE__ ": " format "\n" , ## arg)
+#define dbg(format, arg...) printk(KERN_DEBUG __FILE__ ":%s - " format "\n", __FUNCTION__, ## arg)
#else
I was going to suggest you use the C99 __func__ rather than
__FUNCTION__ but after a quick test it doesn't seem to be supported by
egcs-2.91.66 so I guess that is out for the time being? It is
supported by gcc-2.95 though.
--
Nick Craig-Wood
[email protected]
Greg KH wrote:
>
>Your patch makes whitespace changes to a lot of dbg() statements, but
>does not modify their contents. Can you please change this, as this
>change does not need to happen.
>
Yes, I realized it, but unfortunately too late. It's hard to revise all
patch chunk by chunk and undo changes when content not changed.
>>Patch is against 2.4.17
>>
>
>2.4.18-pre2 has a _lot_ of usb changes and this patch misses a number of
>places.
>
>I'd also like to see this against the 2.5.x tree first, as the
>recommended compiler for the 2.4.x tree is still 2.95.3, and I don't
>think that will change anytime soon.
>
>thanks,
>
>greg k-h
>
Patch against 2.4.18-pre2 attached. For 2.5 tree - wait a bit, I have to
return for a moment to business I get salary for.
Nick Craig-Wood wrote:
>
>I was going to suggest you use the C99 __func__ rather than
>__FUNCTION__ but after a quick test it doesn't seem to be supported by
>egcs-2.91.66 so I guess that is out for the time being? It is
>supported by gcc-2.95 though.
>
I considered it too. Unfortunately, __func__ is not supported by all gcc
versions, so __FUNCTION__ seems to be best solution so far. Also, I
aggregated all changes to one place in macro definition, it should be
easier to change, should this need arise.
>
On Thu, Jan 10, 2002 at 12:52:57PM +0200, Vladimir Kondratiev wrote:
> Patch against 2.4.18-pre2 attached. For 2.5 tree - wait a bit, I have to
> return for a moment to business I get salary for.
You still are changing a few dbg() macros that you don't have to change.
Also, info(), warn() and err() should not have __FUNCTION__ added to
them. Have you tried running the usb code with this patch? The USB
group gets enough grief about all of the kernel log messages that we
spit out. We do not need to see the function name for every message we
write (the user does not need it.)
I think I'll wait for the debug level messages cleanup in the 2.5 USB
code to make this kind of change (as talked about in a previous
message.)
thanks,
greg k-h
Greg KH wrote:
>You still are changing a few dbg() macros that you don't have to change.
>
Guilty. Goal was to force some common style. There are mix of function
call styles like f() and f (). I suppose f() (without space between
function name and parenthes) is better, but it is of course religious
issue and have no 'proper' solution.
>
>Also, info(), warn() and err() should not have __FUNCTION__ added to
>them. Have you tried running the usb code with this patch? The USB
>group gets enough grief about all of the kernel log messages that we
>spit out. We do not need to see the function name for every message we
>write (the user does not need it.)
>
Questionable. There are lots of calls that used to have __FUNCTION__,
ex. 1-st chunks from patch are:
--- linux-2.4.18-pre2.orig/drivers/usb/CDCEther.c Fri Dec 21
19:41:55 2001
+++ linux-2.4.18-pre2.patched/drivers/usb/CDCEther.c Thu Jan 10
10:28:21 2002
- warn( __FUNCTION__ " failed submint rx_urb %d", res);
+ warn("failed submint rx_urb %d", res);
- err( "write_bulk_callback: device not running" );
+ err( "device not running" );
It is hard to be both flexible and easy. I suppose always have function
name in the log is a good idea. Maybe, I would even add line number.
Anyway, original macros used to add __FILE__ prefix. All these stuff
goes to syslog, and user usually do not see it at all. If one wants to
browse /var/log/messages, he is supposed to understand a little what he
going to see, and function names may help.
In places where great flexibility is required, I'd propose to keep using
printk().
>
>I think I'll wait for the debug level messages cleanup in the 2.5 USB
>code to make this kind of change (as talked about in a previous
>message.)
>
>thanks,
>
>greg k-h
>
What about macros for simple one line messages for generic usage? Any
thoughts?
I propose the following messages format:
<module>:<file>:<line>:<function> - <message_text>
Corresponded macro would be, for example:
#ifdef MODULE
#define err(format, arg...) printk(KERN_ERR "%s:%s:%d:%s - " format "\n"
,THIS_MODULE->name ,__FILE__, __LINE__, __FUNCTION__, ## arg)
#else
#define err(format, arg...) printk(KERN_ERR "%s:%d:%s - " format "\n" ,
__FILE__, __LINE__, __FUNCTION__, ## arg)
#endif