On (03/10/14 09:40), Simo Sorce wrote:
----- Original Message -----
> From: "Lukas Slebodnik" <lslebodn(a)redhat.com>
> To: "Simo Sorce" <simo(a)redhat.com>
> Cc: sssd-devel(a)lists.fedorahosted.org
> Sent: Thursday, October 2, 2014 4:20:56 PM
> Subject: Re: [SSSD] [PATCH] Suppress warning maybe-uninitialized
>
> On (02/10/14 15:12), Simo Sorce wrote:
> >On Thu, 2 Oct 2014 19:33:02 +0200
> >Lukas Slebodnik <lslebodn(a)redhat.com> wrote:
> >
> >> ehlo,
> >>
> >> There is a warning with some version of gcc (4.9.1, 4.4.7).
> >> Warning is not visible with gcc 4.8
> >> In my opinion, variable "a" should be initialized every time if
> >> "maps" is not NULL.
> >
> >I agree that "a" should be intialized every time, in fact I do not
> >agree with your patch where you set it to be a volatile, it is not.
> >
> >The main issue here is that the name and way this variable is used is
> >confusing.
> >
> >A better way would be to define a locally in the first loop, and in the
> >function define a better name initialized to a negative value.
> >
> >the name should probably be called something like base_attr_idx and
> >intialized as:
> >
> >int base_attr_idx = 0;
> >
> >and if the first part of the code finds the base_attr it is looking for
> >it would simply assing a to it (note we do away with "a" completely).
> >
> >if (map) {
> > for (i = 1; i < attrs_num; i++) {
> > ...
> > }
> > if (i < attrs_num) {
> > store = true;
> > base_attr_idx = i;
> > ...
> > }
> >}
> >
> >then the second for loop where you get the error would be change to:
> >for (ai = base_attr_idx; ai > 0 && ai < attrs_num; ai++) {
> >
> I suppose you meant similar patch
> diff --git a/src/providers/ldap/sdap.c b/src/providers/ldap/sdap.c
> index 7034b11..6e8b16e 100644
> --- a/src/providers/ldap/sdap.c
> +++ b/src/providers/ldap/sdap.c
> @@ -305,6 +305,7 @@ int sdap_parse_entry(TALLOC_CTX *memctx,
> char *str;
> int lerrno;
> int a, i, ret, ai;
> + int base_attr_idx;
> const char *name;
> bool store;
> bool base64;
> @@ -412,6 +413,7 @@ int sdap_parse_entry(TALLOC_CTX *memctx,
> /* interesting attr */
> if (a < attrs_num) {
> store = true;
> + base_attr_idx = a;
> name = map[a].sys_name;
> if (strcmp(name, SYSDB_SSH_PUBKEY) == 0) {
> base64 = true;
> @@ -478,7 +480,7 @@ int sdap_parse_entry(TALLOC_CTX *memctx,
> * attrs in case there is a map. Find all that match
> * and copy the value
> */
> - for (ai = a; ai < attrs_num; ai++) {
> + for (ai = base_attr_idx; ai < attrs_num; ai++) {
> /* check if this attr is valid with the chosen
> * schema */
> if (!map[ai].name) continue;
>
>
> It didn't help.
> src/providers/ldap/sdap.c: In function 'sdap_parse_entry':
> src/providers/ldap/sdap.c:308:9: error: 'base_attr_idx' may be used
> uninitialized in this function [-Werror=maybe-uninitialized]
> int base_attr_idx;
> ^
> cc1: all warnings being treated as errors
> (BTW: There is no difference between using a and base_attr_idx)
>
> Any other better solution is welcomed.
You did not follow my advice.
There were 2 fundamental changes in my proposal
1. declare and initialize the new variable to 0
int base_attr_idx = 0;
It would be the same solution as initialising variable "a" to 0,
it fixes warning as well. I just want to avoid such solution,
because it can hide real problem where variable can be used uninitialized.
2. in the second for loop add ai > 0 to the condition, otherwise
you'll
run through even if base_attr_idx was never intentionally set.
With these 2 changes you won't get the warning and the behaviour will be correct.
Do we really need the additional condition?
In worst case, we will search base_attr in map from beginning. I would like to
have as simple change as possible. (don't want to break anything).
Additionally, if you want, you can completely drop the 'a' variable and use
'i'
everywhere instead.
yes, will do.
It seems that we will not find better solution than initialize
base_attr_idx to 0. If there aren't any objections or better solution
I will send patch on Monday.
LS