Fixed ALL compiler warnings

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

Fixed ALL compiler warnings

Frank Morgner
I fixed all compiler warnings from clang/gcc. If someone wants to use
OpenSC in a productive environment, the least thing you want to do is to
check the compiler warnings! (If you are Apple you might even want to
turn on more warnings that usual... otherwise you `goto fail`).

In the process I found and fixed some critical bugs. Especially
`pkcs15init_initialize` from src/pkcs11/framework-pkcs15init.c needs a
rewrite as it implements a wrong prototype (see
c18ec388bc29b9a084d5b1ebbaff69098f5a6ddb).

Also, we should start to agree upon some coding rules. The first, I want
to add is to treat compiler warnings as errors!

For more information/reviews see pull request here:
https://github.com/OpenSC/OpenSC/pull/222

--
Frank Morgner

Virtual Smart Card Architecture http://vsmartcard.sourceforge.net
OpenPACE                        http://openpace.sourceforge.net
IFD Handler for libnfc Devices  http://sourceforge.net/projects/ifdnfc

------------------------------------------------------------------------------
Flow-based real-time traffic analytics software. Cisco certified tool.
Monitor traffic, SLAs, QoS, Medianet, WAAS etc. with NetFlow Analyzer
Customize your own dashboards, set traffic alerts and generate reports.
Network behavioral analysis & security monitoring. All-in-one tool.
http://pubads.g.doubleclick.net/gampad/clk?id=126839071&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel

attachment0 (985 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Fixed ALL compiler warnings

Ludovic Rousseau
Hello,

2014-03-02 11:42 GMT+01:00 Frank Morgner <[hidden email]>:
> I fixed all compiler warnings from clang/gcc. If someone wants to use
> OpenSC in a productive environment, the least thing you want to do is to
> check the compiler warnings! (If you are Apple you might even want to
> turn on more warnings that usual... otherwise you `goto fail`).

Great.
The hard part is to define a correct level of warnings.

> Also, we should start to agree upon some coding rules. The first, I want
> to add is to treat compiler warnings as errors!
>
> For more information/reviews see pull request here:
> https://github.com/OpenSC/OpenSC/pull/222

Why this change at line 46 of src/libopensc/card-dnie.c ?

-#ifdef ENABLE_SM
+#if ENABLE_SM && 0

If I am correct (and I checked using a sample code) the condition will
never be true.
What is the warning you solved with this change?


At line 1957 of the same file you move the call to
LOG_FUNC_CALLED(card->ctx); _inside_ the #ifdef ENABLE_SM. So the call
will not be made if ENABLE_SM is not defined.
Why this change?


In src/pkcs15init/pkcs15-cardos.c line 175 your change is:
- if (current > preferred || preferred > CARDOS_PIN_ID_MAX)
+ if (preferred > CARDOS_PIN_ID_MAX)

Why removing the "current > preferred" part of the check?


As a general rule I do like to have the warning that is fixed by the
patch in the patch commit message. Yes, it is more work but it is then
easier to know why a patch was made a few years after.


Also it is a good idea to have not mix different fixes in the same
patch. Warnings in a patch and a semantic change in another patch with
a clear explanation of _why_ the change is made.

Thanks for your work.

--
 Dr. Ludovic Rousseau

------------------------------------------------------------------------------
Flow-based real-time traffic analytics software. Cisco certified tool.
Monitor traffic, SLAs, QoS, Medianet, WAAS etc. with NetFlow Analyzer
Customize your own dashboards, set traffic alerts and generate reports.
Network behavioral analysis & security monitoring. All-in-one tool.
http://pubads.g.doubleclick.net/gampad/clk?id=126839071&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: Fixed ALL compiler warnings

Frank Morgner
On Sunday, March 02 at 05:11PM, Ludovic Rousseau wrote:

> Hello,
>
> 2014-03-02 11:42 GMT+01:00 Frank Morgner <[hidden email]>:
> > I fixed all compiler warnings from clang/gcc. If someone wants to use
> > OpenSC in a productive environment, the least thing you want to do is to
> > check the compiler warnings! (If you are Apple you might even want to
> > turn on more warnings that usual... otherwise you `goto fail`).
>
> Great.
> The hard part is to define a correct level of warnings.
As said before, in a productive environment OpenSC is dealing with
critical data. So you should consider all warnings possible.

> > Also, we should start to agree upon some coding rules. The first, I want
> > to add is to treat compiler warnings as errors!
> >
> > For more information/reviews see pull request here:
> > https://github.com/OpenSC/OpenSC/pull/222
>
> Why this change at line 46 of src/libopensc/card-dnie.c ?
>
> -#ifdef ENABLE_SM
> +#if ENABLE_SM && 0
>
> If I am correct (and I checked using a sample code) the condition will
> never be true.
> What is the warning you solved with this change?
dnie's functions for wrapping SM were defined but never actually (which
is exactly what the warning looked like). See comment in
https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/card-dnie.c#L554

