Bug 5739 - Cosmetic corrections
Cosmetic corrections
Status: RESOLVED FIXED
Product: Samba 4.0
Classification: Unclassified
Component: Other
unspecified
Other All
: P3 normal
: ---
Assigned To: Andrew Bartlett
Andrew Bartlett
http://repo.or.cz/w/Samba/mdw.git?a=l...
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-07 15:30 UTC by Matthias Dieter Wallnöfer
Modified: 2008-10-13 09:57 UTC (History)
2 users (show)

See Also:


Attachments
KERBEROS cosmetic patch (5.81 KB, patch)
2008-09-07 15:35 UTC, Matthias Dieter Wallnöfer
no flags Details
DSDB cosmetic patch (51.76 KB, patch)
2008-09-07 15:55 UTC, Matthias Dieter Wallnöfer
no flags Details
LDB library cosmetic patch (51.76 KB, patch)
2008-09-07 15:56 UTC, Matthias Dieter Wallnöfer
no flags Details
LDB backends cosmetic patch (51.76 KB, patch)
2008-09-07 15:56 UTC, Matthias Dieter Wallnöfer
no flags Details
DSDB cosmetic patch (4.97 KB, patch)
2008-09-07 16:01 UTC, Matthias Dieter Wallnöfer
no flags Details
LDB library cosmetic patch (7.40 KB, patch)
2008-09-07 16:01 UTC, Matthias Dieter Wallnöfer
no flags Details
LDB backends cosmetic patch (16.27 KB, patch)
2008-09-07 16:02 UTC, Matthias Dieter Wallnöfer
no flags Details
DSDB cosmetic patch (4.97 KB, patch)
2008-09-08 04:34 UTC, Matthias Dieter Wallnöfer
no flags Details
Last correction (1.33 KB, patch)
2008-09-24 16:54 UTC, Matthias Dieter Wallnöfer
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Matthias Dieter Wallnöfer 2008-09-07 15:30:33 UTC
I'll provide here patches who perform cosmetic correctures in the SAMBA 4 code.
Comment 1 Matthias Dieter Wallnöfer 2008-09-07 15:35:56 UTC
Created attachment 3534 [details]
KERBEROS cosmetic patch
Comment 2 Matthias Dieter Wallnöfer 2008-09-07 15:55:43 UTC
Created attachment 3535 [details]
DSDB cosmetic patch
Comment 3 Matthias Dieter Wallnöfer 2008-09-07 15:56:12 UTC
Created attachment 3536 [details]
LDB library cosmetic patch
Comment 4 Matthias Dieter Wallnöfer 2008-09-07 15:56:36 UTC
Created attachment 3537 [details]
LDB backends cosmetic patch
Comment 5 Matthias Dieter Wallnöfer 2008-09-07 16:01:13 UTC
Created attachment 3538 [details]
DSDB cosmetic patch

Corrected version
Comment 6 Matthias Dieter Wallnöfer 2008-09-07 16:01:48 UTC
Created attachment 3539 [details]
LDB library cosmetic patch

Corrected version
Comment 7 Matthias Dieter Wallnöfer 2008-09-07 16:02:10 UTC
Created attachment 3540 [details]
LDB backends cosmetic patch

Corrected version
Comment 8 Matthias Dieter Wallnöfer 2008-09-08 04:34:21 UTC
Created attachment 3541 [details]
DSDB cosmetic patch

Updated DSDB cosmetic patch for changed kludge_acl module.
Comment 9 Simo Sorce 2008-09-19 17:33:09 UTC
(In reply to comment #6)
> Created an attachment (id=3539) [edit]
> LDB library cosmetic patch
> 
> Corrected version
> 

This patch is not completely ok.
While in most cases your change from a numberic code to LDB_ERR_OPERATIONS_ERROR is ok, in some other it is not.
Look for example at: ldb_should_b64_encode()

This function should probably really return a boolean true|false, but changing the return of 1 (which means true here) to LDB_ERR_OPERATIONS_ERROR is semantically wrong.

Please check each caller and make sure that what they expect in return is actually an ldb error.
I now of other places within ldb where returning 1,0,-1 has meaning and it would break if something else is returned.

Simo.
Comment 10 Matthias Dieter Wallnöfer 2008-09-20 06:08:34 UTC
1.) Add also Jelmer to this bug, because he is also interested in it.

2.) Regarding comment #9 (Simo):
It's for me a bit difficult to know where the constants are appropriate and where not. Also due to the fact that I haven't written the code. It would be a lot easier to correct my patches if I knew at least the filenames (modules) or function forms (e.g. comparison functions not) where the patching isn't allowed/required.
Comment 11 Matthias Dieter Wallnöfer 2008-09-20 06:16:18 UTC
Comment on attachment 3534 [details]
KERBEROS cosmetic patch

This isn't the latest version of the patch (see the GIT branch for a newer version).
Comment 12 Simo Sorce 2008-09-20 10:48:01 UTC
(In reply to comment #10)

> It's for me a bit difficult to know where the constants are appropriate and
> where not. Also due to the fact that I haven't written the code. It would be a
> lot easier to correct my patches if I knew at least the filenames (modules) or
> function forms (e.g. comparison functions not) where the patching isn't
> allowed/required.

The easiest way is to use something like ctags/cscope to jump to all users of a function you modify and verify how they use the return. I did this for a sample of functions and that's how I saw the patch was not completely correct.
Once you know how a calss of functions operate wrt return value you can skip strict checking for all functions of that class.

Comment 13 Matthias Dieter Wallnöfer 2008-09-24 15:35:06 UTC
Good, in the meantime the patches are merged, but the LDB results remain to fix. I hope I'll do some investigations on how to fix it in the next days.
Comment 14 Matthias Dieter Wallnöfer 2008-09-24 16:54:22 UTC
Created attachment 3628 [details]
Last correction

All patches should be applied. Here I provide a last update to satisfy Simo's concerns about "ldb_should_b64_encode()". I checked also the other functions with the "cscope" utility and found nothing special. After this is applied, it should be safe to close the bug.
Comment 15 Matthias Dieter Wallnöfer 2008-09-25 09:24:16 UTC
Good, I've rebased my cosmetic branch now, did some checks with "cscope" and wait for the review.
Comment 16 Matthias Dieter Wallnöfer 2008-09-29 07:52:50 UTC
My branch has been updated due to the LDB ASYNC changes.
Comment 17 Matthias Dieter Wallnöfer 2008-10-13 09:57:31 UTC
Patches applied.