Cleanup and small fixes

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Cleanup and small fixes

Volker Diels-Grabsch
Dear SANE developers,

I'm new to SANE and I want to improve the hp5590 backend driver.
As a start, to "get into" the code base, I fixed various minor
issues, mostly compiler warnings and typos.

I'm not sure which is the best way to contribute, so I'm providing
this as a series of (mostly) independent patches to this mailing list.
I could also open one or more tickets in the issue tracker, if that
helps.

I believe that most patches are uncontroversial, although I expect
some discussion for patches 0009 and 0012, as I possibly didn't find
the best solution there.


Regards,
Volker

--
Volker Diels-Grabsch
----<<<((()))>>>----

--
sane-devel mailing list: [hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/sane-devel
Unsubscribe: Send mail with subject "unsubscribe your_password"
             to [hidden email]

0001-Fix-typos-in-comments.patch.gz (2K) Download Attachment
0002-Bugfix-On-error-return-the-actual-error-code-in-sane.patch.gz (782 bytes) Download Attachment
0003-Fix-interface-of-helper-function-write_many.patch.gz (1K) Download Attachment
0004-Fix-scope-of-negation.patch.gz (774 bytes) Download Attachment
0005-Introduce-md5_set_uint32.patch.gz (1K) Download Attachment
0006-Mark-internal-function-toupper_ascii-as-static.patch.gz (786 bytes) Download Attachment
0007-Ensure-that-sanei_thread_waitpid-always-has-a-define.patch.gz (728 bytes) Download Attachment
0008-Remove-dead-code-due-to-unused-variables.patch.gz (2K) Download Attachment
0009-Add-dummy-code-snippets-to-ensure-that-no-translatio.patch.gz (1K) Download Attachment
0010-Merge-all-compatibility-macros-around-__func__-and-_.patch.gz (2K) Download Attachment
0011-Use-consistently-__func__-instead-of-__FUNCTION__.patch.gz (41K) Download Attachment
0012-Change-GCC-mode-from-ISO-C90-to-ISO-C99.patch.gz (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Cleanup and small fixes

Volker Diels-Grabsch
Dear SANE developers,

Volker Diels-Grabsch schrieb:
> I'm not sure which is the best way to contribute, so I'm providing
> this as a series of (mostly) independent patches to this mailing list.
> I could also open one or more tickets in the issue tracker, if that
> helps.

Is there anyone who likes to review my work?

What can I do to simplify reviewing for you?  Should I post them
one-by-one to the issue tracker?


Regards,
Volker

--
Volker Diels-Grabsch
----<<<((()))>>>----

--
sane-devel mailing list: [hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/sane-devel
Unsubscribe: Send mail with subject "unsubscribe your_password"
             to [hidden email]
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Cleanup and small fixes

Olaf Meeuwissen-4
Hi Volker,

Volker Diels-Grabsch writes:

> Dear SANE developers,
>
> Volker Diels-Grabsch schrieb:
>> I'm not sure which is the best way to contribute, so I'm providing
>> this as a series of (mostly) independent patches to this mailing list.
>> I could also open one or more tickets in the issue tracker, if that
>> helps.
>
> Is there anyone who likes to review my work?

I'll see if I can get myself to review your patches this weekend.  If
anyone else beats me to it, that's okay.

# Having a brutal two weeks @the-office ... :-(

> What can I do to simplify reviewing for you?  Should I post them
> one-by-one to the issue tracker?

That would at least prevent them from dropping off the radar.

Hope this helps,
--
Olaf Meeuwissen, LPIC-2            FSF Associate Member since 2004-01-27
Support Free Software               Support the Free Software Foundation
https://my.fsf.org/donate                        https://my.fsf.org/join
 GnuPG key: F84A2DD9/B3C0 2F47 EA19 64F4 9F13  F43E B8A4 A88A F84A 2DD9


--
sane-devel mailing list: [hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/sane-devel
Unsubscribe: Send mail with subject "unsubscribe your_password"
             to [hidden email]
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Cleanup and small fixes

Olaf Meeuwissen-4
Hi again Volker,

Olaf Meeuwissen writes:

> Volker Diels-Grabsch writes:
>
>> Dear SANE developers,
>>
>> Volker Diels-Grabsch schrieb:
>>> I'm not sure which is the best way to contribute, so I'm providing
>>> this as a series of (mostly) independent patches to this mailing list.
>>> I could also open one or more tickets in the issue tracker, if that
>>> helps.
>>
>> Is there anyone who likes to review my work?
>
> I'll see if I can get myself to review your patches this weekend.  If
> anyone else beats me to it, that's okay.

* 0001-Fix-typos-in-comments.patch.gz

  There's about 100 other "annoying" `dont`'s you didn't fix ;-), but
  there is nothing wrong with your patch.

* 0002-Bugfix-On-error-return-the-actual-error-code-in-sane.patch.gz

  Nice catch.

* 0003-Fix-interface-of-helper-function-write_many.patch.gz

  I'm all in favour of const-correctness and less casts.

* 0004-Fix-scope-of-negation.patch.gz

  Why not simply write: if (dev->model->is_sheetfed == SANE_FALSE)?
  I've skipped your patch and pushed one that does the above.

* 0005-Introduce-md5_set_uint32.patch.gz

  Hmm, fixing code that originated from glibc an aeon ago.
  Maybe we should consider updating with more recent upstreams rather
  than trying to patch up things ourselves.

  Skipping this for now.

* 0006-Mark-internal-function-toupper_ascii-as-static.patch.gz

  Clarifies intended scope and help finding dead code.  Good.

* 0007-Ensure-that-sanei_thread_waitpid-always-has-a-define.patch.gz

  I'm not sure on what the expected return value should be in case of
  failure.  The documentation in include/sane/sanei_thread.h is of no
  help here.

  On top of that, the implementation caters to at least three different
  types of Sane_PID: int and two flavours of the implementation defined
  pthread_t type.

  Skipping this for now.

* 0008-Remove-dead-code-due-to-unused-variables.patch.gz

  OK.  The unused variables are not used in #ifdef'd code either so they
  can safely be zapped.

* 0009-Add-dummy-code-snippets-to-ensure-that-no-translatio.patch.gz

  If nothing of the file is used, it shouldn't be compiled in the first
  place.  I'll see if I can fix this as part of my autotool-reform
  branch over at GitLab[1].

  Skipping this for now.

* 0010-Merge-all-compatibility-macros-around-__func__-and-_.patch.gz
* 0011-Use-consistently-__func__-instead-of-__FUNCTION__.patch.gz

  Thanks for cleaning this up!

* 0012-Change-GCC-mode-from-ISO-C90-to-ISO-C99.patch.gz

  This is actually less controversial than you might think.  It has been
  discussed[2][3] after we released 1.0.25 and is on my very unofficial
  milestone[4] for 1.0.26.

  I don't think your patch is complete but it'll do for now.

>> What can I do to simplify reviewing for you?  Should I post them
>> one-by-one to the issue tracker?
>
> That would at least prevent them from dropping off the radar.

 [1] https://gitlab.com/sane-project/backends/tree/autotool-reform
 [2] https://lists.alioth.debian.org/pipermail/sane-devel/2015-October/034002.html
 [3] https://lists.alioth.debian.org/pipermail/sane-devel/2015-October/034019.html
 [4] https://gitlab.com/sane-project/backends/milestones/1

Hope this helps,
--
Olaf Meeuwissen, LPIC-2            FSF Associate Member since 2004-01-27
Support Free Software               Support the Free Software Foundation
https://my.fsf.org/donate                        https://my.fsf.org/join
 GnuPG key: F84A2DD9/B3C0 2F47 EA19 64F4 9F13  F43E B8A4 A88A F84A 2DD9


--
sane-devel mailing list: [hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/sane-devel
Unsubscribe: Send mail with subject "unsubscribe your_password"
             to [hidden email]
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Cleanup and small fixes

Volker Diels-Grabsch
Hi Olaf,

(Sorry for the previous email.  I overlooked that one.)

Thanks for reviewing my proposals!


Olaf Meeuwissen schrieb:
>
> * 0001-Fix-typos-in-comments.patch.gz
>
>   There's about 100 other "annoying" `dont`'s you didn't fix ;-), but
>   there is nothing wrong with your patch.

That's strange.  I thought I did a full-text search.  Anyway! :-)

> * 0004-Fix-scope-of-negation.patch.gz
>
>   Why not simply write: if (dev->model->is_sheetfed == SANE_FALSE)?
>   I've skipped your patch and pushed one that does the above.

I wasn't sure if there may be some subtle difference between
"== SANE_FALSE" and "!= SANE_TRUE".  Good to know that it's
really that simple. :-)

> * 0005-Introduce-md5_set_uint32.patch.gz
>
>   Hmm, fixing code that originated from glibc an aeon ago.
>   Maybe we should consider updating with more recent upstreams rather
>   than trying to patch up things ourselves.
>
>   Skipping this for now.

That was my first attempt, too.  Indeed, there are newer versions of
GNUlib!  However, early on, GNUlib's license changed into a strong
copyleft license (GPL or something).  That happened long before they
updated their MD5 implementation.

So if you use a newer GNUlib, sane-backends will effectively have a
stronger license.  I, personally, would have no problem with that.
But I thought it would be inappropriate for a contribution to impose
a different license on a project.

My patch was an attempt to fix exactly that one issue on my own,
isolated, leaving everything else untouched, so that no licensing
issues occur.

Another idea is to use a completely different MD5 implementation.


Regards,
Volker

--
Volker Diels-Grabsch
----<<<((()))>>>----

--
sane-devel mailing list: [hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/sane-devel
Unsubscribe: Send mail with subject "unsubscribe your_password"
             to [hidden email]
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Cleanup and small fixes

Olaf Meeuwissen-4
Hi Volker,

Volker Diels-Grabsch writes:

> Hi Olaf,
>
> (Sorry for the previous email.  I overlooked that one.)
>
> Thanks for reviewing my proposals!

You're welcome.

> Olaf Meeuwissen schrieb:
>>
>> * 0001-Fix-typos-in-comments.patch.gz
>>
>>   There's about 100 other "annoying" `dont`'s you didn't fix ;-), but
>>   there is nothing wrong with your patch.
>
> That's strange.  I thought I did a full-text search.  Anyway! :-)

Try: git grep -wi dont

> [...]
>
>> * 0005-Introduce-md5_set_uint32.patch.gz
>>
>>   Hmm, fixing code that originated from glibc an aeon ago.
>>   Maybe we should consider updating with more recent upstreams rather
>>   than trying to patch up things ourselves.
>>
>>   Skipping this for now.
>
> That was my first attempt, too.  Indeed, there are newer versions of
> GNUlib!  However, early on, GNUlib's license changed into a strong
> copyleft license (GPL or something).  That happened long before they
> updated their MD5 implementation.

I've discovered the same.  Both gnulib and gcc have GPL'd versions of
md5.c.  The one in glibc is still LGPL but doesn't fix the strict
aliasing issue that your patch fixes.

> So if you use a newer GNUlib, sane-backends will effectively have a
> stronger license.  I, personally, would have no problem with that.
> But I thought it would be inappropriate for a contribution to impose
> a different license on a project.

I was thinking along the same lines but also had a look at what is using
the MD5 code.  Turns out that it is only used by the scanimage and saned
frontends, both of which are plain GPL-v2 or later.  That is, no SANE
exception is applied.

The sanei/sanei_auth.c file also includes it but apart from saned, there
is nothing that includes the corresponding sanei_auth.h header.  None of
the backends link with sanei_auth.lo either.

Now there is one possible catch: the MD5 code is used in the bowels of
the auth_callback() function that is passed to the backend(s) by the
scanimage frontend.  However, the rest of the function is already GPL-v2.
Hence, whether the MD5 code is LGPL or GPL doesn't really matter, AFAIU,
for scanimage.

For saned, the situation is slightly different as that uses the
sanei_authorize() function from sanei/sanei_auth.c which is GPL-v2 w/
SANE exception.  In this case it might matter whether the MD5 code is
LGPL or GPL but I am not quite sure how the GPL status of saned itself
affects the whole thing.

# The GPL FAQ[1] makes my head swim :-(, once again
# [1] http://www.gnu.org/licenses/gpl-faq.html

> My patch was an attempt to fix exactly that one issue on my own,
> isolated, leaving everything else untouched, so that no licensing
> issues occur.

Your attention to the licensing details is to be commended and, given
the above, I think that applying your patch is the best solution.  I'll
push it shortly together with a minor tweak of my own to get rid of the
(char *) casts.

Hope this helps,
--
Olaf Meeuwissen, LPIC-2            FSF Associate Member since 2004-01-27
 GnuPG key: F84A2DD9/B3C0 2F47 EA19 64F4 9F13  F43E B8A4 A88A F84A 2DD9
 Support Free Software                        https://my.fsf.org/donate
 Support the Free Software Foundation           https://my.fsf.org/join



--
sane-devel mailing list: [hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/sane-devel
Unsubscribe: Send mail with subject "unsubscribe your_password"
             to [hidden email]
Loading...