[PATCH] Fix up discard-const warnings in s4 and tries to remove "discard_const_p"s/"CONST DISCARD"s in both s3 and s4

[Matthias Dieter Wallnöfer]

Hi metze,

thanks for your quick response.

Stefan (metze) Metzmacher schrieb:
> Hi Matthias,
>
>   
>> as suggested by abartlet, I tried to reorganize the patch in smaller
>> commits (e.g. const related warnings, non-const related warnings for s4;
>> const fixes without functional changes - "discard_const_p" macro, const
>> fixes with functional changes for s3...). I hope I made it now easier to
>> review (I know it's a very heavy patch).
>>
>> Therefore I rebased it now in a different private branch here:
>> http://repo.or.cz/w/Samba/mdw.git?a=log;h=refs/heads/const
>>     
>
> please create single patch for the // status = ntvfs_connect(ntvfs_req,
> /* TODO */); change and don't use c++ comments.
>   
Changed and put in a separate commit.
> Why is it so good to remove the usages of discard_const_p()?
>   
Why is it so good to remove ... Basically I would say when you "const" 
something you want to say that the data of the variable remains over the 
whole lifetime "constant" - there shouldn't be a possibility to change 
it. If you want to, you should make a copy. But as we all know the 
"discard_const_p" ignores this restriction and makes variations 
possible. If you consider the declaration on the top of "replace.h" 
there is the comment which says at the beginning "this is a warning hack".
> I just read the first few patches, while some of the patches are really
> nice to have, they're still very large bulk commits combining unrelated
> changes.
>   
For each file a own commit - I don't know - then we had approximately 
150 of them.
> I think a some of the changes are not needed and some introduce real
> bugs, e.g. the talloc_strdup() for the uint8_t buffers in the
> gensec_gssapi and schannel_sign code.
>   
Where exactly lies the problem here? Memory? (So I can understand the 
problem)
> This is also wrong
>
>         do {
>                 long int r = random();
>                 TDB_DATA key, dbuf;
> -               key.dptr = (unsigned char *)"store test";
> +               asprintf((char **) &key.dptr, "store test");
>                 key.dsize = strlen((char *)key.dptr);
> -               dbuf.dptr = (unsigned char *)&r;
> +               dbuf.dptr = (uint8_t *) &r;
>                 dbuf.dsize = sizeof(r);
>                 tdb_store(tdb, key, dbuf, TDB_REPLACE);
>                 t = _end_timer();
> +               SAFE_FREE(key.dptr);
> +               SAFE_FREE(dbuf.dptr);
>                 ops++;
>         } while (t < timelimit);
>
> 1. I think asprintf is not needed
> 2. SAFE_FREE(dbuf.dptr) will try to free &r, a stack variable.
>
> This should also fix this compiler warning and looks much simpler:
>
> -               key.dptr = (unsigned char *)"store test";
> +               key.dptr = discard_const_p(uint8_t, "store test");
>
>   
I fixed this. Thanks for pointing out.

Matthias
> If each single change that can stand on its own, would be a single
> commit, it would be much easier to just cherry-pick the good ones...
>
> metze
>
>   
>> Matthias Dieter Wallnöfer schrieb:
>>     
>>> Hi all developers,
>>>
>>> this patch is the result of a big work:
>>> - for s4 it corrects a huge part of the (const) build warnings and
>>> some others - but not those from heimdal kerberos
>>> - for s3 I removed the "CONST DISCARD" macro and substituted it with
>>> "discard_const_p".
>>> - generally: I tried to substitute on both code bases
>>> "discard_const_p"s on strings with string duplication functions (which
>>> should be the right behavior to modify a const object - you have to
>>> clone it first).
>>> I decided to split it up in the work done for s3, for s4 and for the
>>> common libraries. Therefore not everyone has to watch over the whole
>>> code. I'm sure that the one and the other change won't fit (since I
>>> don't know much about those code parts) so I encourage you to tell me
>>> this and how to make it better or simply to revert to the
>>> "discard_const_p" macro (previous behavior).
>>>
>>> The patch as a whole shouldn't generate new warnings/bring build
>>> failures.
>>>
>>> Since the three commits are big I don't post them here, but I give the
>>> URLs in my personal repo:
>>> s4:
>>> http://repo.or.cz/w/Samba/mdw.git?a=commitdiff;h=9593f12281209a6215835dd4357e0b16e7a760ef
>>>
>>> s3:
>>> http://repo.or.cz/w/Samba/mdw.git?a=commitdiff;h=4741c10cc3927094130b4bfb1b6c5bae17b22cb6
>>>
>>> common:
>>> http://repo.or.cz/w/Samba/mdw.git?a=commitdiff;h=f7d97cbd622e1d162b0a231050433d683286a63f
>>>
>>>
>>> Matthias
>>>
>>>       
>
>
>