Looks good. John
On Jan 9, 2012, at 4:49 AM, Tomas Dzik wrote: > Hi John, > I removed that test from test_create_service.py module. > New webrev is here: > > https://cr.opensolaris.org/action/browse/caiman/t.dzik/7052679-6/webrev/ > > Is that OK now ? > > Best regards, > > Tomas D. > > Dne 5.01.12 17:41, John Fischer napsal(a): >> 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