> At line 1957 of the same file you move the call to
> LOG_FUNC_CALLED(card->ctx); _inside_ the #ifdef ENABLE_SM. So the call
> will not be made if ENABLE_SM is not defined.
> Why this change?
>
>
> In src/pkcs15init/pkcs15-cardos.c line 175 your change is:
> - if (current > preferred || preferred > CARDOS_PIN_ID_MAX)
> + if (preferred > CARDOS_PIN_ID_MAX)
>
> Why removing the "current > preferred" part of the check?
If I am not mistaken, there is no case possible where `preferred` is
greater than `current`, so I deleted the check. It is also done in some
other driver. The warning was:

assuming signed overflow does not occur when assuming that (X + c) < X is always false

> As a general rule I do like to have the warning that is fixed by the
> patch in the patch commit message. Yes, it is more work but it is then
> easier to know why a patch was made a few years after.

OK, I'll remember this for future changes. In the given case I would
advocate for carefully reviewing the changes.

> Also it is a good idea to have not mix different fixes in the same
> patch. Warnings in a patch and a semantic change in another patch with
> a clear explanation of _why_ the change is made.

I tried to split all non invasive changes (that work around the
warnings) from stuff that fixes bugs or changes the functionality. These
are split into separate patches.

--
Frank Morgner

Virtual Smart Card Architecture http://vsmartcard.sourceforge.net
OpenPACE                        http://openpace.sourceforge.net
IFD Handler for libnfc Devices  http://sourceforge.net/projects/ifdnfc

------------------------------------------------------------------------------
Flow-based real-time traffic analytics software. Cisco certified tool.
Monitor traffic, SLAs, QoS, Medianet, WAAS etc. with NetFlow Analyzer
Customize your own dashboards, set traffic alerts and generate reports.
Network behavioral analysis & security monitoring. All-in-one tool.
http://pubads.g.doubleclick.net/gampad/clk?id=126839071&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel

