Tim thanks for review.
I have made the changes and updated webrev is located at :
https://cr.opensolaris.org/action/browse/pkg/saurabhv/CR-7120902-rev3/webrev/
See inline ...
On 03/02/12 02:05, Tim Foster wrote:
On 03/ 2/12 12:11 AM, Abhinandan Ekande wrote:
Made the changes to set owner as pkg5srv for /var/cache/pkg/sysrepo.
Added the test case also.
The updated webrev is located at :
https://cr.opensolaris.org/action/browse/pkg/saurabhv/CR-7120902-rev2/webrev/
src/sysrepo.py
line 293, don't hardcode the name of the cache directory - there's a
cache_dir variable passed to the method that you should use instead.
Agree. I have incorporated this comment.
line 309, why are you changing the permissions?
If user removes the cache directory, then it needs to be recreated with
permission 0755.
Hence changed the permission to 0755.
line 310, you're chowning all files in the cache, which seems like
overkill, but you're also doing it 3 or 4 times. I think it's enough
to simply ensure the cachedir itself has the correct permissions.
In the code we are just chowning the cache directory and not all the
files in the cache.
src/tests/cli/t_sysrepo.py
line 484, if something goes wrong with the sysrepo, this could cause
the testsuite to hang indefinitely (which would be bad). It would be
better to look for the existence of the cache directory a fixed number
of times, before giving up and throwing a test failure.
This is a good catch. I have made change to loop finite number of times
before reporting
the error if sysrepo service does not start.
Abhi.
cheers,
tim
Completed unit testing. As well ran the test suite which was successful.
Thanks,
Abhi.
On 02/24/12 16:18, Abhinandan Ekande wrote:
Thanks Brock and Tim for review.
I will make the changes to set owner as pkg5src for sysrepo directory
and post the webrev.
Thanks,
Abhi.
On 02/24/12 12:23, Tim Foster wrote:
On 02/24/12 07:20 PM, Brock Pytlik wrote:
On 02/23/12 21:03, Abhinandan Ekande wrote:
Folks,
Please review fix for CR :
7120902 var/cache/pkg/sysrepo recreated with broken perms
This change looks correct, but I'd like to see a test added to pkg's
test suite to check for this condition. In addition, I'm curious
about
the user and group, shouldn't we make sure that the directory is
created
as pkg5srv, and not root as described in the bug?
Yep, it should be created 0755 as the pkg5srv user, just the way it
was packaged in the system repository package manifest:
dir group=bin mode=0755 owner=pkg5srv path=var/cache/pkg
dir group=bin mode=0755 owner=pkg5srv path=var/cache/pkg/sysrepo
The sysrepo.py script runs as root but chowns everything to pkg5srv,
since that's the user that Apache ultimately runs its processes as.
I missed the original bug, but deleting random packaged system
directories doesn't seem to be a good move on the whole. I agree it'd
be nice if we worked around every occurrence of that however.
cheers,
tim
--
Oracle
Abhinandan Ekande
Solaris Install,
Revenue Product Engineering (RPE), Systems
Phone: +91 8041847267 | Fax: +91 80 22231794 | Mobile: +91 9632144088
ORACLE India | Off Langford Road | Bangalore | 560025
Green Oracle <http://www.oracle.com/commitment> Oracle is committed to
developing practices and products that help protect the environment
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
