Tomas, It is my understanding that the test suite is only for the Python modules. Over time we hope that everything will migrate to use only Python. Thus over time the code will have complete test coverage.
With that said, you do not need to attempt to test those things that are within the setup-sparc script that have changed. Sorry if I made you assume that was required. Therefore, you can remove the test_permissions section and any setup associated with the setup-sparc script within the test_create_service.py module. Thanks, John On Jan 5, 2012, at 3:46 AM, Tomas Dzik wrote: > Hi John, > do you have any comments to this version ? > > Best regards, > > Tomas D. > > Dne 20.12.11 16:52, Tomas Dzik napsal(a): >> Hi John, >> I added test case for dhcp changes and sorted imports in the >> test_create_service.py. New webrev is here: >> >> https://cr.opensolaris.org/action/browse/caiman/t.dzik/7052679-5/ >> >> I still use Popen('setup-sparc') from following reasons: >> >> 1) Fix is in setup-sparc.sh script >> >> 2) The only method from which this script is called is _init_wanboot in >> AIService object. To be able to call this method I would need to create >> instance of this object which would require a lot of work and working >> infrastructure on test server. >> >> Best regards, >> >> Tomas D. >> >> Dne 16.12.11 23:53, John Fischer napsal(a): >>> Tomas, >>> >>> Thanks for creating the test cases. I would have expected >>> _write_config_file() >>> to be called or some other function instead of Popen('sparc_setup.sh'). >>> Also the imports for the test_create_service.py should be alphabetized. >>> Finally, does this test the dhcp changes? >>> >>> Thanks, >>> >>> John >>> >>> >>> On Dec 16, 2011, at 12:09 PM, Tomas Dzik wrote: >>> >>>> Hi John, >>>> I added unit tests for file permissions and fixed typo in bug synopsis. >>>> New webrev is here: >>>> >>>> https://cr.opensolaris.org/action/browse/caiman/t.dzik/7052679-4/ >>>> >>>> Could you please look at it ? >>>> >>>> Best regards, >>>> >>>> Tomas D. >>>> >>>> Dne 12.12.11 17:50, John Fischer napsal(a): >>>>> Tomas, >>>>> >>>>> There are times when a test need not be written because it is >>>>> covered elsewhere >>>>> or some other valid justification can be made. I think in this case >>>>> there is a valid >>>>> justification for a test or set of tests being made. >>>>> >>>>> Perhaps one test where a restrictive umask is used and then a wide >>>>> open umask >>>>> and checking the permissions against what is expected for each >>>>> python module >>>>> with critical file permissions. >>>>> >>>>> Thanks, >>>>> >>>>> John >>>>> >>>>> On Dec 12, 2011, at 8:16 AM, tomas dzik wrote: >>>>> >>>>>> Hi John, >>>>>> is that really a question or you think that I should add these test >>>>>> ? ;-) >>>>>> Also because we are fixing the issue with root having too >>>>>> restrictive umask - do you think that these tests should run with >>>>>> restrictive umask ? If yes - should the restrictive umask be set >>>>>> just in particular test case or as part of setUpClass ? >>>>>> >>>>>> Regards, >>>>>> >>>>>> Tomas D. >>>>>> >>>>>> On 12.12.2011 15:31, John Fischer wrote: >>>>>>> Tomas, >>>>>>> >>>>>>> These changes look good. My only question now is should the tests >>>>>>> be updated >>>>>>> to check the permissions of these files that get created? The only >>>>>>> one that looks >>>>>>> a little challenging to do would be the dhcp.py test. >>>>>>> >>>>>>> Thanks, >>>>>>> >>>>>>> John >>>>>>> >>>>>>> On Dec 12, 2011, at 2:56 AM, Tomas Dzik wrote: >>>>>>> >>>>>>>> Hi all, >>>>>>>> the updated webrev is here: >>>>>>>> >>>>>>>> https://cr.opensolaris.org/action/browse/caiman/t.dzik/7052679-3/ >>>>>>>> >>>>>>>> Could you please let me know whether it is OK now ? >>>>>>>> >>>>>>>> Best regards, >>>>>>>> >>>>>>>> Tomas D. >>>>>>>> >>>>>>>> Dne 9.12.11 01:34, Ethan Quach napsal(a): >>>>>>>>> >>>>>>>>> On 12/08/11 09:42, Tomas Dzik wrote: >>>>>>>>>> Hi John, >>>>>>>>>> it's easy to change that code to use umask instead of chmod. On >>>>>>>>>> the >>>>>>>>>> other hand it's very hard to get consensus on this mailing list >>>>>>>>>> how to >>>>>>>>>> do such simple thing like changing ownership of a file in a >>>>>>>>>> shell script. >>>>>>>>>> >>>>>>>>>> So I would like to know definitive answer before I will change the >>>>>>>>>> code (it should take 2 minutes) and retest it (it takes me more >>>>>>>>>> then 3 >>>>>>>>>> hours to build it on both platforms, run tests, update test ai >>>>>>>>>> servers >>>>>>>>>> on both platforms and to create service and install client >>>>>>>>>> machines). >>>>>>>>>> And this is already 3rd version of this fix. >>>>>>>>>> >>>>>>>>>> Ethan what do you think ? >>>>>>>>> John's suggestion sounds fine to me. >>>>>>>>> >>>>>>>>> -ethan >>>>>>>>> >>>>>>>>>> Best regards, >>>>>>>>>> >>>>>>>>>> Tomas D. >>>>>>>>>> >>>>>>>>>> Dne 8.12.11 18:15, John Fischer napsal(a): >>>>>>>>>>> Tomas, >>>>>>>>>>> >>>>>>>>>>> In that case can you please use a umask so that the >>>>>>>>>>> permissions are >>>>>>>>>>> correct >>>>>>>>>>> when the file is created. >>>>>>>>>>> >>>>>>>>>>> Thanks, >>>>>>>>>>> >>>>>>>>>>> John >>>>>>>>>>> >>>>>>>>>>> On Dec 8, 2011, at 9:06 AM, Tomas Dzik wrote: >>>>>>>>>>> >>>>>>>>>>>> Dne 8.12.11 17:51, John Fischer napsal(a): >>>>>>>>>>>>> Tomas, >>>>>>>>>>>>> >>>>>>>>>>>>> Why are you using umask for everything except the >>>>>>>>>>>>> sparc-setup.sh code? >>>>>>>>>>>>> The chmod still concerns me. Is there a reason why an >>>>>>>>>>>>> appropriate >>>>>>>>>>>>> umask >>>>>>>>>>>>> can not be used in the sparc-setup.sh script? >>>>>>>>>>>>> >>>>>>>>>>>>> Thanks, >>>>>>>>>>>>> >>>>>>>>>>>>> John >>>>>>>>>>>> Hi John, >>>>>>>>>>>> there is no particular reason for using chmod in sparc-setup.sh >>>>>>>>>>>> code. Both ways would work. I just picked one. >>>>>>>>>>>> >>>>>>>>>>>> Best regards, >>>>>>>>>>>> >>>>>>>>>>>> Tomas D. >>>>>>>>>>>> >>>>>>>>>>>>> On Dec 8, 2011, at 8:45 AM, Ethan Quach wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> Tomas, >>>>>>>>>>>>>> >>>>>>>>>>>>>> It looks fine to me. >>>>>>>>>>>>>> >>>>>>>>>>>>>> thanks, >>>>>>>>>>>>>> -ethan >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> On 12/08/11 08:26, Tomas Dzik wrote: >>>>>>>>>>>>>>> Hi Ethan, >>>>>>>>>>>>>>> I changed chmod to absolute 0644 and the latest webrev is >>>>>>>>>>>>>>> here: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> https://cr.opensolaris.org/action/browse/caiman/t.dzik/7052679-2/ >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Could you please look at it and let me know whether is it >>>>>>>>>>>>>>> OK now ? >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Thanks a lot in advance, >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Tomas D. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Dne 7.12.11 09:24, Ethan Quach napsal(a): >>>>>>>>>>>>>>>> Tomas, >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> On 12/06/11 04:43, Tomas Dzik wrote: >>>>>>>>>>>>>>>>> Hi Ethan, >>>>>>>>>>>>>>>>> I was considering all three ways and deciding which way >>>>>>>>>>>>>>>>> to use >>>>>>>>>>>>>>>>> and I >>>>>>>>>>>>>>>>> choose setting umask to be cleanest and safest option. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Combination of os.fdopen(os.open()) does not work because >>>>>>>>>>>>>>>>> according to >>>>>>>>>>>>>>>>> python docs: >>>>>>>>>>>>>>>>> http://docs.python.org/library/os.html#os.open >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> "Open the file file and set various flags according to >>>>>>>>>>>>>>>>> flags and >>>>>>>>>>>>>>>>> possibly its mode according to mode. The default mode is >>>>>>>>>>>>>>>>> 0777 >>>>>>>>>>>>>>>>> (octal), >>>>>>>>>>>>>>>>> and the current umask value is first masked out." >>>>>>>>>>>>>>>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>>>>>>>>>>>>>>> Ok. I didn't realize it masked against the umask for >>>>>>>>>>>>>>>> cases where >>>>>>>>>>>>>>>> a mode >>>>>>>>>>>>>>>> is passed in ... >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Chmoding these files after creation is possible - >>>>>>>>>>>>>>>>> however you >>>>>>>>>>>>>>>>> never >>>>>>>>>>>>>>>>> know what might be security critical. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> For the changes in setup-sparc.sh I intentionally just >>>>>>>>>>>>>>>>> added >>>>>>>>>>>>>>>>> the 'r' >>>>>>>>>>>>>>>>> permission because I know that these files must be >>>>>>>>>>>>>>>>> readable for >>>>>>>>>>>>>>>>> everyone and I didn't want to anticipate anything else - >>>>>>>>>>>>>>>>> it's >>>>>>>>>>>>>>>>> up to >>>>>>>>>>>>>>>>> root to set his umask according to his needs/security >>>>>>>>>>>>>>>>> policy. >>>>>>>>>>>>>>>>> However >>>>>>>>>>>>>>>>> if you feel that application should pretend to be >>>>>>>>>>>>>>>>> smarter then >>>>>>>>>>>>>>>>> admin I >>>>>>>>>>>>>>>>> can easily change it. >>>>>>>>>>>>>>>> But we're effectively doing this for the files being created >>>>>>>>>>>>>>>> where umask >>>>>>>>>>>>>>>> is used, so why do it differently with these two files? >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> So do you want me to change os.umask to os.chmod(00644) in >>>>>>>>>>>>>>>>> python code >>>>>>>>>>>>>>>>> and >>>>>>>>>>>>>>>> Using umask is fine with me here. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> chmod ugo+r to chmod 0644 in setup-sparc.sh ? >>>>>>>>>>>>>>>> I would prefer to set them to 0644 explicitly. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> thanks, >>>>>>>>>>>>>>>> -ethan >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Regards, >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Tomas D. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Dne 6.12.11 03:07, Ethan Quach napsal(a): >>>>>>>>>>>>>>>>>> Tomas, >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Rather than dancing with setting/re-setting the umask, why >>>>>>>>>>>>>>>>>> shouldn't we >>>>>>>>>>>>>>>>>> just explicitly set the perms of the file to exactly >>>>>>>>>>>>>>>>>> what we >>>>>>>>>>>>>>>>>> want at >>>>>>>>>>>>>>>>>> time of creation? >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> I noticed that the built-in open() function does not allow >>>>>>>>>>>>>>>>>> specification >>>>>>>>>>>>>>>>>> of perms for cases where it creates the file, but the >>>>>>>>>>>>>>>>>> os.open() function >>>>>>>>>>>>>>>>>> does, and that can be wrapped around an os.fdopen() >>>>>>>>>>>>>>>>>> call to be >>>>>>>>>>>>>>>>>> used with >>>>>>>>>>>>>>>>>> 'with'. >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> For example, the 'with' line in dhcp.py could be >>>>>>>>>>>>>>>>>> changed to the >>>>>>>>>>>>>>>>>> following to achieve what we need: >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> with os.fdopen(os.open(tmp_cfgfle, os.O_WRONLY | >>>>>>>>>>>>>>>>>> os.O_CREAT, >>>>>>>>>>>>>>>>>> 0644), 'w') >>>>>>>>>>>>>>>>>> as tmp_cfg: >>>>>>>>>>>>>>>>>> .... >>>>>>>>>>>>>>>>>> ... >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> I'm no python expert so I don't know if there are any >>>>>>>>>>>>>>>>>> serious >>>>>>>>>>>>>>>>>> drawbacks >>>>>>>>>>>>>>>>>> to using this versus using the built-in open(), but to me >>>>>>>>>>>>>>>>>> temporarily >>>>>>>>>>>>>>>>>> setting something in the env (the umask in this case) >>>>>>>>>>>>>>>>>> across a >>>>>>>>>>>>>>>>>> chunk of >>>>>>>>>>>>>>>>>> code seems susceptible to future bugs. >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Alternatively, if the file in question is not a >>>>>>>>>>>>>>>>>> security-critical file >>>>>>>>>>>>>>>>>> (which seem to be true for the files in this fix), what's >>>>>>>>>>>>>>>>>> wrong with >>>>>>>>>>>>>>>>>> simply chmod'ing the file to exactly what we want right >>>>>>>>>>>>>>>>>> after its >>>>>>>>>>>>>>>>>> creation? >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> For the changes in setup-sparc.sh, just adding the 'r' >>>>>>>>>>>>>>>>>> perm >>>>>>>>>>>>>>>>>> would still >>>>>>>>>>>>>>>>>> leave some bits susceptible to the umask; so at a >>>>>>>>>>>>>>>>>> minimum, we >>>>>>>>>>>>>>>>>> should >>>>>>>>>>>>>>>>>> just set it entirely to what we want, 0644 in this case. >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> -ethan >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> On 12/05/11 02:23, Tomas Dzik wrote: >>>>>>>>>>>>>>>>>>> This is just a reminder that I would appreciate if >>>>>>>>>>>>>>>>>>> someone could >>>>>>>>>>>>>>>>>>> review these changes. >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> Thanks a lot in advance, >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> Tomas D. >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> Dne 30.11.11 13:14, Tomas Dzik napsal(a): >>>>>>>>>>>>>>>>>>>> Hi all, >>>>>>>>>>>>>>>>>>>> I would like to ask you for a code review for bug: >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> 7052679 AI Sparc client gets wantboot.conf error when >>>>>>>>>>>>>>>>>>>> booting due to >>>>>>>>>>>>>>>>>>>> restrictive umask >>>>>>>>>>>>>>>>>>>> Webrev: >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> https://cr.opensolaris.org/action/browse/caiman/t.dzik/7052679/ >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> Testing: >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> 1) I built the gate with fix on x86 and on sparc. >>>>>>>>>>>>>>>>>>>> 2) Using distro constructor I created AI iso image on >>>>>>>>>>>>>>>>>>>> both >>>>>>>>>>>>>>>>>>>> platforms >>>>>>>>>>>>>>>>>>>> 3) I created virtual machines on x86 and on sparc and >>>>>>>>>>>>>>>>>>>> updated these >>>>>>>>>>>>>>>>>>>> machines to the built gate >>>>>>>>>>>>>>>>>>>> 4) I set umask for root to 0066 (as reported in bug) >>>>>>>>>>>>>>>>>>>> 5) On x86 I created install service. For sparc I >>>>>>>>>>>>>>>>>>>> created it >>>>>>>>>>>>>>>>>>>> from >>>>>>>>>>>>>>>>>>>> publisher and for x86 I created it from built AI iso >>>>>>>>>>>>>>>>>>>> image >>>>>>>>>>>>>>>>>>>> 6) I verified permissions of wanboot.conf and >>>>>>>>>>>>>>>>>>>> system.conf >>>>>>>>>>>>>>>>>>>> 7) I modified manifest, added it as a new default >>>>>>>>>>>>>>>>>>>> manifest and >>>>>>>>>>>>>>>>>>>> installed >>>>>>>>>>>>>>>>>>>> x86 client (also on virtual machine) from this AI >>>>>>>>>>>>>>>>>>>> server. >>>>>>>>>>>>>>>>>>>> I observed that client installed correctly. >>>>>>>>>>>>>>>>>>>> 8) On sparc I created install service for sparc from >>>>>>>>>>>>>>>>>>>> built >>>>>>>>>>>>>>>>>>>> iso, added >>>>>>>>>>>>>>>>>>>> new default manifest and also added new profile. (All >>>>>>>>>>>>>>>>>>>> command were run >>>>>>>>>>>>>>>>>>>> with umask set to 0066.) >>>>>>>>>>>>>>>>>>>> 9) I installed sparc client from this AI server and >>>>>>>>>>>>>>>>>>>> verified >>>>>>>>>>>>>>>>>>>> that >>>>>>>>>>>>>>>>>>>> client >>>>>>>>>>>>>>>>>>>> installed correctly and used the right manifest and >>>>>>>>>>>>>>>>>>>> profile. >>>>>>>>>>>>>>>>>>>> 10) Sources are pep8 clean. >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> Best regards, >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> Tomas D. >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> _______________________________________________ >>>>>>>>>>>>>>>>>>>> caiman-discuss mailing list >>>>>>>>>>>>>>>>>>>> [email protected] >>>>>>>>>>>>>>>>>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> _______________________________________________ >>>>>>>>>>>>>>>>>>> caiman-discuss mailing list >>>>>>>>>>>>>>>>>>> [email protected] >>>>>>>>>>>>>>>>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> _______________________________________________ >>>>>>>>>>>>>>>>>> caiman-discuss mailing list >>>>>>>>>>>>>>>>>> [email protected] >>>>>>>>>>>>>>>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>> _______________________________________________ >>>>>>>>>>>>>> caiman-discuss mailing list >>>>>>>>>>>>>> [email protected] >>>>>>>>>>>>>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >>>>> >>>> >>> >> >> _______________________________________________ >> caiman-discuss mailing list >> [email protected] >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss > _______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
