Stef writes: > On 08/03/2013 00:45, Olaf Meeuwissen wrote: >> Stef writes: >> >>> On 05/03/2013 01:35, Olaf Meeuwissen wrote: >>>> Stef writes: >>>> >>>>> here's a patch set to improve sanei_usb to review. >>>> Patches 2, 3 and 4 are fine with me but the first patch mixes several >>>> changes that make it unnecessarily hard to review. Could split that >>>> patch so it focusses on the advertised changeset and refrain from the >>>> small changes in whitespace, comments and debugging feedback? >> Sorry for the late follow-up. I don't care for the later patches so I >> only looked at: >> >> 0001-sanei_usb_init-rework.patch >> 0002-add-sanei_usb_exit-function.patch >> >> The call to kernel_scan_devices() in sanei_usb_scan_devices() struck me >> as odd. At least for linux-2.6.3 or later it is not needed because >> there is no kernel scanner module anymore. But looking at the code, I >> noticed that it may be necessary on other OSs. Still, if using one of >> the libusb libraries on any OS, you should be fine without the kernel >> scan, no? >> >> In sanei_usb_exit() you should reset sanei_usb_ctx to NULL because you >> check for it in sanei_usb_init(). This should be fixed. >> >> As a matter of style, in sanei_usb_exit(), I would put the "int i" in >> the "if (initialized==0)" branch, limiting its existence to the scope >> where it is needed. >> >> I'll leave commenting on usbcall_scan_devices() to somebody who is >> familiar with that API (but it looks like you just lifted that from its >> original location) and I skipped all the changes you made to the >> debugging messages in the first patch (as these are off-topic). >> >> Hope this helps, > Hello, > > thanks for your time and the feedback. > I thought the same about kernel scanner device. As you suggest, I > will make exclusive from libusb, so it will be used only is there no > libusb library available. I will also reset ctx to null. > Regarding the debugging message, since they are moving to another > function they had to be changed because they print the function name as > part of the message. I choose to use __FUNCTION__ so I don't have to > change them again if I put them in another function when splitting big > functions, or if I have to change the name of the enclosing function.
OK, I see. I missed this little detail during my review where I tried to focus on the logic. But of course you're right that the debugging feedback needs to be adjusted. It's just that I would have done that in a separate changeset, no big deal. Please note that __FUNCTION__ is not part of any C standard, but __func__ is in C99[1]. Hmm, both are used in the backends. A majority (of files) vote would favour __FUNCTION__. I guess either way is fine. [1] http://gcc.gnu.org/onlinedocs/gcc/Function-Names.html#Function-Names Hope this helps, -- Olaf Meeuwissen, LPIC-2 FLOSS Engineer -- AVASYS CORPORATION FSF Associate Member #1962 Help support software freedom http://www.fsf.org/jf?referrer=1962