attachment0 (985 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Fixed ALL compiler warnings

Frank Morgner
Hi!

> > In src/pkcs15init/pkcs15-cardos.c line 175 your change is:
> > - if (current > preferred || preferred > CARDOS_PIN_ID_MAX)
> > + if (preferred > CARDOS_PIN_ID_MAX)
> >
> > Why removing the "current > preferred" part of the check?
>
> If I am not mistaken, there is no case possible where `preferred` is
> greater than `current`, so I deleted the check. It is also done in some
> other driver. The warning was:
>
> assuming signed overflow does not occur when assuming that (X + c) < X is always false
I think there is one corner case that i didn't capture. I hope, I have
restored the semantics with this commit:
https://github.com/frankmorgner/OpenSC/commit/73971f229504306b6d0979807f0ed5ec50a6fe42

--
Frank Morgner

Virtual Smart Card Architecture http://vsmartcard.sourceforge.net
OpenPACE                        http://openpace.sourceforge.net
IFD Handler for libnfc Devices  http://sourceforge.net/projects/ifdnfc

------------------------------------------------------------------------------
Subversion Kills Productivity. Get off Subversion & Make the Move to Perforce.
With Perforce, you get hassle-free workflows. Merge that actually works.
Faster operations. Version large binaries.  Built-in WAN optimization and the
freedom to use Git, Perforce or both. Make the move to Perforce.
http://pubads.g.doubleclick.net/gampad/clk?id=122218951&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel

attachment0 (985 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Fixed ALL compiler warnings

Ludovic Rousseau
In reply to this post by Frank Morgner
2014-03-02 22:13 GMT+01:00 Frank Morgner <[hidden email]>:

> On Sunday, March 02 at 05:11PM, Ludovic Rousseau wrote:
>> Hello,
>>
>> 2014-03-02 11:42 GMT+01:00 Frank Morgner <[hidden email]>:
>> > I fixed all compiler warnings from clang/gcc. If someone wants to use
>> > OpenSC in a productive environment, the least thing you want to do is to
>> > check the compiler warnings! (If you are Apple you might even want to
>> > turn on more warnings that usual... otherwise you `goto fail`).
>>
>> Great.
>> The hard part is to define a correct level of warnings.
>
> As said before, in a productive environment OpenSC is dealing with
> critical data. So you should consider all warnings possible.

OpenSC generates a lot of "unused parameter" warnings. These warnings
are harmless and do not need to be fixed.

You can rebuilt OpenSC using "-Wall -Wextra -funsigned-char
-fstrict-aliasing -Wchar-subscripts -Wundef -Wshadow -Wcast-align
-Wwrite-strings -Wunused -Wuninitialized -Wpointer-arith
-Wredundant-decls -Winline -Wformat -Wformat-security -Wswitch-enum
-Winit-self -Wmissing-include-dirs -Wmissing-prototypes
-Wstrict-prototypes -Wold-style-definition -Wbad-function-cast
-Wnested-externs -Wmissing-declarations" and see all the remaining
warnings.

But maybe I am using a too high level of warnings.

>> > Also, we should start to agree upon some coding rules. The first, I want
>> > to add is to treat compiler warnings as errors!
>> >
>> > For more information/reviews see pull request here:
>> > https://github.com/OpenSC/OpenSC/pull/222
>>
>> Why this change at line 46 of src/libopensc/card-dnie.c ?
>>
>> -#ifdef ENABLE_SM
>> +#if ENABLE_SM && 0
>>
>> If I am correct (and I checked using a sample code) the condition will
>> never be true.
>> What is the warning you solved with this change?
>
> dnie's functions for wrapping SM were defined but never actually (which
> is exactly what the warning looked like). See comment in
> https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/card-dnie.c#L554

So this patch does not fix a warning but uses a complex way to
"remove" dead code.
Please do not do that. If you want to remove code then remove it in a
specific commit and document why you removed the code.

>> At line 1957 of the same file you move the call to
>> LOG_FUNC_CALLED(card->ctx); _inside_ the #ifdef ENABLE_SM. So the call
>> will not be made if ENABLE_SM is not defined.
>> Why this change?

No comment?

>> In src/pkcs15init/pkcs15-cardos.c line 175 your change is:
>> - if (current > preferred || preferred > CARDOS_PIN_ID_MAX)
>> + if (preferred > CARDOS_PIN_ID_MAX)
>>
>> Why removing the "current > preferred" part of the check?
>
> If I am not mistaken, there is no case possible where `preferred` is
> greater than `current`, so I deleted the check. It is also done in some
> other driver. The warning was:
>
> assuming signed overflow does not occur when assuming that (X + c) < X is always false

Then document the compiler warning and explain why you think you are
correct to remove the code in the commit message.

>> As a general rule I do like to have the warning that is fixed by the
>> patch in the patch commit message. Yes, it is more work but it is then
>> easier to know why a patch was made a few years after.
>
> OK, I'll remember this for future changes. In the given case I would
> advocate for carefully reviewing the changes.

I would like you to redo your patches in a cleaner way before I accept
them in OpenSC.

>> Also it is a good idea to have not mix different fixes in the same
>> patch. Warnings in a patch and a semantic change in another patch with
>> a clear explanation of _why_ the change is made.
>
> I tried to split all non invasive changes (that work around the
> warnings) from stuff that fixes bugs or changes the functionality. These
> are split into separate patches.

A good way to not mix things is to create a git commit for each
different compiler warning.

Bye

--
 Dr. Ludovic Rousseau

------------------------------------------------------------------------------
Subversion Kills Productivity. Get off Subversion & Make the Move to Perforce.
With Perforce, you get hassle-free workflows. Merge that actually works.
Faster operations. Version large binaries.  Built-in WAN optimization and the
freedom to use Git, Perforce or both. Make the move to Perforce.
http://pubads.g.doubleclick.net/gampad/clk?id=122218951&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: Fixed ALL compiler warnings

Frank Morgner
On Monday, March 03 at 05:47PM, Ludovic Rousseau wrote:

> 2014-03-02 22:13 GMT+01:00 Frank Morgner <[hidden email]>:
> > On Sunday, March 02 at 05:11PM, Ludovic Rousseau wrote:
> >> Hello,
> >>
> >> 2014-03-02 11:42 GMT+01:00 Frank Morgner <[hidden email]>:
> >> > I fixed all compiler warnings from clang/gcc. If someone wants to use
> >> > OpenSC in a productive environment, the least thing you want to do is to
> >> > check the compiler warnings! (If you are Apple you might even want to
> >> > turn on more warnings that usual... otherwise you `goto fail`).
> >>
> >> Great.
> >> The hard part is to define a correct level of warnings.
> >
> > As said before, in a productive environment OpenSC is dealing with
> > critical data. So you should consider all warnings possible.
>
> OpenSC generates a lot of "unused parameter" warnings. These warnings
> are harmless and do not need to be fixed.
>
> You can rebuilt OpenSC using "-Wall -Wextra -funsigned-char
> -fstrict-aliasing -Wchar-subscripts -Wundef -Wshadow -Wcast-align
> -Wwrite-strings -Wunused -Wuninitialized -Wpointer-arith
> -Wredundant-decls -Winline -Wformat -Wformat-security -Wswitch-enum
> -Winit-self -Wmissing-include-dirs -Wmissing-prototypes
> -Wstrict-prototypes -Wold-style-definition -Wbad-function-cast
> -Wnested-externs -Wmissing-declarations" and see all the remaining
> warnings.
>
> But maybe I am using a too high level of warnings.
I'd love to see you fix all those warnings in OpenSC ;-)
I considered only the warnings generated from the default build
parameters.

> >> > Also, we should start to agree upon some coding rules. The first, I want
> >> > to add is to treat compiler warnings as errors!
> >> >
> >> > For more information/reviews see pull request here:
> >> > https://github.com/OpenSC/OpenSC/pull/222
> >>
> >> Why this change at line 46 of src/libopensc/card-dnie.c ?
> >>
> >> -#ifdef ENABLE_SM
> >> +#if ENABLE_SM && 0
> >>
> >> If I am correct (and I checked using a sample code) the condition will
> >> never be true.
> >> What is the warning you solved with this change?
> >
> > dnie's functions for wrapping SM were defined but never actually (which
> > is exactly what the warning looked like). See comment in
> > https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/card-dnie.c#L554
>
> So this patch does not fix a warning but uses a complex way to
> "remove" dead code.
> Please do not do that. If you want to remove code then remove it in a
> specific commit and document why you removed the code.
I would not consider adding '&& 0' being very complex. It seemed like
the code should be used at some point so I wanted to leave deleting it
to the dnie maintainer or to some maintainer with a broader view than
mine. And while the maintainer is at it, he or she could have a look at
all those other '#if 0's...

