Kurt Van Dijck wrote: > On Mon, Dec 14, 2009 at 02:06:12PM +0100, Wolfgang Grandegger wrote: >> Kurt Van Dijck wrote: >>>>>> I still believe that the message and the backtrace does not help the >>>>>> user to understand that he has done something wrong. Nevertheless, if >>>>>> there is nobody else sharing my concerns, please go ahead preparing the >>>>>> patch as you suggested above. The future will show if they are >>>>>> reasonable. >>>>> I share a bit the concern, but if I understand well, this is to avoid >>>>> PF_PACKET sockets to break CAN rules. When regular PF_CAN is used, the >>>>> user will be informed via can_send in af_can.c, isn't it? >>>> Yes, that's also my understanding: >>>> >>>> http://lxr.linux.no/#linux+v2.6.32/net/can/af_can.c#L218 >>> Wolfgang, >>> >>> To address your concern, would a construction like this suit, when >>> fitted in Oliver's proposal? >>> >>> inline int no_can_skb((struct sk_buff *skb) >>> { >>> struct can_frame *cf = (struct can_frame *)skb->data >>> >>> if ((skb->len != sizeof(*cf)) || ((cf->can_dlc > 8)) { >>> if (skb->sk && !sock_flag(skb->sk, SOCK_DEAD)) { >>> skb->sk->sk_err = EINVAL; >>> skb->sk->sk_error_report(skb->sk); /* can this block?*/ >>> } >>> >>> WARN_ONCE(1, "non conform skbuf: ..."); >>> "Dropped non conform skbuf: len %d, can_dlc %d\n", >>> skb->len, cf->can_dlc); >>> return 1; >>> } >>> return 0; >>> } >> My primary concern is about using *WARN_ONCE*. The BUG, WARN, functions >> and friends indicate to the user that there is a problem with the >> kernel, e.g. a bug and therefore I prefer a simple dev_err(). Also the > I see. > I agree that a dev_err seems more appropriate here. > Since this is TX path, kernel message flood is with respect to local > activity?
Indeed i would like to have a PRINTK_ONCE() or DEV_ERR_ONCE() :-) >> word "skbuf" does say little to the normal Linux users. I find >> s/skbuf/packet/ more intuitive. Of course, if there is a better way to > ack. >> inform the user we should use it. Unfortunately, I can't tell if your >> approach will work. > I went into the code, in net/core/sock.c:sock_def_error_report. > that looks like atomic code to me, so the above sk_error_report() stuff > should work. > no_can_skb() would not even need a dev_err in that case. Using sk_error_report() and leaving out dev_err() is an interesting idea! Is that a usual way to provide this kind of error notification, e.g. for broken PF_PACKET packets? Regards, Oliver _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
