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

Stefan (metze) Metzmacher metze at samba.org
Tue Sep 29 06:30:16 MDT 2009


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.

Why is it so good to remove the usages of discard_const_p()?

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.

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.

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");


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
>>
> 



More information about the samba-technical mailing list