> >> At line 1957 of the same file you move the call to
> >> LOG_FUNC_CALLED(card->ctx); _inside_ the #ifdef ENABLE_SM. So the call
> >> will not be made if ENABLE_SM is not defined.
> >> Why this change?
>
> No comment?

Without SM the compiler warns about res not being used so I put it into
the ifdef. But then you can't call LOG_FUNC_CALLED(card->ctx);
beforehand since the definition of res has to appear first.

Is that a problem? I don't think so. Without SM the function really
doesn't have any meaningful purpose. It simply returns
SC_ERROR_NOT_SUPPORTED. I don't think it is necessary to document
entering the function in that case.

When merging the pull request you may also choose to initialize res and
USE it in the non-SM case. It's up to you, it doesn't make much of a
difference.

> >> In src/pkcs15init/pkcs15-cardos.c line 175 your change is:
> >> - if (current > preferred || preferred > CARDOS_PIN_ID_MAX)
> >> + if (preferred > CARDOS_PIN_ID_MAX)
> >>
> >> Why removing the "current > preferred" part of the check?
> >
> > If I am not mistaken, there is no case possible where `preferred` is
> > greater than `current`, so I deleted the check. It is also done in some
> > other driver. The warning was:
> >
> > assuming signed overflow does not occur when assuming that (X + c) < X is always false
>
> Then document the compiler warning and explain why you think you are
> correct to remove the code in the commit message.
I would like to avoid writing a two page essay about deleting two words.
I think it is easier to read the code and agree upon whether
https://github.com/OpenSC/OpenSC/pull/222/files#diff-af263605882161ee11049ca347b4bd7bR164
https://github.com/OpenSC/OpenSC/pull/222/files#diff-7682658a432d2f725fe98d701e4f83c9R183
are functionality preserving or not.

