[PATCH] Minor fixes in various places

[Jan Zelený]

I'm attaching some patches fixing various minor issues found by clang:

0001-Initialized-return-value-in-dp_copy_options.patch
Ticket: #577

0002-Fixed-potential-comparison-of-undefined-variable.patch
Ticket: #578

0003-Fixed-printing-of-undefined-value-in-sdap_async_acco.patch
Ticket: #579

0004-Fixed-uninialized-value-in-proxy_id-provider.patch
Ticket: #580

0005-Deleted-some-needless-assignments.patch
Ticket: #582

--
Jan

[Jakub Hrozek]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 08/31/2010 09:59 AM, Jan Zelený wrote:
> I'm attaching some patches fixing various minor issues found by clang:
>
> 0001-Initialized-return-value-in-dp_copy_options.patch
> Ticket: #577

ACK
> 0002-Fixed-potential-comparison-of-undefined-variable.patch
> Ticket: #578

ACK
> 0003-Fixed-printing-of-undefined-value-in-sdap_async_acco.patch
> Ticket: #579

Provided that it is safe to pass a NULL pointer to fprintf(), this is
OK. I know that glibc handles this, but I'm not entirely sure if that it
is good behaviour (if this would work with all possible optimalizations)
and if other libc implementations behave the same as glibc.
> 0004-Fixed-uninialized-value-in-proxy_id-provider.patch
> Ticket: #580

I guess NSS_STATUS_UNAVAIL is as good as any of the others in this case,
so ACK
> 0005-Deleted-some-needless-assignments.patch
> Ticket: #582

I think we should check for potential errors from sdap_id_op_done()
calls instead of removing the assignments altogether.
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkx8zjsACgkQHsardTLnvCUX9QCeKF0D7f5hplx7yT2gATh897+g
> B+cAnikdyJkYGrUwceHyvwUmr7dQYpn+
> =MrJk
> -----END PGP SIGNATURE-----

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 08/31/2010 05:41 AM, Jakub Hrozek wrote:
> On 08/31/2010 09:59 AM, Jan Zelený wrote:
> > I'm attaching some patches fixing various minor issues found by clang:
> > 0001-Initialized-return-value-in-dp_copy_options.patch
> > Ticket: #577
>
> ACK
> > 0002-Fixed-potential-comparison-of-undefined-variable.patch
> > Ticket: #578
>
> ACK
> > 0003-Fixed-printing-of-undefined-value-in-sdap_async_acco.patch
> > Ticket: #579
>
> Provided that it is safe to pass a NULL pointer to fprintf(), this is
> OK. I know that glibc handles this, but I'm not entirely sure if that it
> is good behaviour (if this would work with all possible optimalizations)
> and if other libc implementations behave the same as glibc.

This is a bug in e.g. the Intel compiler, if I remember correctly.
Please fix fprintf. I recommend using the construct:
name ? name : "";
in the fprintf call.
> > 0004-Fixed-uninialized-value-in-proxy_id-provider.patch
> > Ticket: #580
>
> I guess NSS_STATUS_UNAVAIL is as good as any of the others in this case,
> so ACK

I don't understand this comment, Jakub. Jan used NSS_STATUS_TRYAGAIN
here (which I think is certainly correct).
> > 0005-Deleted-some-needless-assignments.patch
> > Ticket: #582
>
> I think we should check for potential errors from sdap_id_op_done()
> calls instead of removing the assignments altogether.

I agree with Jakub. These assignments shouldn't be removed, they should
be tested for failures.

- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkx842YACgkQeiVVYja6o6N6zgCbBfJoA1NqW0Uhm7ik8gk/yNDZ
> iYgAn0X/YOojmrWYsPI73FTwyrJK2Xbs
> =kVke
> -----END PGP SIGNATURE-----

[Jakub Hrozek]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 08/31/2010 01:11 PM, Stephen Gallagher wrote:
> > > > > 0004-Fixed-uninialized-value-in-proxy_id-provider.patch
> > > > > Ticket: #580
> > >
> > > I guess NSS_STATUS_UNAVAIL is as good as any of the others in this case,
> > > so ACK
>
> I don't understand this comment, Jakub. Jan used NSS_STATUS_TRYAGAIN
> here (which I think is certainly correct).

Sorry, that was supposed to say NSS_STATUS_TRYAGAIN and yes, that one is
correct.
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkx85UYACgkQHsardTLnvCXYGwCg2+qgLA43DeuKjunj3G54Hrr8
> +gIAoLuWufS23Y4A9pjz+tyb9PnHTCol
> =2u3q
> -----END PGP SIGNATURE-----

