> 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
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
