[pkg-discuss] code review request for CR 7120902
Abhinandan Ekande
Please review fix for CR :
7120902 var/cache/pkg/sysrepo recreated with broken perms
webrev :
https://cr.opensolaris.org/action/browse/pkg/saurabhv/CR-7120902-rev1/webrev/
Unit testing done :
* Removed the directory /var/cache/pkg/sysrep
* Then enabled the system-repository servic
* Verified that service is comes online
* Verified the permission on the directory, /var/cache/pkg/sysrep
Here are details :
# rmdir /var/cache/pkg/sysrepo
# svcadm enable svc:/application/pkg/system-repository:default
# svcs -a | grep system-repository
online 9:10:25 svc:/application/pkg/system-repository:default
# ls -ld /system/volatile/pkg/sysrepo
drwxr-xr-x 3 root root 322 Feb 23 09:10 /system/volatile/pkg/sysrepo
# ls -ld /var/cache/pkg/sysrepo
drwxr-xr-x 2 root root 2 Feb 23 09:10 /var/cache/pkg/sysrepo
# ls -ld /var/log/pkg/sysrepo
drwxr-xr-x 2 root bin 5 Feb 23 09:10 /var/log/pkg/sysrepo
Thanks,
Abhi.
--
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
pkg-d...@opensolaris.org
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
Brock Pytlik
> Folks,
>
> Please review fix for CR :
> 7120902 var/cache/pkg/sysrepo recreated with broken perms
>
> webrev :
> https://cr.opensolaris.org/action/browse/pkg/saurabhv/CR-7120902-rev1/webrev/
>
> Unit testing done :
>
> * Removed the directory /var/cache/pkg/sysrep
> * Then enabled the system-repository servic
> * Verified that service is comes online
> * Verified the permission on the directory, /var/cache/pkg/sysrep
>
> Here are details :
>
> # rmdir /var/cache/pkg/sysrepo
> # svcadm enable svc:/application/pkg/system-repository:default
> # svcs -a | grep system-repository
> online 9:10:25 svc:/application/pkg/system-repository:default
> # ls -ld /system/volatile/pkg/sysrepo
> drwxr-xr-x 3 root root 322 Feb 23 09:10 /system/volatile/pkg/sysrepo
> # ls -ld /var/cache/pkg/sysrepo
> drwxr-xr-x 2 root root 2 Feb 23 09:10 /var/cache/pkg/sysrepo
> # ls -ld /var/log/pkg/sysrepo
> drwxr-xr-x 2 root bin 5 Feb 23 09:10 /var/log/pkg/sysrepo
>
> Thanks,
> Abhi.
>
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?
Brock
Tim Foster
> 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
Abhinandan Ekande
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
Abhinandan Ekande
Added the test case also.
The updated webrev is located at :
https://cr.opensolaris.org/action/browse/pkg/saurabhv/CR-7120902-rev2/webrev/
Completed unit testing. As well ran the test suite which was successful.
Thanks,
Abhi.
Tim Foster
> 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.
line 309, why are you changing the permissions?
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.
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.
cheers,
tim
_______________________________________________
Abhinandan Ekande
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.
--
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
Tim Foster
> 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/
>> 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.
Fair enough (though this code runs for several other directories too)
It looks like the intent there was to ensure that the system repository
runtime dir was set to 700, but I can't see why we need that now.
Either way, the code and the manifest should agree, so that seems right.
>> 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.
You're right, sorry - I was led astray by the creative use of 'dir.find'
there. Any reason why you didn't use 'dir == foo' rather than
'dir.find(foo)'?
Also, rather than relying on the user and group of the parent
directories of the cache_dir being set correctly, it's probably better
to use the 'SYSREPO_USER' and 'SYSREPO_GROUP' globals - see line 497 for
an example.
cheers,
tim
Abhinandan Ekande
> You're right, sorry - I was led astray by the creative use of
> 'dir.find' there. Any reason why you didn't use 'dir == foo' rather
> than 'dir.find(foo)'?
I will do check as you have suggested.
>
> Also, rather than relying on the user and group of the parent
> directories of the cache_dir being set correctly, it's probably better
> to use the 'SYSREPO_USER' and 'SYSREPO_GROUP' globals - see line 497
> for an example.
The SYSREPO_GROUP is initialized as 'pkg5srv'. As per manifest the gid for
cache directory is 'bin'. Therefore I took the route of setting user and
group as per
parent directory of cache directory. Is it okay to set group as
'pkg5srv' for cache
directory ? Or is initializing of SYSREPO_GROUP incorrect ? Please let
me know.
Thanks,
Abhi.
>
> cheers,
> tim
> _______________________________________________
> pkg-discuss mailing list
> pkg-d...@opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
--
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
Abhinandan Ekande
The updated webrev is located at :
https://cr.opensolaris.org/action/browse/pkg/saurabhv/CR-7120902-rev4/webrev/
Please let me know your comments.
Thanks,
Abhi.
Tim Foster
On 03/ 6/12 11:45 PM, Abhinandan Ekande wrote:
> Tim, as per discussion with you over IM I have made the changes.
> The updated webrev is located at :
> https://cr.opensolaris.org/action/browse/pkg/saurabhv/CR-7120902-rev4/webrev/
This looks fine, with the one addition, that it'd be nice if sysrepo.py
still ran as non-root. You could check $PKG5_TEST_ENV when trying to
chown to pkg5srv - see line 502 for an example.
cheers,
tim
> Please let me know your comments.
Abhinandan Ekande
Here is latest webrev :
https://cr.opensolaris.org/action/browse/pkg/saurabhv/CR-7120902-rev5/webrev/
Thanks for helping me to resolve the test issue when it was run as
non-root.
Abhi.
Tim Foster
> https://cr.opensolaris.org/action/browse/pkg/saurabhv/CR-7120902-rev5/webrev/
>
> Thanks for helping me to resolve the test issue when it was run as
> non-root.
No problem, that change looks good to me. Thanks for fixing it.
cheers,
tim