> >> As a general rule I do like to have the warning that is fixed by the
> >> patch in the patch commit message. Yes, it is more work but it is then
> >> easier to know why a patch was made a few years after.
> >
> > OK, I'll remember this for future changes. In the given case I would
> > advocate for carefully reviewing the changes.
>
> I would like you to redo your patches in a cleaner way before I accept
> them in OpenSC.

OK.

> >> Also it is a good idea to have not mix different fixes in the same
> >> patch. Warnings in a patch and a semantic change in another patch with
> >> a clear explanation of _why_ the change is made.
> >
> > I tried to split all non invasive changes (that work around the
> > warnings) from stuff that fixes bugs or changes the functionality. These
> > are split into separate patches.
>
> A good way to not mix things is to create a git commit for each
> different compiler warning.
>
> Bye
>
> --
>  Dr. Ludovic Rousseau
>
> ------------------------------------------------------------------------------
> Subversion Kills Productivity. Get off Subversion & Make the Move to Perforce.
> With Perforce, you get hassle-free workflows. Merge that actually works.
> Faster operations. Version large binaries.  Built-in WAN optimization and the
> freedom to use Git, Perforce or both. Make the move to Perforce.
> http://pubads.g.doubleclick.net/gampad/clk?id=122218951&iu=/4140/ostg.clktrk
> _______________________________________________
> Opensc-devel mailing list
> [hidden email]
> https://lists.sourceforge.net/lists/listinfo/opensc-devel
>
--
Frank Morgner

Virtual Smart Card Architecture http://vsmartcard.sourceforge.net
OpenPACE                        http://openpace.sourceforge.net
IFD Handler for libnfc Devices  http://sourceforge.net/projects/ifdnfc

------------------------------------------------------------------------------
Subversion Kills Productivity. Get off Subversion & Make the Move to Perforce.
With Perforce, you get hassle-free workflows. Merge that actually works.
Faster operations. Version large binaries.  Built-in WAN optimization and the
freedom to use Git, Perforce or both. Make the move to Perforce.
http://pubads.g.doubleclick.net/gampad/clk?id=122218951&iu=/4140/ostg.clktrk
_______________________________________________
Opensc-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/opensc-devel

attachment0 (985 bytes) Download Attachment