[SSSD] [INI] Patches for ding-libs: Merging config sections, handling metadata, remaining Coverity issues...

Jan Zelený jzeleny at redhat.com
Thu Apr 5 13:54:22 UTC 2012


> On 04/03/2012 08:01 AM, Stephen Gallagher wrote:
> > On Tue, 2012-04-03 at 10:08 +0200, Jan Zelený wrote:
> >>> Ok the whole set once more:
> >>> 
> >>> 1) Fixed the trailing spaces on patch 15 - they showed as warning when
> >>> I applied the patch in a different repo
> >>> 2) The problem with permissions that Jan have seen was caused by the
> >>> fact that when the repo is created as root the permissions on the files
> >>> that are used in tests are different. For now I added a different
> >>> check. In future there might be a need for some improvement in this
> >>> area. I created a task https://fedorahosted.org/sssd/ticket/1284 to
> >>> track this effort.
> >>> 3) Added two missing cleanup calls to patch 27 as Jan rightly indicated
> >>> that they were missing
> >>> 
> >>> All the rest is the same.
> >> 
> >> I'm not completely comfortable with your solution of the access check.
> >> You are just assuming in the comment that the file was created by root
> >> and has therefore different permissions. But what if it wasn't created
> >> by root and still has these different permissions? I think the test
> >> could then let an error in the code pass through. Please check that the
> >> file indeed belongs to root just to be sure. Also error2 is not
> >> necessary IMO, you can re-user error.
> >> 
> >> This is the last minor change I'm asking for. If you want, I'll do it
> >> for you. Other than that all patches are fine.
> > 
> > Actually, I think it's more likely that what's happening here is that
> > root and a casual user have a different default umask on most systems.
> > So this probably IS a bug in the code. You should be setting the umask
> > before creating the file (and resetting the umask to its old value
> > afterwards) in order to guarantee that it is always written with the
> > proper permissions.
> 
> The file is a part of the repo. It is checked into the source control. I
> do not have control on its permissions.
> I think it is the umask but not during execution but rather during git
> clone.
> 
> Yes this area needs more thought but IMO in a separate patch. And may be
> done differently. For example create a file on the fly and make sure it
> has specific permissions. But this is way out of the scope of the
> current patch. This is why I opened a separate ticket to handle it.

Ok, thanks for clarification. Since the issue is in the test suite and it has 
been worked around, I give these patches a green light.

Ack to all

Thanks
Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120405/a5b7263c/attachment.sig>


More information about the sssd-devel mailing list