On Sun, 08 Oct 2006 22:34:17 +0200, Paolo Abeni <[email protected]> wrote:
> An ioctl method is added to each usbmon text files. The ioctl implements
> two operation: retrieving the max size of data that usbmon is able to
> capture for each URB, and changing this value.
>[...]
> This change is useful to get full content of exchanged URB. A recently
> introduced libpcap patch make it possible to sniff from USB port via
> usbmon, but the full USB packet contents is currently unavailable.
I wish whoever did the libpcap fixes designed a new, non-text API instead.
There is no reason to format URBs into text for that.
Is there no chance of that happening?
Also, the patch is a little dirty stylistically, not to mention full of
little, annoying buglets.
> +/* ioctl macros */
> +#define MON_IOC_MAGIC 0xff
Oh bah. Could you at least use something like 0x92 or whatever?
You are going to confuse strace and if you do this.
> +#define MON_IOCS_DATA_MAX _IOW(MON_IOC_MAGIC, 1, int)
> +#define MON_IOCG_DATA_MAX _IOR(MON_IOC_MAGIC, 2, int)
Is that right? You are passing pointers to int, so ioctl numbers get
wrong sizes. This is important on some platforms, and doubly so when
32-bit binaries run under 64-bit kernel.
> + int ret=0, pos;
This needs to be made prettier, on separate lines and with spaces.
> + /* try to allocate early all required buffer and preserve old values
> + * in case of failure */
> + if (!(new_printf_buf = kmalloc(printf_size, GFP_KERNEL)))
> + {
I'd appreciate if this looked like this:
/*
* Blah blah blah
*/
if ((new_prinf_buf = kmalloc(size, GFP)) == NULL) {
}
> + if (!(new_e_slab = kmem_cache_create(new_name,
> + sizeof(struct mon_event_text)+data_max, sizeof(long), 0,
> + mon_text_ctor, NULL))) {
> + kfree(new_printf_buf);
> + ret = -1;
> + goto out;
> + }
This is a highly dubious way to allocate, when we talk about data_max 1024.
> + /* event list must be flush because old entries have differnd size */
"flushed", and "different".
> +static inline struct mon_event_text * mon_event_alloc(
> + struct mon_reader_text *rp)
> +{
> + struct mon_event_text *ep = kmem_cache_alloc(rp->e_slab, SLAB_ATOMIC);
> + if (!ep)
> + return 0;
> + ep->data = ((unsigned char*)ep) + sizeof(struct mon_event_text);
> + return ep;
> +}
Aside from needlessly made inline, this is not bad. But you
obviously thought about out-of-line data, why stop here?
> @@ -249,7 +340,7 @@ static int mon_text_open(struct inode *i
> INIT_LIST_HEAD(&rp->e_list);
> init_waitqueue_head(&rp->wait);
> mutex_init(&rp->printf_lock);
> -
> +
> rp->printf_size = PRINTF_DFL;
Pointless whitespace addition. Don't.
> + int ret=0, new_data_max;
Same syntax as before.
> + u32 __user *argp = (u32 __user *)arg;
>[.]
> + case MON_IOCS_DATA_MAX:
> + if (get_user(new_data_max, argp))
This obviously is not needed, because the new size would fit the ioctl
argument just fine.
> + default:
> + ret = -ENOTTY;
Good!
Anyway, maybe I should look into it closer. What do you use on top of
libpcap? Wireshark?
-- Pete
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