[Jan Zelený]

Stephen Gallagher  wrote:
> > > 0003-Fixed-printing-of-undefined-value-in-sdap_async_acco.patch
> > > Ticket: #579
> >
> > Provided that it is safe to pass a NULL pointer to fprintf(), this is
> > OK. I know that glibc handles this, but I'm not entirely sure if that it
> > is good behaviour (if this would work with all possible optimalizations)
> > and if other libc implementations behave the same as glibc.
>
> This is a bug in e.g. the Intel compiler, if I remember correctly.
> Please fix fprintf. I recommend using the construct:
> name ? name : "";
> in the fprintf call.

Wouldn't it be better just to initialize the variable with "" then?

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 08/31/2010 07:39 AM, Jan Zelený wrote:
> Stephen Gallagher  wrote:
> > > > 0003-Fixed-printing-of-undefined-value-in-sdap_async_acco.patch
> > > > Ticket: #579
> > >
> > > Provided that it is safe to pass a NULL pointer to fprintf(), this is
> > > OK. I know that glibc handles this, but I'm not entirely sure if that it
> > > is good behaviour (if this would work with all possible optimalizations)
> > > and if other libc implementations behave the same as glibc.
> >
> > This is a bug in e.g. the Intel compiler, if I remember correctly.
> > Please fix fprintf. I recommend using the construct:
> > name ? name : "";
> > in the fprintf call.
>
> Wouldn't it be better just to initialize the variable with "" then?

Jakub mentioned off-list that we should probably fix this in the DEBUG()
macro, rather than trying to guard against it in each possible use-case.
I think I agree with him.

- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkx86zMACgkQeiVVYja6o6MQDwCfedx54FFhNnYte4iiAw5QVvNu
> 7DEAn2gPgpW0emk73+8A0YY1oDS1Nyee
> =VO8O
> -----END PGP SIGNATURE-----

[Jan Zelený]

Stephen Gallagher  wrote:
> On 08/31/2010 07:39 AM, Jan Zelený wrote:
> > Stephen Gallagher  wrote:
> > > > > 0003-Fixed-printing-of-undefined-value-in-sdap_async_acco.patch
> > > > > Ticket: #579
> > > >
> > > > Provided that it is safe to pass a NULL pointer to fprintf(), this is
> > > > OK. I know that glibc handles this, but I'm not entirely sure if that
> > > > it is good behaviour (if this would work with all possible
> > > > optimalizations) and if other libc implementations behave the same as
> > > > glibc.
> > >
> > > This is a bug in e.g. the Intel compiler, if I remember correctly.
> > > Please fix fprintf. I recommend using the construct:
> > > name ? name : "";
> > > in the fprintf call.
> >
> > Wouldn't it be better just to initialize the variable with "" then?
>
> Jakub mentioned off-list that we should probably fix this in the DEBUG()
> macro, rather than trying to guard against it in each possible use-case.
> I think I agree with him.

Yes, that's probably a good idea. Is the patch ACKed in that case?

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 08/31/2010 07:56 AM, Jan Zelený wrote:
> Stephen Gallagher  wrote:
> > On 08/31/2010 07:39 AM, Jan Zelený wrote:
> > > Stephen Gallagher  wrote:
> > > > > > 0003-Fixed-printing-of-undefined-value-in-sdap_async_acco.patch
> > > > > > Ticket: #579
> > > > >
> > > > > Provided that it is safe to pass a NULL pointer to fprintf(), this is
> > > > > OK. I know that glibc handles this, but I'm not entirely sure if that
> > > > > it is good behaviour (if this would work with all possible
> > > > > optimalizations) and if other libc implementations behave the same as
> > > > > glibc.
> > > >
> > > > This is a bug in e.g. the Intel compiler, if I remember correctly.
> > > > Please fix fprintf. I recommend using the construct:
> > > > name ? name : "";
> > > > in the fprintf call.
> > >
> > > Wouldn't it be better just to initialize the variable with "" then?
> >
> > Jakub mentioned off-list that we should probably fix this in the DEBUG()
> > macro, rather than trying to guard against it in each possible use-case.
> > I think I agree with him.
>
> Yes, that's probably a good idea. Is the patch ACKed in that case?

Yes. Ack.

- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkx/zlcACgkQeiVVYja6o6N7MgCfV8IcE/OIT0GLEGGNVEAtg/JC
> H/MAn2Qr7WlnSvJd1/dDGCrFFXMewR8X
> =xqe+
> -----END PGP SIGNATURE-----

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/02/2010 12:18 PM, Stephen Gallagher wrote:
> On 08/31/2010 07:56 AM, Jan Zelený wrote:
> > Stephen Gallagher  wrote:
> > > On 08/31/2010 07:39 AM, Jan Zelený wrote:
> > > > Stephen Gallagher  wrote:
> > > > > > > 0003-Fixed-printing-of-undefined-value-in-sdap_async_acco.patch
> > > > > > > Ticket: #579
> > > > > >
> > > > > > Provided that it is safe to pass a NULL pointer to fprintf(), this is
> > > > > > OK. I know that glibc handles this, but I'm not entirely sure if that
> > > > > > it is good behaviour (if this would work with all possible
> > > > > > optimalizations) and if other libc implementations behave the same as
> > > > > > glibc.
> > > > >
> > > > > This is a bug in e.g. the Intel compiler, if I remember correctly.
> > > > > Please fix fprintf. I recommend using the construct:
> > > > > name ? name : "";
> > > > > in the fprintf call.
> > > >
> > > > Wouldn't it be better just to initialize the variable with "" then?
> > >
> > > Jakub mentioned off-list that we should probably fix this in the DEBUG()
> > > macro, rather than trying to guard against it in each possible use-case.
> > > I think I agree with him.
> > Yes, that's probably a good idea. Is the patch ACKed in that case?
>
> Yes. Ack.

Patches 0001-0004 pushed to master.

- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkx/0FIACgkQeiVVYja6o6PlAQCfRRYPw/YkjOskKjgsqYqoqauf
> SfMAnjXTjYoO7uHqg2tB8sI6OtqB0NEP
> =4JPh
> -----END PGP SIGNATURE-----

[Jan Zelený]

Stephen Gallagher  wrote:
> > > 0005-Deleted-some-needless-assignments.patch
> > > Ticket: #582
> >
> > I think we should check for potential errors from sdap_id_op_done()
> > calls instead of removing the assignments altogether.
>
> I agree with Jakub. These assignments shouldn't be removed, they should
> be tested for failures.

Sorry for an extra email, forgot to mention this in the previous one. I don't 
think this is the case. The result of operation is subsequently checked (it is 
stored also in dp_error variable). We can check ret value as well as dp_error, 
but I don't think it will bring any code improvement. The same applies for 
call of sysdb_attrs_get_string() - its return value isn't checked, but the 
content of "value" variable is tested.

Jan

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 08/31/2010 07:57 AM, Jan Zelený wrote:
> Stephen Gallagher  wrote:
> > > > 0005-Deleted-some-needless-assignments.patch
> > > > Ticket: #582
> > >
> > > I think we should check for potential errors from sdap_id_op_done()
> > > calls instead of removing the assignments altogether.
> >
> > I agree with Jakub. These assignments shouldn't be removed, they should
> > be tested for failures.
>
> Sorry for an extra email, forgot to mention this in the previous one. I don't 
> think this is the case. The result of operation is subsequently checked (it is 
> stored also in dp_error variable). We can check ret value as well as dp_error, 
> but I don't think it will bring any code improvement. The same applies for 
> call of sysdb_attrs_get_string() - its return value isn't checked, but the 
> content of "value" variable is tested.

For the record, clang and Coverity both throw warnings if we do not
receive and check the return value of any function that has a return
value. In order to be in compliance with these tools, we need to handle
this.

Also, our unofficial policy is that the return value needs to be
accurate (since the returned-by-reference value may not be valid or safe
to dereference if the return value is a failure).

- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkx87sAACgkQeiVVYja6o6PdUQCfdSqnmT0UcAN3zkZvS6mMi5T+
> kH4Anj/55WXHyLH38jWajAZkXQZlNJ+T
> =w2le
> -----END PGP SIGNATURE-----

[Jan Zelený]

