[sane-devel] coolscan2 in 1.0.8

[Major A]

Hi Frank,

Thanks for auditing the code so quickly!

> - most private symbols are exported (functions and variables). You must
> declare them as static.

Fixed (in CVS).

> - device_list is never freed. Actually most of the backend is leaking
> memory. Overall I don't see the point in allocating every single
> structure (for instance option_list could be a static member of cs2_t).

Fixed. I think I made it dynamic at the time when I wasn't quite clear
of the way the options worked in the SANE protocol.

> - there is two occurrences of exit(), which is not allowed.

OK, I eliminated them now. In the case in which exit() would have
occured, a SIGSEGV will now occur, which is not good either, but I'll
try and find a way around that.

> - cs2_open() can leak file handles

Are you sure?

> - the backend does not have a man page.

True, I'll add one once it's in the SANE tree. For now, there is a
README.

> - why don't you have a header file to store all those defines and
> structures?

I don't think it's necessary, simply because they are only ever
required by coolscan2.c, no other files.

> - in cs2_scanner_ready(), you should move the usleep a the end of the
> while loop. Maybe this function need a full rewrite? And why it is
> returning a value since only one caller checks for it?

This is a bit tricky, and that part of the code is hard to understand
(well, some might say dirty, which doesn't mean isn't portable and
robust). The idea is that the usleep() should not happen at the
beginning or the end of the timeout count loop, simply to eliminate
any extra delays. The negative initial value of i makes sure that
usleep() is not called when we start waiting. In case the scanner is
ready already, it's not necessary to do any usleep() at all.

As to the return value, it is necessary for that single instance, but
not for the others. I haven't found a better way of implementing a
function that takes as an argument a mask for the state it should be
waiting for, but still needs to return the precise state it ended up
with.

> - comments are scarse

Will add some later.

> - add a $Id:$ tag to keep track of the cvs version.

I will do that when/if it makes it into SANE CVS.

Thanks a lot,

  Andras

===========================================================================
Major Andras
    e-mail: andras@users.sourceforge.net
    www:    http://andras.webhop.org/
===========================================================================