Re: [zones-discuss] s10 brand Phase I webrev
Ed, Thanks for reviewing this again. I took most of your input. For the questions you had or the things I didn't take, I have responded below. Edward Pilatowicz wrote: - could you propegate back your common changes to the original file? I don't want to complicate this project with the additional changes to the sn1 brand and the corresponding additional testing. I filed bug: 6888642 sn1 brand cleanup so that we can track that work as a separate integration. - there is duplicated code here from the pkg(5) brand common.ksh. perhaps the common code should go in /usr/lib/brand/shared/common.ksh? fail_fatal() get_zonepath_ds() get_active_ds() get_zonepath_ds() is not common since the ipkg brand has the additional dependency on the global zone BE which does not exist for the solaris10 brand. - in create_active_ds(), newly created datasets will have different values from pre-existing datasets. new datasets will inherit the mountpoint and zoned properties while existing datasets will have these explicitly set. (if your worried about these having incorrect values for existing datasets, perhaps you should re-inherit them instead of setting them.) We don't want to inherit these, we want to set them. The values will change as the zone gets detached/attached so we always want to set the values we need. - in sysunconfig_zone(), the comment says it sleeps 10 seconds, but then it does 10 iterations of a loop with sleep 10 in it. i feel like i've made this exact same code review comment to you recently. is this code duplicated from the ipkg p2v code? No, it came from the native p2v, just as the ipkg code did. I made the fix here that I also made for the ipkg work. - in sysunconfig_zone(), if the zone never gets to single-user mode then should we return 1 instead of charging ahead and trying to run sys-unconfig? (since in that case sys-unconfig could hang.) I think its worth trying anyway since there may be something else going on and we don't know for sure if sys-unconfig will hang. -- usr/src/lib/brand/solaris10/s10_support/s10_support.c - get_image_emul_version(), doesn't this essentially duplicate the functionality provided by the /usr/lib/brand/solaris10/version file delivered via s10? in both cases we're deriving an emulation version based on the s10 bits within the zone. could you explain why this is necessary and under what conditions each versioning mechanism will be used? I changed this so that we still check for the minimum KU and we now use the optional version file from S10 to determine if we need a specific version of the emulation. - get_image_emul_version(), does an == comparison on the KU. that means whenever a new KU is release, we'll need to update this code. does that mean you forsee verification test runs for each s10 KU, and a subsequent update to this code once the tests complete? if so please add comments to the code explaning this. No, thats incorrect. A new KU will always incorporate the old KU. For example, if you were running the S10u6 KU and then installed the S10u9 KU, your system would then look like it had the u6, u7, u8 and u9 KUs installed. -- usr/src/lib/brand/solaris10/zone/attach.ksh - you have the following comment: # # Try to create a zfs dataset for the zonepath (this is automatically # done by zoneadm for the install subcommand but not for attach). # why not change zoneadm attach to be consistent with install and create these dataset when possible. (seems like that would benifit the ipkg brand as well.) I filed the following bug for that enhancement but I don't want to make that part of this project. 6888652 zoneadm attach should try to create a zfs dataset -- usr/src/lib/brand/solaris10/s10_brand/common/s10_brand.c - since the zc_name, zc_value, and zc_string are all arrays embedded within the zfs_cmd_t, there is no reason to use uucopy to access them individually. Yes, since these are embedded arrays, they must be copied, we can't simply do an assignment. I did change the uucopy to bcopy so that I didn't have to do the cast. - in s10_zone, in the ZONE_GETATTR() case, why isn't S10_TRUSS_POINT_5 used instead of S10_TRUSS_POINT_3? Because the first 3 parameters are required for a truss point. That is, S10_TRUSS_POINT_0 takes 3 parameters, S10_TRUSS_POINT_3 takes 6, which is the number of parameters we're passing. Thanks again, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] s10 brand Phase I webrev
Jordan, Thanks for reviewing this again. I took most of your input. For the things I didn't take, I have responded below. Jordan Vaughan wrote: usr/src/uts/common/brand/solaris10/s10_brand.c 1260-1261,1286-1287,1313,etc.: Couldn't we make arg1 a zoneid_t, arg2 an int, arg3 a char *, and arg4 a size_t and eliminate some of the casts in s10_zone() (as well as some of the automatic variables, e.g., buf and bufsize)? Since thats not always the types of the parameters passed, I don't want to change this since it would complicate any work we might do in the future. -- usr/src/lib/brand/solaris10/s10_support/s10_support.c get_image_emul_version(): I agree with Ed that get_image_emul_version() is superfluous. Now that I've thought about it, $ZONEROOT/usr/lib/brand/solaris10/version should be sufficient for the brand to determine whether it can host the associated S10C. All we need to do is hard-code the maximum version number supported by the brand (for example, as a preprocessor constant), fetch the version number stored in $ZONEROOT/usr/lib/brand/solaris10/version (or zero if the file does not exist), check whether the latter exceeds the former, and set the brand's emulation number to that stored in $ZONEROOT/usr/lib/brand/solaris10/version. See my response to Ed on what I did here. The check for the minimal KU is not superfluous, but I did rewrite this as I explained to Ed. Thanks again, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] s10 brand Phase I webrev
On Tue, Oct 06, 2009 at 09:47:40AM -0600, Jerry Jelinek wrote: Ed, Thanks for reviewing this again. I took most of your input. For the questions you had or the things I didn't take, I have responded below. Edward Pilatowicz wrote: - could you propegate back your common changes to the original file? I don't want to complicate this project with the additional changes to the sn1 brand and the corresponding additional testing. I filed bug: 6888642 sn1 brand cleanup so that we can track that work as a separate integration. sigh. are you commiting to this work? the sn1 brand always get's neglected (says the guy who spent too much time fixing it up to be a real brand). also, i would have though you'd commited to doing this work when you decided to fork the sn1 brand code instead of making it common. besides, aside from the elfexec changes all the changes are Makefile related, right? that's not a whole lot of extra testing. - there is duplicated code here from the pkg(5) brand common.ksh. perhaps the common code should go in /usr/lib/brand/shared/common.ksh? fail_fatal() get_zonepath_ds() get_active_ds() get_zonepath_ds() is not common since the ipkg brand has the additional dependency on the global zone BE which does not exist for the solaris10 brand. ok. but if get_zonepath_ds() is not common, then why are you adding it to usr/src/lib/brand/native/zone/common.ksh? - in create_active_ds(), newly created datasets will have different values from pre-existing datasets. new datasets will inherit the mountpoint and zoned properties while existing datasets will have these explicitly set. (if your worried about these having incorrect values for existing datasets, perhaps you should re-inherit them instead of setting them.) We don't want to inherit these, we want to set them. The values will change as the zone gets detached/attached so we always want to set the values we need. i dont' understand, currently we don't always set these values. if we do a fresh install, mountpoint and zoned are inherited: ---8--- e...@mcescher$ zfs get -o source mountpoint,zoned export/zones/z1/ROOT/zbe SOURCE inherited from export/zones/z1/ROOT inherited from export/zones/z1/ROOT ---8--- so why the difference for attached zones? for attached zones you zfs set the values above. why not just zfs inherit instead. (you already explicitly set them for the ROOT dataset, so they would be correct.) - in sysunconfig_zone(), if the zone never gets to single-user mode then should we return 1 instead of charging ahead and trying to run sys-unconfig? (since in that case sys-unconfig could hang.) I think its worth trying anyway since there may be something else going on and we don't know for sure if sys-unconfig will hang. could you add comments explaining this? -- usr/src/lib/brand/solaris10/s10_support/s10_support.c - get_image_emul_version(), does an == comparison on the KU. that means whenever a new KU is release, we'll need to update this code. does that mean you forsee verification test runs for each s10 KU, and a subsequent update to this code once the tests complete? if so please add comments to the code explaning this. No, thats incorrect. A new KU will always incorporate the old KU. For example, if you were running the S10u6 KU and then installed the S10u9 KU, your system would then look like it had the u6, u7, u8 and u9 KUs installed. please add comments explaining this. -- usr/src/lib/brand/solaris10/s10_brand/common/s10_brand.c - in s10_zone, in the ZONE_GETATTR() case, why isn't S10_TRUSS_POINT_5 used instead of S10_TRUSS_POINT_3? Because the first 3 parameters are required for a truss point. That is, S10_TRUSS_POINT_0 takes 3 parameters, S10_TRUSS_POINT_3 takes 6, which is the number of parameters we're passing. but i thought the caller passed in 4 parameters. (in addition to the cmd.) why are you not printing out buf and bufsize? ed ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] s10 brand Phase I webrev
also, i would have though you'd commited to doing this work when you decided to fork the sn1 brand code instead of making it common. I was wondering about this too. Indeed, there seems be a sizeable amount of duplicated code now. Why is this the right design? -- meem ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] s10 brand Phase I webrev
Ed, Edward Pilatowicz wrote: On Tue, Oct 06, 2009 at 09:47:40AM -0600, Jerry Jelinek wrote: Ed, Thanks for reviewing this again. I took most of your input. For the questions you had or the things I didn't take, I have responded below. Edward Pilatowicz wrote: - could you propegate back your common changes to the original file? I don't want to complicate this project with the additional changes to the sn1 brand and the corresponding additional testing. I filed bug: 6888642 sn1 brand cleanup so that we can track that work as a separate integration. sigh. are you commiting to this work? the sn1 brand always get's neglected (says the guy who spent too much time fixing it up to be a real brand). Sure. I just don't want these coupled together. also, i would have though you'd commited to doing this work when you decided to fork the sn1 brand code instead of making it common. besides, aside from the elfexec changes all the changes are Makefile related, right? that's not a whole lot of extra testing. - there is duplicated code here from the pkg(5) brand common.ksh. perhaps the common code should go in /usr/lib/brand/shared/common.ksh? fail_fatal() get_zonepath_ds() get_active_ds() get_zonepath_ds() is not common since the ipkg brand has the additional dependency on the global zone BE which does not exist for the solaris10 brand. ok. but if get_zonepath_ds() is not common, then why are you adding it to usr/src/lib/brand/native/zone/common.ksh? Sorry, I meant get_active_ds(), not get_zonepath_ds(). - in create_active_ds(), newly created datasets will have different values from pre-existing datasets. new datasets will inherit the mountpoint and zoned properties while existing datasets will have these explicitly set. (if your worried about these having incorrect values for existing datasets, perhaps you should re-inherit them instead of setting them.) We don't want to inherit these, we want to set them. The values will change as the zone gets detached/attached so we always want to set the values we need. i dont' understand, currently we don't always set these values. if we do a fresh install, mountpoint and zoned are inherited: ---8--- e...@mcescher$ zfs get -o source mountpoint,zoned export/zones/z1/ROOT/zbe SOURCE inherited from export/zones/z1/ROOT inherited from export/zones/z1/ROOT ---8--- so why the difference for attached zones? for attached zones you zfs set the values above. why not just zfs inherit instead. (you already explicitly set them for the ROOT dataset, so they would be correct.) OK, I misunderstood what you meant. I'll change this. - in sysunconfig_zone(), if the zone never gets to single-user mode then should we return 1 instead of charging ahead and trying to run sys-unconfig? (since in that case sys-unconfig could hang.) I think its worth trying anyway since there may be something else going on and we don't know for sure if sys-unconfig will hang. could you add comments explaining this? Sure. -- usr/src/lib/brand/solaris10/s10_support/s10_support.c - get_image_emul_version(), does an == comparison on the KU. that means whenever a new KU is release, we'll need to update this code. does that mean you forsee verification test runs for each s10 KU, and a subsequent update to this code once the tests complete? if so please add comments to the code explaning this. No, thats incorrect. A new KU will always incorporate the old KU. For example, if you were running the S10u6 KU and then installed the S10u9 KU, your system would then look like it had the u6, u7, u8 and u9 KUs installed. please add comments explaining this. Sure. -- usr/src/lib/brand/solaris10/s10_brand/common/s10_brand.c - in s10_zone, in the ZONE_GETATTR() case, why isn't S10_TRUSS_POINT_5 used instead of S10_TRUSS_POINT_3? Because the first 3 parameters are required for a truss point. That is, S10_TRUSS_POINT_0 takes 3 parameters, S10_TRUSS_POINT_3 takes 6, which is the number of parameters we're passing. but i thought the caller passed in 4 parameters. (in addition to the cmd.) why are you not printing out buf and bufsize? Those parameters aren't that useful for debugging. I can add them if you'd like. Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] s10 brand Phase I webrev
Peter Memishian wrote: also, i would have though you'd commited to doing this work when you decided to fork the sn1 brand code instead of making it common. I was wondering about this too. Indeed, there seems be a sizeable amount of duplicated code now. Why is this the right design? Because the sn1 brand is an internal brand for testing and is not delivered to customers. Once the solaris10 brand is integrated, the sn1 brand will no longer serve its original role and could even be removed from the source tree if we wanted to. Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] s10 brand Phase I webrev
On Tue, Oct 06, 2009 at 12:15:23PM -0600, Jerry Jelinek wrote: Ed, Edward Pilatowicz wrote: On Tue, Oct 06, 2009 at 09:47:40AM -0600, Jerry Jelinek wrote: Ed, Thanks for reviewing this again. I took most of your input. For the questions you had or the things I didn't take, I have responded below. Edward Pilatowicz wrote: - could you propegate back your common changes to the original file? I don't want to complicate this project with the additional changes to the sn1 brand and the corresponding additional testing. I filed bug: 6888642 sn1 brand cleanup so that we can track that work as a separate integration. sigh. are you commiting to this work? the sn1 brand always get's neglected (says the guy who spent too much time fixing it up to be a real brand). Sure. I just don't want these coupled together. then please make sure to update the bug with a detailed list of all the differences between the two. (should be easy since i already hilighted all the differences in my code review comments.) -- usr/src/lib/brand/solaris10/s10_brand/common/s10_brand.c - in s10_zone, in the ZONE_GETATTR() case, why isn't S10_TRUSS_POINT_5 used instead of S10_TRUSS_POINT_3? Because the first 3 parameters are required for a truss point. That is, S10_TRUSS_POINT_0 takes 3 parameters, S10_TRUSS_POINT_3 takes 6, which is the number of parameters we're passing. but i thought the caller passed in 4 parameters. (in addition to the cmd.) why are you not printing out buf and bufsize? Those parameters aren't that useful for debugging. I can add them if you'd like. yes please. otherwise some anal retentive person who is debugging a problem will get distracted by the fact that the buf pointer and size values are invalid. ed ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] s10 brand Phase I webrev
Ed, Edward Pilatowicz wrote: On Tue, Oct 06, 2009 at 12:15:23PM -0600, Jerry Jelinek wrote: Ed, Edward Pilatowicz wrote: On Tue, Oct 06, 2009 at 09:47:40AM -0600, Jerry Jelinek wrote: Ed, Thanks for reviewing this again. I took most of your input. For the questions you had or the things I didn't take, I have responded below. Edward Pilatowicz wrote: - could you propegate back your common changes to the original file? I don't want to complicate this project with the additional changes to the sn1 brand and the corresponding additional testing. I filed bug: 6888642 sn1 brand cleanup so that we can track that work as a separate integration. sigh. are you commiting to this work? the sn1 brand always get's neglected (says the guy who spent too much time fixing it up to be a real brand). Sure. I just don't want these coupled together. then please make sure to update the bug with a detailed list of all the differences between the two. (should be easy since i already hilighted all the differences in my code review comments.) Sure. I used the file list from your comments but I'll go ahead and paste all of them in. -- usr/src/lib/brand/solaris10/s10_brand/common/s10_brand.c - in s10_zone, in the ZONE_GETATTR() case, why isn't S10_TRUSS_POINT_5 used instead of S10_TRUSS_POINT_3? Because the first 3 parameters are required for a truss point. That is, S10_TRUSS_POINT_0 takes 3 parameters, S10_TRUSS_POINT_3 takes 6, which is the number of parameters we're passing. but i thought the caller passed in 4 parameters. (in addition to the cmd.) why are you not printing out buf and bufsize? Those parameters aren't that useful for debugging. I can add them if you'd like. yes please. otherwise some anal retentive person who is debugging a problem will get distracted by the fact that the buf pointer and size values are invalid. Will do. Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] s10 brand Phase I webrev
On Tue, Oct 06, 2009 at 12:18:21PM -0600, Jerry Jelinek wrote: Peter Memishian wrote: also, i would have though you'd commited to doing this work when you decided to fork the sn1 brand code instead of making it common. I was wondering about this too. Indeed, there seems be a sizeable amount of duplicated code now. Why is this the right design? Because the sn1 brand is an internal brand for testing and is not delivered to customers. Once the solaris10 brand is integrated, the sn1 brand will no longer serve its original role and could even be removed from the source tree if we wanted to. really? i'd have to disagree. i was actually expecting that when nevada dies we'd have to update the sn1 brand to work on opensolaris. i always thought you forked the code because that was faster than re-factoring it to be common. the sn1 brand is still the ideal method for testing the brandz framework itself. using the s10 brand to test the brandz framework is ok, but then you're also testing the s10 emulation layer, and that emulation is far from complete. (witness all the currently unsupported s10 functionality.) as an example, take a look at todds current bug. from todds (a developers) perspective, i think it'd be much better to reproduce the problem, debug it, fix it, and test it on the sn1 brand than on the s10 brand. the sn1 brand makes brandz framework development and regression testing easier for developers. (it's actually a bug, and a dis-service to ourselves, that we never integrated it into our automated zones testing.) the fact that the sn1 code is also for the most part duplicated in the s8 and s9 brands is also lousy. hopefully no one ever makes a successfull business case for porting those to opensolaris. ed ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] s10 brand Phase I webrev
Peter Memishian wrote: I was wondering about this too. Indeed, there seems be a sizeable amount of duplicated code now. Why is this the right design? Because the sn1 brand is an internal brand for testing and is not delivered to customers. Once the solaris10 brand is integrated, the sn1 brand will no longer serve its original role and could even be removed from the source tree if we wanted to. I don't see how that addresses the primary point, which is that Solaris brands seem to suffer from code duplication. Are you asserting that the amount of code duplication between the sn1 and solaris10 brands is unique to that situation and is not something that will occur again when we cons up the next solarisX brand? Yes. If we were to ship another brand that was fundamentally similar to the solaris10 brand (e.g. the solaris9 brand on Solaris Next), then I think it would make sense for that project to try to make more of the code common with the then currently shipping solaris10 brand. However, I don't think that is necessary for a brand we don't ship (but I can also understand if you don't agree with me). Once the solaris10 brand is integrated, I would hope that we would focus our limited resources on the one brand we do ship across all architectures and I would anticipate that the sn1 brand will be of limited usefulness (since its main reason for existence is to test brandz on sparc). If we do anything else with sn1 after this brand integrates is outside the scope of this project and not something we've even discussed (other than my commitment to fix the sn1 brand per Ed's code review comments). Of course, its up to future brand projects to evaluate what makes the most sense for their project and I can't commit those hypothetical projects to anything. Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] s10 brand Phase I webrev
Ed, Edward Pilatowicz wrote: really? i'd have to disagree. i was actually expecting that when nevada dies we'd have to update the sn1 brand to work on opensolaris. i always thought you forked the code because that was faster than re-factoring it to be common. No, that wasn't my thinking, but as I said, we've never discussed the future of the sn1 brand. If we think this is an important issue, we can look at it. From looking at the webrev, I see maybe 11 source files (not counting makefiles, pkging files or configuration files) that might be candidates for commonality (i.e. files with 10% lines of difference). This doesn't seem like a burning issue to me. Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] s10 brand Phase I webrev
I don't see how that addresses the primary point, which is that Solaris brands seem to suffer from code duplication. Are you asserting that the amount of code duplication between the sn1 and solaris10 brands is unique to that situation and is not something that will occur again when we cons up the next solarisX brand? Yes. If we were to ship another brand that was fundamentally similar to the solaris10 brand (e.g. the solaris9 brand on Solaris Next), then I think it would make sense for that project to try to make more of the code common with the then currently shipping solaris10 brand. However, I don't think that is necessary for a brand we don't ship (but I can also understand if you don't agree with me). Indeed, I don't agree, for three reasons: 1. You are establishing a precedent that the way we develop new Solaris brands is via cut-and-paste in a manner that's expedient for the implementor. When the next brand comes along, this existing example will likely be used as justification for why they should not be burdened with the task of properly factoring the software. 2. Shipping or not, the sn1 codebase is still part of our product and needs to be maintained. By forking the source, we are doubling the cost of that maintenance and making it easy for things to get out of sync (and indeed, the tax of maintaining these parallel brands has already started to be levied before the solaris10 brand has even integrated). We've all seen this many times but yet we keep doing it. 3. It is precisely because of our limited resources that we must adhere to best practices and commonize code rather than fork it. All that said, ultimately this is an issue for you and the rest of the zones team to resolve, so I'll bow out of this, but I strongly disagree with the approach that's been taken. -- meem ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] s10 brand Phase I webrev
On 10/ 1/09 05:40 AM, Jerry Jelinek wrote: Edward Pilatowicz wrote: i'm not done yet, but i've attached what i've got so far. Ed, Thanks for your comments. I'll start to work through these while we're waiting for the rest of your input and respond if there is anything we're not going to address. Thank again, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org Hi Jerry, I have a few nits and questions aside from Ed's. Thanks, Jordan -- usr/src/lib/brand/solaris10/cmd/s10_automount.sh usr/src/lib/brand/solaris10/cmd/s10_automountd.sh Shouldn't the scripts' parameters be included at the end of the last line (the exec command) as in s10_isaexec_wrapper.sh? -- usr/src/lib/brand/solaris10/librtld_db/common/solaris10_librtld_db.c The ps_plog() invocation in s10_ldb_fini32() displays lx_ldb_fini when it should be s10_ldb_fini (right?). According to the diff, the sn1 version also uses lx_ldb_fini. -- usr/src/uts/common/brand/solaris10/s10_brand.c 165-171: Are we going to retain the lx brand comments? I remember asking this two or three months ago, but someone answered that we wanted it to look the same as in the lx brand file from which the comment originated. Why? 740-743: I could've simplified this a bit by combining both cases. These lines can be condensed to case CT_TGET: case CT_TSET: return (ctfs_ioctl(rval, fdes, cmd, arg)); 927-928: I could've improved this comment by stating that the path of the *dynamic linker* is the second parameter of s10_native_exec(). 1260-1261,1286-1287,1313,etc.: Couldn't we make arg1 a zoneid_t, arg2 an int, arg3 a char *, and arg4 a size_t and eliminate some of the casts in s10_zone() (as well as some of the automatic variables, e.g., buf and bufsize)? 1298: Shouldn't we move this truss point below the switch block? As it currently stands, if a process issues SYS_zone to get an attribute of the global zone other than ZONE_ATTR_NAME and ZONE_ATTR_BRAND, then truss would report two SYS_zone syscalls instead of one. -- usr/src/lib/brand/solaris10/s10_support/s10_support.c 289-296: Isn't this whole loop simply looking for SUNWcakr's pkginfo file in the zone? If so, then looping through the zone's /var/sadm/pkg directory's entries for SUNWcakr is superfluous: get_ku_patchlist() could simply construct the path $ZONEPATH/root/var/sadm/pkg/SUNWcakr/pkginfo and stat() will fail if it doesn't exist. (Are we planning to examine other packages for patch lists?) get_image_emul_version(): I agree with Ed that get_image_emul_version() is superfluous. Now that I've thought about it, $ZONEROOT/usr/lib/brand/solaris10/version should be sufficient for the brand to determine whether it can host the associated S10C. All we need to do is hard-code the maximum version number supported by the brand (for example, as a preprocessor constant), fetch the version number stored in $ZONEROOT/usr/lib/brand/solaris10/version (or zero if the file does not exist), check whether the latter exceeds the former, and set the brand's emulation number to that stored in $ZONEROOT/usr/lib/brand/solaris10/version. 467,471-472,476-477: The first conditional can be changed to argc != 3 and the other two can be deleted along with their invocations of usage(). ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] s10 brand Phase I webrev
Jordan Vaughan wrote: I have a few nits and questions aside from Ed's. Jordan, Thanks for looking this over. I'll address these once I finish going through Ed's comments. Thanks again, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] s10 brand Phase I webrev
Edward Pilatowicz wrote: i'm not done yet, but i've attached what i've got so far. Ed, Thanks for your comments. I'll start to work through these while we're waiting for the rest of your input and respond if there is anything we're not going to address. Thank again, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] s10 brand Phase I webrev
i've finished looking through the rest of the files and my comments are attached. ed On Tue, Sep 29, 2009 at 05:09:26PM -0600, Jerry Jelinek wrote: Edward Pilatowicz wrote: - also, since the s10 brand is derived from the sn1 brand, could you please ensure that all the new s10 brand that are being created are derived from the corresponding sn1 brand files? ie, the s10 brand files which are derived from sn1 brand files should be created via hg cp ... instead of cp ...; hg new ... (this will preserve the sn1 brand revision history when looking at s10 brand files.) additionally, danek has an enhanced version of webrev where, if the s10 branded files are hg cpd from the sn1 files, we'll just see the deltas against the sn1 files (instead of having all these files show up as new). I've generated a new webrev using some improvements Ed made to webrev. This, combined with the use of 'hg copy', make the webrev much smaller and easier to review. I've uploaded the new webrev to: http://cr.opensolaris.org/~gjelinek/webrev.646/ Please get me any comments by the end of the week so that we have time to make the necessary changes and rerun the tests before we integrate. Thanks, Jerry -- usr/src/lib/brand/solaris10/s10_support/s10_support.c - s10_err() should probably write the error to stderr instead of stdout. - s10_verify(), you don't support adding sound devices, but you do support other devices? seems kinda arbitrary? perhaps a comment explaining this would help? - s10_verify(), why are zfs fsent entries experimental? zone administrators don't have any abilities to manipulate properties on those types of zfs filesystem. from the zones perspective they are treated the same as ufs, tmpfs, vxfs, etc. only POSIX operations are permitted on these filesystems. (afaik.) - get_image_emul_version(), doesn't this essentially duplicate the functionality provided by the /usr/lib/brand/solaris10/version file delivered via s10? in both cases we're deriving an emulation version based on the s10 bits within the zone. could you explain why this is necessary and under what conditions each versioning mechanism will be used? - get_image_emul_version(), does an == comparison on the KU. that means whenever a new KU is release, we'll need to update this code. does that mean you forsee verification test runs for each s10 KU, and a subsequent update to this code once the tests complete? if so please add comments to the code explaning this. -- usr/src/lib/brand/solaris10/zone/clone.ksh - you should check for errors from mkdir and chmod -- usr/src/lib/brand/solaris10/zone/p2v.ksh - shouldn't safe_rm() go into /usr/lib/brand/shared/common.ksh - should safe_rm return an error if the object exists but isn't a file? - please check error codes from mktemp - please check error codes from chown and chmod. -- usr/src/lib/brand/solaris10/zone/image_install.ksh - trap_cleanup(), comment talks about IPDs - why set EXIT_CODE=$ZONE_SUBPROC_OK at 249? if we ctrl-C the process after this and the zone won't have the install log in it but it will still be listed as installed. - can't you use safe_dir() to verify /var in the zone? -- usr/src/lib/brand/solaris10/zone/attach.ksh - trap_cleanup(), comment talks about IPDs - you have the following comment: # # Try to create a zfs dataset for the zonepath (this is automatically # done by zoneadm for the install subcommand but not for attach). # why not change zoneadm attach to be consistent with install and create these dataset when possible. (seems like that would benifit the ipkg brand as well.) - same comments about zfs property inheritance vs explicit setting that i made about create_active_ds() in common.ksh apply here. actually, why can't you just add an argument to create_active_ds() to indicate if the datasets must already exist, and then just invoke that here? - the two /var/log comments i made about image_install.ksh apply here. so why not create a function that does all the /var/log checks and copies the log files into the zone and put it in common.ksh? -- usr/src/lib/brand/solaris10/s10_brand/common/s10_brand.c - the d, s, and val parameters should have parens around them in the zfs_assign macro. - there is nothing zfs specified about the zfs_assign macro. this would be more appropritely names struct_member_assign or struct_member_copy. - since the zc_name, zc_value, and zc_string are all arrays embedded within the zfs_cmd_t, there is no reason to use uucopy to access them individually. - given the level of indentation in s10_auditsys(), wouldn't it make sense to reduce the indentation by doing: if (bsmcmd != BSM_AUDITCTL) return (__systemcall(rval, SYS_auditsys + 1024, bsmcmd, a0, a1, a2)); - the comment above s10_exec_native() says:
Re: [zones-discuss] s10 brand Phase I webrev
i'm not done yet, but i've attached what i've got so far. ed On Tue, Sep 29, 2009 at 05:09:26PM -0600, Jerry Jelinek wrote: Edward Pilatowicz wrote: - also, since the s10 brand is derived from the sn1 brand, could you please ensure that all the new s10 brand that are being created are derived from the corresponding sn1 brand files? ie, the s10 brand files which are derived from sn1 brand files should be created via hg cp ... instead of cp ...; hg new ... (this will preserve the sn1 brand revision history when looking at s10 brand files.) additionally, danek has an enhanced version of webrev where, if the s10 branded files are hg cpd from the sn1 files, we'll just see the deltas against the sn1 files (instead of having all these files show up as new). I've generated a new webrev using some improvements Ed made to webrev. This, combined with the use of 'hg copy', make the webrev much smaller and easier to review. I've uploaded the new webrev to: http://cr.opensolaris.org/~gjelinek/webrev.646/ Please get me any comments by the end of the week so that we have time to make the necessary changes and rerun the tests before we integrate. Thanks, Jerry -- usr/src/lib/brand/solaris10/s10_brand/Makefile - could you propegate back your common changes to the original file? -- usr/src/lib/brand/solaris10/s10_support/Makefile - instead of: ../../../../uts could you do: $(SRC)/uts - could you propegate back your common changes to the original file? -- usr/src/lib/brand/solaris10/librtld_db/Makefile.com - could you propegate back your common changes to the original file? -- usr/src/lib/brand/solaris10/zone/config.xml - could you propegate back your common changes to the original file? -- usr/src/lib/brand/solaris10/zone/platform.xml - could you propegate back your common changes to the original file? -- usr/src/uts/common/brand/solaris10/s10_brand.c - in s10_getattr() and s10_setattr(), how about just doing: if (bufsize != sizeof (int)) and then you don't need to bother setting bufsize in s10_getattr(). - in s10_elfexec() it seems like the changes required to make ld.so.1 run (ie, sedp-sed_entry = NULL, up-u_auxv[i].a_un.a_val =) run should be backported to the sn1 brand. -- usr/src/lib/brand/solaris10/zone/uninstall.ksh - this seems to largely be a duplicate of the unistall script from the pkg(5) gate. i only see two differences between the scripts: - the method used to determine the zfs filesystems for the zone (beadm vs zoneadm) and filtering based on uuids. - the code at the very end under the Check if there are any other datasets left. comment. this one seems like a potential mismerge, since i would think that you'd actually want to keep this bit of code. to avoid duplicating this entire script, could you please move most this code into usr/lib/brand/shared and either: - make it a common script that is called by a brand specific script that passes it a list of datasets to delete - or have the script take a command line option which controls how the list of datasets to delete is determined. then after you putback the ipkg brand can be updated to use this same code. -- usr/src/lib/brand/solaris10/zone/query.ksh - also a dup of the query in the pkg(5) gate, should be common. -- usr/src/lib/brand/solaris10/zone/s10_boot.ksh - is_zone_dir() looks like a dup of /usr/lib/brand/shared/common.ksh`safe_dir() - shouldn't safe_replace() go into /usr/lib/brand/shared/common.ksh? -- usr/src/lib/brand/solaris10/zone/detach.ksh - do you use anything from? /usr/lib/brand/shared/common.ksh - why? ZONEROOT=$ZONEPATH/root logdir=$ZONEROOT/var/log -- usr/src/lib/brand/solaris10/zone/common.ksh - shouldn't the EXIT_* definitions go in /usr/lib/brand/shared/common.ksh? - hm. i find the PROP_ACTIVE and libbe references strange. - there is duplicated code here from the pkg(5) brand common.ksh. perhaps the common code should go in /usr/lib/brand/shared/common.ksh? fail_fatal() get_zonepath_ds() get_active_ds() - in create_active_ds(), newly created datasets will have different values from pre-existing datasets. new datasets will inherit the mountpoint and zoned properties while existing datasets will have these explicitly set. (if your worried about these having incorrect values for existing datasets, perhaps you should re-inherit them instead of setting them.) - in create_active_ds(), you should check the return value for mkdir. - in sysunconfig_zone(), the comment says it sleeps 10 seconds, but then it does 10 iterations of a loop with sleep 10 in it. i feel like i've made this exact same code review comment to you recently. is this code duplicated from the ipkg p2v code? - in
Re: [zones-discuss] s10 brand Phase I webrev
Edward Pilatowicz wrote: - also, since the s10 brand is derived from the sn1 brand, could you please ensure that all the new s10 brand that are being created are derived from the corresponding sn1 brand files? ie, the s10 brand files which are derived from sn1 brand files should be created via hg cp ... instead of cp ...; hg new ... (this will preserve the sn1 brand revision history when looking at s10 brand files.) additionally, danek has an enhanced version of webrev where, if the s10 branded files are hg cpd from the sn1 files, we'll just see the deltas against the sn1 files (instead of having all these files show up as new). I've generated a new webrev using some improvements Ed made to webrev. This, combined with the use of 'hg copy', make the webrev much smaller and easier to review. I've uploaded the new webrev to: http://cr.opensolaris.org/~gjelinek/webrev.646/ Please get me any comments by the end of the week so that we have time to make the necessary changes and rerun the tests before we integrate. Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] s10 brand Phase I webrev
Edward Pilatowicz wrote: hey jerry, do you have an updated ws+webrev where the s10 files were created using hg cp? (i'm waiting for that before doing a review.) also, when were you planning to integrate? (so i can avoid a last minute rush.) Ed, I wasn't aware that this was holding you up. I haven't done anything on it yet. I'll work on regenerating the webrevs by tomorrow and send out an announcement. We are targeting b127 so it would be good to get any comments this week so that there is time to respond and test any changes. Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] s10 brand Phase I webrev
On Mon, Sep 28, 2009 at 03:06:00PM -0600, Jerry Jelinek wrote: Edward Pilatowicz wrote: hey jerry, do you have an updated ws+webrev where the s10 files were created using hg cp? (i'm waiting for that before doing a review.) also, when were you planning to integrate? (so i can avoid a last minute rush.) Ed, I wasn't aware that this was holding you up. I haven't done anything on it yet. I'll work on regenerating the webrevs by tomorrow and send out an announcement. We are targeting b127 so it would be good to get any comments this week so that there is time to respond and test any changes. ah. yeah. i was waiting because i wanted to use the hg cp' workspace to generate a webrev using daneks copy of webrev. (since that would show just the delta to all the copied files, there by greatly reducing the amount of code to review.) ed ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] s10 brand Phase I webrev
two high level comments to start with. - since the s10 brand is derived from the sn1 brand, are there any framework changes which were made to the s10 brand that should be backported to the sn1 brand? - also, since the s10 brand is derived from the sn1 brand, could you please ensure that all the new s10 brand that are being created are derived from the corresponding sn1 brand files? ie, the s10 brand files which are derived from sn1 brand files should be created via hg cp ... instead of cp ...; hg new ... (this will preserve the sn1 brand revision history when looking at s10 brand files.) additionally, danek has an enhanced version of webrev where, if the s10 branded files are hg cpd from the sn1 files, we'll just see the deltas against the sn1 files (instead of having all these files show up as new). ed On Tue, Sep 15, 2009 at 08:48:19AM -0600, Jerry Jelinek wrote: We've completed the development for the Phase I work on the solaris10 brand. I've posted a full webrev at: http://cr.opensolaris.org/~gjelinek/webrev.646/ Let me know if there are any comments. Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org ___ zones-discuss mailing list zones-discuss@opensolaris.org
[zones-discuss] s10 brand Phase I webrev
We've completed the development for the Phase I work on the solaris10 brand. I've posted a full webrev at: http://cr.opensolaris.org/~gjelinek/webrev.646/ Let me know if there are any comments. Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] s10 brand Phase I webrev
We've completed the development for the Phase I work on the solaris10 brand. I've posted a full webrev at: http://cr.opensolaris.org/~gjelinek/webrev.646/ Let me know if there are any comments. I see that ip-type=exclusive is regarded as experimental in s10_support.c; is that planned for Phase II? -- meem ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] s10 brand Phase I webrev
Peter Memishian wrote: We've completed the development for the Phase I work on the solaris10 brand. I've posted a full webrev at: http://cr.opensolaris.org/~gjelinek/webrev.646/ Let me know if there are any comments. I see that ip-type=exclusive is regarded as experimental in s10_support.c; is that planned for Phase II? Yes, that's right. We haven't done any of the testing or evaluation yet on what it will take to make exclusive fully work for the brand but we will do that for Phase II. Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