Stephen Gallagher  wrote:
> On 08/31/2010 07:57 AM, Jan Zelený wrote:
> > Stephen Gallagher  wrote:
> > > > > 0005-Deleted-some-needless-assignments.patch
> > > > > Ticket: #582
> > > >
> > > > I think we should check for potential errors from sdap_id_op_done()
> > > > calls instead of removing the assignments altogether.
> > >
> > > I agree with Jakub. These assignments shouldn't be removed, they should
> > > be tested for failures.
> >
> > Sorry for an extra email, forgot to mention this in the previous one. I
> > don't think this is the case. The result of operation is subsequently
> > checked (it is stored also in dp_error variable). We can check ret value
> > as well as dp_error, but I don't think it will bring any code
> > improvement. The same applies for call of sysdb_attrs_get_string() - its
> > return value isn't checked, but the content of "value" variable is
> > tested.
>
> For the record, clang and Coverity both throw warnings if we do not
> receive and check the return value of any function that has a return
> value. In order to be in compliance with these tools, we need to handle
> this.
>
> Also, our unofficial policy is that the return value needs to be
> accurate (since the returned-by-reference value may not be valid or safe
> to dereference if the return value is a failure).

Both your points are valid. I'm attaching a patch where the check is 
performed. I still think it is redundant, but it might be more safe in the 
future in case the code around it changes.

--
Jan

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/01/2010 05:56 AM, Jan Zelený wrote:
> Both your points are valid. I'm attaching a patch where the check is 
> performed. I still think it is redundant, but it might be more safe in the 
> future in case the code around it changes.

Nack.

This line:
dp_error = DP_ERR_FATAL;
is completely unnecessary. dp_error is never read again after this
assignment.

I realize that it was there before this patch, but it's unnecessary and
wrong. Please remove it.

The rest of the patch looks good.

- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkx/rvkACgkQeiVVYja6o6MipQCfdl27dqBq1xN42AWoJmaTs0bJ
> QWgAn2yN/ZF7LkOgvLzy9myYunC2sK9x
> =QtVl
> -----END PGP SIGNATURE-----

[Jan Zelený]

Stephen Gallagher  wrote:
> On 09/01/2010 05:56 AM, Jan Zelený wrote:
> > Both your points are valid. I'm attaching a patch where the check is
> > performed. I still think it is redundant, but it might be more safe in
> > the future in case the code around it changes.
>
> Nack.
>
> This line:
> dp_error = DP_ERR_FATAL;
> is completely unnecessary. dp_error is never read again after this
> assignment.
>
> I realize that it was there before this patch, but it's unnecessary and
> wrong. Please remove it.
>
> The rest of the patch looks good.

Thanks, I don't know how could I miss it. Now it is removed.

--
Jan

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/03/2010 07:39 AM, Jan Zelený wrote:
> Stephen Gallagher  wrote:
> > On 09/01/2010 05:56 AM, Jan Zelený wrote:
> > > Both your points are valid. I'm attaching a patch where the check is
> > > performed. I still think it is redundant, but it might be more safe in
> > > the future in case the code around it changes.
> >
> > Nack.
> >
> > This line:
> > dp_error = DP_ERR_FATAL;
> > is completely unnecessary. dp_error is never read again after this
> > assignment.
> >
> > I realize that it was there before this patch, but it's unnecessary and
> > wrong. Please remove it.
> >
> > The rest of the patch looks good.
>
> Thanks, I don't know how could I miss it. Now it is removed.

Ack.

- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkyA3xEACgkQeiVVYja6o6MkRQCfV8MpEjpnqHk28K1zn5TE4V9A
> 40QAnRME3+umtFnYIL39eKIqdDBfEyxw
> =HI0Z
> -----END PGP SIGNATURE-----

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/03/2010 07:42 AM, Stephen Gallagher wrote:
> On 09/03/2010 07:39 AM, Jan Zelený wrote:
> > Stephen Gallagher  wrote:
> > > On 09/01/2010 05:56 AM, Jan Zelený wrote:
> > > > Both your points are valid. I'm attaching a patch where the check is
> > > > performed. I still think it is redundant, but it might be more safe in
> > > > the future in case the code around it changes.
> > >
> > > Nack.
> > >
> > > This line:
> > > dp_error = DP_ERR_FATAL;
> > > is completely unnecessary. dp_error is never read again after this
> > > assignment.
> > >
> > > I realize that it was there before this patch, but it's unnecessary and
> > > wrong. Please remove it.
> > >
> > > The rest of the patch looks good.
> > Thanks, I don't know how could I miss it. Now it is removed.
>
> Ack.

Pushed to master.

- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEUEARECAAYFAkyGS9kACgkQeiVVYja6o6OtagCXZ/LqfgWEWlSwbv2YTCmqSoDN
> CQCggMrhBUXGjvJKL0Wpze+ETofGAeg=
> =NA5R
> -----END PGP SIGNATURE-----