Re: [Libusb-devel] About hotplug support in libusb [contains patch for libusb-1.0.8]

[Nathan H. <hj...@me...>]

I want to hear Dan's feedback as well. I know he has already taken a crack at defining the hotplug extension to the API. Once I know how he wants to proceed I will give you more in depth feedback on the changes to the darwin backend.

Some quick comments:
 - When modifying a file please code in a style consistent with author's style. It makes integration much easier! (i don't use // comments or gotos for example)
 - There shouldn't be a need to have multiple mach ports or event threads. Once I head how Dan's wants to proceed I will look into this and we can discuss the best way to proceed.
 - It would have been great if you had been active on this mailing list earlier.
 - Same comments as Ludovic.

Thanks for the patch!

-Nathan

On Aug 4, 2010, at 5:58 AM, Ludovic Rousseau wrote:

> 2010/8/4 Vil...@no... <Vil...@no...>:
>> Hello all,
>> For the purposes of providing USB connectivity support for certain types of wireless communication devices on Mac OS X and on Linux we decided to try out libusb. So first of all, thank you all for the great work you have done. However, some of our use-cases depend on having support for hotplug events and currently libusb does not provide that. Since we really wanted to use the library we thought of doing some work for the hotplug support on our own.
> 
> Thanks for the patch.
> 
> +static inline unsigned long hash(const char* sz)
> +{
> +    unsigned long r = 5381;
> +    int c;
> +    while ((c = *sz++))
> +        r = ((r << 5) + r) + c; /* r * 33 + c */
> +    return r;
> +}
> 
> Where does this hash function comes from?
> I do not object to the code. I am just curious to know if it is an
> already known algorithm.
> 
> Why don't you let the compiler optimize the code for you and just use
> "r = r * 33 + c"?
> I have read a paper indicating that "modern" compiler are better than
> humans at optimisations like these.
> The compiler is also able to inline functions itself when appropriate.
> 
> 
> Your patch is big and includes changes that are not related to the
> hotplug mechanism.
> For example (in libusb/os/darwin_usb.c):
> 
> -    pthread_exit ((void *)kresult);
> +    pthread_exit ((void *) &kresult);
> 
> And I think this patch is wrong. kresult is a local variable so I am
> not sure the pointer will point to a valid value after pthread_exit()
> 
> 
> Another example (in libusb/os/linux_usbfs.c):
> 
> @@ -653,8 +701,7 @@
> 		if (errno == ENODEV)
> 			return LIBUSB_ERROR_NO_DEVICE;
> 
> -		/* we hit this error path frequently with buggy devices :( */
> -		usbi_warn(DEVICE_CTX(dev),
> +		usbi_err(DEVICE_CTX(dev),
> 			"get_configuration failed ret=%d errno=%d", r, errno);
> 		return LIBUSB_ERROR_IO;
> 	}
> 
> 
> But I don't know how Daniel Drake (libusb maintainer) wants to handle
> your patch.
> 
> Regards,
> 
> -- 
>  Dr. Ludovic Rousseau
> 
> ------------------------------------------------------------------------------
> The Palm PDK Hot Apps Program offers developers who use the
> Plug-In Development Kit to bring their C/C++ apps to Palm for a share
> of $1 Million in cash or HP Products. Visit us here for more details:
> http://p.sf.net/sfu/dev2dev-palm
> _______________________________________________
> Libusb-devel mailing list
> Lib...@li...
> https://lists.sourceforge.net/lists/listinfo/libusb-devel