[sane-devel] sanei usb improvements

[Olaf Meeuwissen]

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