Re: [zones-discuss] code review: native brand refactoring
Ed, Edward Pilatowicz wrote: > hey jerry, > some final comments. > ed > > - could you update the sn1 brand so that it will still work? > (it's broken on x86 because of 6703962, but it should still work on sparc.) I'll take a look at what is going on there. > usr/src/lib/libbrand/dtd/brand.dtd.1 > > - so after reading the comments for "predetach" and "detach" > i still have no idea what the difference is between when > the two callbacks are invoked. I had gotten similar input from Dan. I have added more comments to try to clarify this. > - for the "clone" callback, is it possible for the clone operation > to fail after the callback has been invoked? (in which case the > callback needs to be re-run friendly like the "*detach" callbacks.) Yes it can fail, but it can't be re-run. I think the comments already explain this: If this hook exits with a non-zero exit status, the clone operation will fail and the zone will be left in the "incomplete" state, otherwise the state will be changed to "installed". Let me know if you think I still need to clarify that. > - "validatesnap", perhaps the description could be changed to: > Identifies the hook to invoke when cloning a zone to validate > the source zone snapshot that should have been created > using the built-in ZFS clone support. I added some additional comments to clarify this. Thanks again, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
hey jerry, some final comments. ed - could you update the sn1 brand so that it will still work? (it's broken on x86 because of 6703962, but it should still work on sparc.) usr/src/lib/libbrand/dtd/brand.dtd.1 - so after reading the comments for "predetach" and "detach" i still have no idea what the difference is between when the two callbacks are invoked. - for the "clone" callback, is it possible for the clone operation to fail after the callback has been invoked? (in which case the callback needs to be re-run friendly like the "*detach" callbacks.) - "validatesnap", perhaps the description could be changed to: Identifies the hook to invoke when cloning a zone to validate the source zone snapshot that should have been created using the built-in ZFS clone support. On Fri, May 30, 2008 at 08:40:22AM -0600, Jerry Jelinek wrote: > I believe I have resolved all of the code review > comments received so far. I have posted an updated > webrev at the same url. > > http://cr.opensolaris.org/~gjelinek/webrev/ > > I'll be doing some final testing with these changes > so if there are any additional comments, please let > me know soon. > > Thanks, > Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
I believe I have resolved all of the code review comments received so far. I have posted an updated webrev at the same url. http://cr.opensolaris.org/~gjelinek/webrev/ I'll be doing some final testing with these changes so if there are any additional comments, please let me know soon. Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
Edward Pilatowicz wrote: > On Wed, May 28, 2008 at 01:00:08PM -0600, Jerry Jelinek wrote: >> Edward Pilatowicz wrote: >>> hm. from what i remember mount mode actually puts the zone root at >>> /a and lofs mounts stuff from the global zone at , >>> all so that the svr4 packaging code can "enter" the zone to do packaging >>> operations. i thought this dance was necessary because we wanted packaging >>> scripts to execute inside a zone (since for security reasons, we wouldn >>> t want them to be able to muck with stuff outside a zone). but IPS doesn't >>> have packaging scripts. so will this complicated setup really be necessary >>> for IPS? ie, could we do away with the /a mount in mount mode for IPS? >>> >>> i'm asking about this because the /a mount code in zoneadmd has lots of hard >>> coded paths and makes zoneadmd pretty complicated. (just look at >>> mount_filesystems() and build_mounted_post_var().) i've had to modify this >>> code a few times, i've broken it a couple times, and been confused by it all >>> the time... >> Ed, >> >> Yes, this description is correct but not because of the pkging >> code. We want to be able to enter the zone while running the >> sw that is installed in the global zone, irrespective of what is >> inside the non-global zone. All of the global zone sw is mounted >> read-only in a mounted zone so we can login into the zone and >> have a safe environment that matches the global zone. The non-global >> zone is mounted under /a so we can safely operate on it within the zone >> using the global zones sw. None of this has any relationship to >> the pkging code in either zone. All the mount is doing is setting >> up a safe zone that is compatible with the global zone. With this >> definition, you can use mount to safely do any kind of admin work, >> not just running pkging code. The problem with the current mount >> is it lofs mounts the zone's etc and var back into the global zone >> mount area. This obviously won't work for non-native zones or even >> older zones that we are migrating to the new host, which is why I >> had to fix this for update on attach. >> > > ah. ok. thanks for clearing that up for me. i assume all that stuff > was for zulu/lu and we would be able to rip it out someday... Ed, It was originally added for zulu but I think this is generally useful and we wouldn't want to remove it, but just make it work for all brand types. For example, if you try to install an rpm from the gz into a lx branded zone that has been previously booted (assuming that is possible), then the same security issues exist as with svr4 pkgs. I think we want to maintain the idea that we have a safe way to do gz administration of ngz's for all sorts of admin operations, not just pkging, and the mount gives us that. Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
On Wed, May 28, 2008 at 01:00:08PM -0600, Jerry Jelinek wrote: > Edward Pilatowicz wrote: >> hm. from what i remember mount mode actually puts the zone root at >> /a and lofs mounts stuff from the global zone at , >> all so that the svr4 packaging code can "enter" the zone to do packaging >> operations. i thought this dance was necessary because we wanted packaging >> scripts to execute inside a zone (since for security reasons, we wouldn >> t want them to be able to muck with stuff outside a zone). but IPS doesn't >> have packaging scripts. so will this complicated setup really be necessary >> for IPS? ie, could we do away with the /a mount in mount mode for IPS? >> >> i'm asking about this because the /a mount code in zoneadmd has lots of hard >> coded paths and makes zoneadmd pretty complicated. (just look at >> mount_filesystems() and build_mounted_post_var().) i've had to modify this >> code a few times, i've broken it a couple times, and been confused by it all >> the time... > > Ed, > > Yes, this description is correct but not because of the pkging > code. We want to be able to enter the zone while running the > sw that is installed in the global zone, irrespective of what is > inside the non-global zone. All of the global zone sw is mounted > read-only in a mounted zone so we can login into the zone and > have a safe environment that matches the global zone. The non-global > zone is mounted under /a so we can safely operate on it within the zone > using the global zones sw. None of this has any relationship to > the pkging code in either zone. All the mount is doing is setting > up a safe zone that is compatible with the global zone. With this > definition, you can use mount to safely do any kind of admin work, > not just running pkging code. The problem with the current mount > is it lofs mounts the zone's etc and var back into the global zone > mount area. This obviously won't work for non-native zones or even > older zones that we are migrating to the new host, which is why I > had to fix this for update on attach. > ah. ok. thanks for clearing that up for me. i assume all that stuff was for zulu/lu and we would be able to rip it out someday... ed ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
Edward Pilatowicz wrote: > hm. from what i remember mount mode actually puts the zone root at > /a and lofs mounts stuff from the global zone at , > all so that the svr4 packaging code can "enter" the zone to do packaging > operations. i thought this dance was necessary because we wanted packaging > scripts to execute inside a zone (since for security reasons, we wouldn > t want them to be able to muck with stuff outside a zone). but IPS doesn't > have packaging scripts. so will this complicated setup really be necessary > for IPS? ie, could we do away with the /a mount in mount mode for IPS? > > i'm asking about this because the /a mount code in zoneadmd has lots of hard > coded paths and makes zoneadmd pretty complicated. (just look at > mount_filesystems() and build_mounted_post_var().) i've had to modify this > code a few times, i've broken it a couple times, and been confused by it all > the time... Ed, Yes, this description is correct but not because of the pkging code. We want to be able to enter the zone while running the sw that is installed in the global zone, irrespective of what is inside the non-global zone. All of the global zone sw is mounted read-only in a mounted zone so we can login into the zone and have a safe environment that matches the global zone. The non-global zone is mounted under /a so we can safely operate on it within the zone using the global zones sw. None of this has any relationship to the pkging code in either zone. All the mount is doing is setting up a safe zone that is compatible with the global zone. With this definition, you can use mount to safely do any kind of admin work, not just running pkging code. The problem with the current mount is it lofs mounts the zone's etc and var back into the global zone mount area. This obviously won't work for non-native zones or even older zones that we are migrating to the new host, which is why I had to fix this for update on attach. Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
On Wed, May 28, 2008 at 12:24:07PM -0600, Jerry Jelinek wrote: > Ed, > > Responses below. > > Edward Pilatowicz wrote: >> On Tue, May 27, 2008 at 08:48:17AM -0600, Jerry Jelinek wrote: >>> I have updated the webrev at: >>> >>> http://cr.opensolaris.org/~gjelinek/webrev/ >>> >>> This includes the changes for the feedback I have >>> received so far. I also added the zlogin.c file >>> to the webrev with two bug fixes. One of these was for >>> a bug I was hitting during testing of these changes >>> and there is a second bug in zlogin that came in >>> which I also fixed. So, at a minimum, it would be >>> good to take a look at that additional file. >>> >> >> here's my last batch of comments. >> (for sw_support.c i only looked at the new functions you introduced.) >> ed >> >> -- >> general >> >> - iirc, there is still tons of zulu code in zoneadmd. this code creates >> the special live upgrade environment, and LU is tied to svr4 >> packaging, right? so it seems like the zulu code is brand specific >> and should also be factored out into "mount" callbacks. (and iirc, >> the "mount" operations isn't currenly supported by any branded zones.) >> thoughts? > > I don't think there is really any zulu code in zoneadmd but I'll > take a pass through to double check. If I see anything like > that, I'll fix it up and update the webrev. > > The mount code is not really svr4 pkg specific either, but is > specific to the native brand. I have extensions to mount which > would allow us to mount any type of brand with very little change. > I added those with the update on attach work. Being able to > mount any zone is something I've thought would be handy for a long > time now, so I'll take a look at that as I am making the pass through > zoneadmd. > hm. from what i remember mount mode actually puts the zone root at /a and lofs mounts stuff from the global zone at , all so that the svr4 packaging code can "enter" the zone to do packaging operations. i thought this dance was necessary because we wanted packaging scripts to execute inside a zone (since for security reasons, we wouldn t want them to be able to muck with stuff outside a zone). but IPS doesn't have packaging scripts. so will this complicated setup really be necessary for IPS? ie, could we do away with the /a mount in mount mode for IPS? i'm asking about this because the /a mount code in zoneadmd has lots of hard coded paths and makes zoneadmd pretty complicated. (just look at mount_filesystems() and build_mounted_post_var().) i've had to modify this code a few times, i've broken it a couple times, and been confused by it all the time... ed ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
Ed, Responses below. Edward Pilatowicz wrote: > On Tue, May 27, 2008 at 08:48:17AM -0600, Jerry Jelinek wrote: >> I have updated the webrev at: >> >> http://cr.opensolaris.org/~gjelinek/webrev/ >> >> This includes the changes for the feedback I have >> received so far. I also added the zlogin.c file >> to the webrev with two bug fixes. One of these was for >> a bug I was hitting during testing of these changes >> and there is a second bug in zlogin that came in >> which I also fixed. So, at a minimum, it would be >> good to take a look at that additional file. >> > > here's my last batch of comments. > (for sw_support.c i only looked at the new functions you introduced.) > ed > > -- > general > > - iirc, there is still tons of zulu code in zoneadmd. this code creates > the special live upgrade environment, and LU is tied to svr4 > packaging, right? so it seems like the zulu code is brand specific > and should also be factored out into "mount" callbacks. (and iirc, > the "mount" operations isn't currenly supported by any branded zones.) > thoughts? I don't think there is really any zulu code in zoneadmd but I'll take a pass through to double check. If I see anything like that, I'll fix it up and update the webrev. The mount code is not really svr4 pkg specific either, but is specific to the native brand. I have extensions to mount which would allow us to mount any type of brand with very little change. I added those with the update on attach work. Being able to mount any zone is something I've thought would be handy for a long time now, so I'll take a look at that as I am making the pass through zoneadmd. > > -- > usr/src/lib/brand/native/zone/sw_support.c > > - in main(), an invalid cmd value will result in returning an > uninitialzed err value. makes me wonder, is this code linting > properly? I am not getting any lint warning from that, which is surprising. I fixed this. > - nit: in install_func(), status should be initialized to Z_ERR not 1. Fixed. > - in install_func(), if the snprintf() overflows, you'll pass a > a constant error value (Z_ERR) to subproc_status() (which is > expecting a waitpid() encoded status value). That was old code that just got moved, but it is broken and I fixed it. > - hm. attach_func() looks for the -F force flag, but as discussed > previously, in the case of forced attach, the attach brand callback > is currently not executed. I duplicated that when I split things up. I have removed it. Thanks again, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
On Tue, May 27, 2008 at 08:48:17AM -0600, Jerry Jelinek wrote: > I have updated the webrev at: > > http://cr.opensolaris.org/~gjelinek/webrev/ > > This includes the changes for the feedback I have > received so far. I also added the zlogin.c file > to the webrev with two bug fixes. One of these was for > a bug I was hitting during testing of these changes > and there is a second bug in zlogin that came in > which I also fixed. So, at a minimum, it would be > good to take a look at that additional file. > here's my last batch of comments. (for sw_support.c i only looked at the new functions you introduced.) ed -- general - iirc, there is still tons of zulu code in zoneadmd. this code creates the special live upgrade environment, and LU is tied to svr4 packaging, right? so it seems like the zulu code is brand specific and should also be factored out into "mount" callbacks. (and iirc, the "mount" operations isn't currenly supported by any branded zones.) thoughts? -- usr/src/lib/brand/native/zone/sw_support.c - in main(), an invalid cmd value will result in returning an uninitialzed err value. makes me wonder, is this code linting properly? - nit: in install_func(), status should be initialized to Z_ERR not 1. - in install_func(), if the snprintf() overflows, you'll pass a a constant error value (Z_ERR) to subproc_status() (which is expecting a waitpid() encoded status value). - hm. attach_func() looks for the -F force flag, but as discussed previously, in the case of forced attach, the attach brand callback is currently not executed. ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
Edward Pilatowicz wrote: > On Wed, May 28, 2008 at 07:02:32AM -0600, Jerry Jelinek wrote: >>> - in the case of a forced attach why don't we run the brand callback? >> That is really the definition of forced. We don't do anything when >> forced except change the state of the zone to installed. >> > > hm. that differs from my concept of a forced operation, which > i thought always meant, do all the same things you normally do, but > ignore any errors encountered along the way. > > if we do a forced boot, do we invoke the brand boot callback? > > if you leave it as is, could you document this behavior in the dtd > comment? Ed, I'll update the dtd since this behavior hasn't changed with this update to the code and I would prefer to keep it as is. Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
On Wed, May 28, 2008 at 07:02:32AM -0600, Jerry Jelinek wrote: > >> - in the case of a forced attach why don't we run the brand callback? > > That is really the definition of forced. We don't do anything when > forced except change the state of the zone to installed. > hm. that differs from my concept of a forced operation, which i thought always meant, do all the same things you normally do, but ignore any errors encountered along the way. if we do a forced boot, do we invoke the brand boot callback? if you leave it as is, could you document this behavior in the dtd comment? ed ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
Edward Pilatowicz wrote: > On Wed, May 28, 2008 at 07:02:32AM -0600, Jerry Jelinek wrote: >>> - why is there no locking around the pre-detach hook? >> That hasn't changed with this code but if I recall correctly, the idea >> was that predetach is not making any changes to the zone so we don't >> lock until actual changes to the state are being made. >> > > hm. that differs from the description in the dtd. the dtd says the > callback should be carefull making changes since it could be invoked > multiple times. it doesn't say anything about being read-only or > concurrent invocations. i'd probably just be best to include locking > around the pre-detach call. Ed, OK, I'll move the locking up. Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
Nicolas Williams wrote: > On Tue, May 27, 2008 at 08:48:17AM -0600, Jerry Jelinek wrote: >> This includes the changes for the feedback I have >> received so far. I also added the zlogin.c file >> to the webrev with two bug fixes. One of these was for >> a bug I was hitting during testing of these changes >> and there is a second bug in zlogin that came in >> which I also fixed. So, at a minimum, it would be >> good to take a look at that additional file. > > Why timeout the poll(2) call when you can have a signal handler that > writes a byte into a pipe that you could poll() for? > > That's the "standard" way (well, one of them) to handle signals + > polling. (E.g., we use it in Sun_SSH.) > > If you do it this way you get more responsiveness and you don't waste > time waking up the zlogin process when there's nothing to do (this last > is important w.r.t. power consumption). Noco, Thanks for looking at this. I'll take a look at implementing your suggestion. Thanks again, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
On Wed, May 28, 2008 at 07:02:32AM -0600, Jerry Jelinek wrote: > >> - why is there no locking around the pre-detach hook? > > That hasn't changed with this code but if I recall correctly, the idea > was that predetach is not making any changes to the zone so we don't > lock until actual changes to the state are being made. > hm. that differs from the description in the dtd. the dtd says the callback should be carefull making changes since it could be invoked multiple times. it doesn't say anything about being read-only or concurrent invocations. i'd probably just be best to include locking around the pre-detach call. ed ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
See also: 6475075 zlogin i/o loop needs work 6263984 zlogin's i/o loop can sometimes drop data on child death On Wed, May 28, 2008 at 11:53:18AM -0500, Nicolas Williams wrote: > On Tue, May 27, 2008 at 08:48:17AM -0600, Jerry Jelinek wrote: > > This includes the changes for the feedback I have > > received so far. I also added the zlogin.c file > > to the webrev with two bug fixes. One of these was for > > a bug I was hitting during testing of these changes > > and there is a second bug in zlogin that came in > > which I also fixed. So, at a minimum, it would be > > good to take a look at that additional file. > > Why timeout the poll(2) call when you can have a signal handler that > writes a byte into a pipe that you could poll() for? > > That's the "standard" way (well, one of them) to handle signals + > polling. (E.g., we use it in Sun_SSH.) > > If you do it this way you get more responsiveness and you don't waste > time waking up the zlogin process when there's nothing to do (this last > is important w.r.t. power consumption). > > Nico > -- ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
On Tue, May 27, 2008 at 08:48:17AM -0600, Jerry Jelinek wrote: > This includes the changes for the feedback I have > received so far. I also added the zlogin.c file > to the webrev with two bug fixes. One of these was for > a bug I was hitting during testing of these changes > and there is a second bug in zlogin that came in > which I also fixed. So, at a minimum, it would be > good to take a look at that additional file. Why timeout the poll(2) call when you can have a signal handler that writes a byte into a pipe that you could poll() for? That's the "standard" way (well, one of them) to handle signals + polling. (E.g., we use it in Sun_SSH.) If you do it this way you get more responsiveness and you don't waste time waking up the zlogin process when there's nothing to do (this last is important w.r.t. power consumption). Nico -- ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
Edward Pilatowicz wrote: > - why are you parsing arguments by hand in stead of using getopt()? > this will result in non-standard argument parsing and probably break > things that used to work like: > zoneadm -z foo clone -mcopy -sexport/zones/[EMAIL PROTECTED] ... > (note the lack of spaces between the flag and the option) or: > zoneadm -z foo attach -Fu ... Ed, Your comment prompted me to go back and take another look at this and now I am not sure what I thought I was seeing before, but it seems like I can make this work, so I will go back to using getopt. Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
Steve Lawrence wrote:
> Hey Jerry,
>
> Does this address this comment in 6621020:
>
> "
> This appears to point out at least one bug in zlogin, namely that it
> keeps stdout_pipe[1] and stderr_pipe[1] from noninteractive_login()
> open when returning to the parent.
> "
>
> Basically, I think the filer expected to see something like:
>
>
> pipe(stdout);
> pipe(stderr);
>
> if (fork() == 0) {
> /* in child, close pipe sides read by parent) */
> close(stdout[0]);
> close(stderr[0]);
>
> .. write to std*[1]...
> ...
> exit(..);
> }
> /* in parent, close pipe sides written by child */
> close(stdout([1]);
> close(stderr([1]);
>
> ... read from std*[0] ...
> ...
>
> I think the "in child" part is handled by the closefrom on line 1559, but
> the parent does not close the sides of the pipes that the child writes to.
Steve,
I did not make the change discussed above. Let me go back and take
another look at that.
Thanks,
Jerry
___
zones-discuss mailing list
zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
Ed, My responses are in-line. Edward Pilatowicz wrote: > On Tue, May 27, 2008 at 08:48:17AM -0600, Jerry Jelinek wrote: >> I have updated the webrev at: >> >> http://cr.opensolaris.org/~gjelinek/webrev/ >> >> This includes the changes for the feedback I have >> received so far. I also added the zlogin.c file >> to the webrev with two bug fixes. One of these was for >> a bug I was hitting during testing of these changes >> and there is a second bug in zlogin that came in >> which I also fixed. So, at a minimum, it would be >> good to take a look at that additional file. >> > > more comments below. > all that's left is sw_support.c, i'll get to that tommorow. > ed > > -- > usr/src/cmd/zoneadm/zoneadm.c > > - isn't it kinda weird to have the brand specific cli options come > before > the brand name when cloning a zone? I'm not sure that I know which code you are thinking of here, but since these are options, the order doesn't matter. > - why are you parsing arguments by hand in stead of using getopt()? > this will result in non-standard argument parsing and probably break > things that used to work like: > zoneadm -z foo clone -mcopy -sexport/zones/[EMAIL PROTECTED] ... > (note the lack of spaces between the flag and the option) or: > zoneadm -z foo attach -Fu ... I would prefer to use getopt but I don't think there is any way to make getopt work with a mix of known and unknown options. I initially coded this to handle the returned ? with opterr=0 when an option that is not in the argument string is encountered, but getopt is then in an error state and won't process any additional options. So, I had to fall back to using the manual parsing I have now which does handle unknown options, although doesn't handle all of the permutations that getopt could, as you noted. I am not really happy with what is there now, so if you have any suggestions, let me know. > - in clone_func(), "done:" always calls zonecfg_release_lock_file(), but > there are paths that can invoke "goto done;" without first calling > zonecfg_grab_lock_file(). Fixed. > - in clone_func(), it seems like the is_system_labeled() check should > be moved out of here and into the native brand postclone callback. Moved. > - why is there no locking around the pre-detach hook? That hasn't changed with this code but if I recall correctly, the idea was that predetach is not making any changes to the zone so we don't lock until actual changes to the state are being made. > - in dryrun_get_brand(), use STDIN_FILENO instead of 0. Fixed. > - in dryrun_get_brand(), you might want to always read the manifest > into a temporary file. with the current code specifying a manifest > path > of "/dev/fd/0" (which is a standard substitute for "-") won't work. Done. > - in the case of a forced attach why don't we run the brand callback? That is really the definition of forced. We don't do anything when forced except change the state of the zone to installed. > - might it simplify zoneadm to just include locking for help request > subcommands? It probably does but I didn't want to lock the zone when we don't have to and this is consistent with the original behavior where the built-in help never locked the zone. > - in attach_func(), if the force flag is specified it seems like we'll > always call zonecfg_release_lock_file(). (Even if we didn't aquire > the lock beforehand.) We should be doing this but I think I see the problem you were wondering about. The test for the combination of dry-run and force flags was wrong. I fixed that. > - uninstall_func() has lots of error returns that don't call > zonecfg_release_lock_file(). It looks like most of those are pre-existing bugs, but I went ahead and fixed them. Thanks for spotting that. Thanks again for all your comments on this code-review, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
Hey Jerry,
Does this address this comment in 6621020:
"
This appears to point out at least one bug in zlogin, namely that it
keeps stdout_pipe[1] and stderr_pipe[1] from noninteractive_login()
open when returning to the parent.
"
Basically, I think the filer expected to see something like:
pipe(stdout);
pipe(stderr);
if (fork() == 0) {
/* in child, close pipe sides read by parent) */
close(stdout[0]);
close(stderr[0]);
.. write to std*[1]...
...
exit(..);
}
/* in parent, close pipe sides written by child */
close(stdout([1]);
close(stderr([1]);
... read from std*[0] ...
...
I think the "in child" part is handled by the closefrom on line 1559, but
the parent does not close the sides of the pipes that the child writes to.
-Steve
On Tue, May 27, 2008 at 08:48:17AM -0600, Jerry Jelinek wrote:
> I have updated the webrev at:
>
> http://cr.opensolaris.org/~gjelinek/webrev/
>
> This includes the changes for the feedback I have
> received so far. I also added the zlogin.c file
> to the webrev with two bug fixes. One of these was for
> a bug I was hitting during testing of these changes
> and there is a second bug in zlogin that came in
> which I also fixed. So, at a minimum, it would be
> good to take a look at that additional file.
>
> Thanks,
> Jerry
> ___
> zones-discuss mailing list
> zones-discuss@opensolaris.org
___
zones-discuss mailing list
zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
On Tue, May 27, 2008 at 08:48:17AM -0600, Jerry Jelinek wrote: > I have updated the webrev at: > > http://cr.opensolaris.org/~gjelinek/webrev/ > > This includes the changes for the feedback I have > received so far. I also added the zlogin.c file > to the webrev with two bug fixes. One of these was for > a bug I was hitting during testing of these changes > and there is a second bug in zlogin that came in > which I also fixed. So, at a minimum, it would be > good to take a look at that additional file. > more comments below. all that's left is sw_support.c, i'll get to that tommorow. ed -- usr/src/cmd/zoneadm/zoneadm.c - isn't it kinda weird to have the brand specific cli options come before the brand name when cloning a zone? - why are you parsing arguments by hand in stead of using getopt()? this will result in non-standard argument parsing and probably break things that used to work like: zoneadm -z foo clone -mcopy -sexport/zones/[EMAIL PROTECTED] ... (note the lack of spaces between the flag and the option) or: zoneadm -z foo attach -Fu ... - in clone_func(), "done:" always calls zonecfg_release_lock_file(), but there are paths that can invoke "goto done;" without first calling zonecfg_grab_lock_file(). - in clone_func(), it seems like the is_system_labeled() check should be moved out of here and into the native brand postclone callback. - why is there no locking around the pre-detach hook? - in dryrun_get_brand(), use STDIN_FILENO instead of 0. - in dryrun_get_brand(), you might want to always read the manifest into a temporary file. with the current code specifying a manifest path of "/dev/fd/0" (which is a standard substitute for "-") won't work. - in the case of a forced attach why don't we run the brand callback? - might it simplify zoneadm to just include locking for help request subcommands? - in attach_func(), if the force flag is specified it seems like we'll always call zonecfg_release_lock_file(). (Even if we didn't aquire the lock beforehand.) - uninstall_func() has lots of error returns that don't call zonecfg_release_lock_file(). ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
I have updated the webrev at: http://cr.opensolaris.org/~gjelinek/webrev/ This includes the changes for the feedback I have received so far. I also added the zlogin.c file to the webrev with two bug fixes. One of these was for a bug I was hitting during testing of these changes and there is a second bug in zlogin that came in which I also fixed. So, at a minimum, it would be good to take a look at that additional file. Thanks, Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
Ed,
My responses are in-line.
Edward Pilatowicz wrote:
> On Wed, May 21, 2008 at 09:43:27AM -0600, Jerry Jelinek wrote:
>> Thanks again for your input. I am rebuilding and retesting with
>> these changes. Once that is done, I'll post an updated webrev.
>>
>
> i couldn't wait. :)
> more comments are below.
> ed
>
> --
> usr/src/lib/libbrand/common/libbrand.c
>
> - ok
>
> --
> usr/src/cmd/zoneadm/zfs.c
>
> - this comment doesn't seem right:
> Run the brand's post-snapshot hook before we take a ZFS snapshot
> i think you want to s/before/after/
Fixed.
> --
> usr/src/lib/libzonecfg/common/libzonecfg.c
>
> - nit: "void *" pointers are evil. could you redefine:
> static void *g_devwalk_data
> as
> typedef struct __g_devwalk_data *g_devwalk_data_t;
> static g_devwalk_data_t g_devwalk_data;
> (for details about why to use the above syntax see PSARC/2004/413)
This was old code but I went ahead and changed it as you suggest.
> - why does: zonecfg_add_patch_obs() take a zone_dochandle_t?
I had been thinking about keeping the signatures on the related
functions consistent but there was no real reason for this so
I fixed it.
> - in zonecfg_add_patch_obs(): why is cur a "void *" instead of a
> xmlNodePtr?
Some of the consumers of this library don't know about xml. In
particular the stc which I don't want to break.
> - why are you moving all the zoneadm locking and door code into zonecfg?
> (i can't find anyone else other than zoneadm calling this stuff...)
The zonecfg_call_zoneadmd calls within sw_support bring in these
dependencies.
> - perhaps the comment:
> .../var/run/zones/.zoneadm.lock
> should be changed to:
> .../var/run/zones/.libzonecfg.lock
I think the name is still ok since all of this activity
still flows from an invocation of zoneadm, even though
this code has moved to libzonecfg.
> - in zonecfg_grab_lock_file() could you:
> assert(zone_lock_cnt >= 0);
> assert(getenv(LOCK_ENV_VAR) != NULL);
> if (zone_lock_cnt > 0) {
> assert(atoi(getenv(LOCK_ENV_VAR)) == 1);
> }
> assert(atoi(getenv(LOCK_ENV_VAR)) == 0);
This was existing code that I just moved but I went ahead
and added this.
> - nit: in start_zoneadmd(), instead of:
> if ((child_pid = fork()) == -1) {
> zperror(gettext("could not fork"));
> goto out;
> } else if (child_pid == 0) {
> ...
> _exit(1);
> } else {
> ...
> }
> how about:
> if ((child_pid = fork()) == -1) {
> zperror(gettext("could not fork"));
> goto out;
> }
>
> if (child_pid == 0) {
> ...
> _exit(1);
> }
>
> ...
OK.
> - nit: we have direct bindings now. :) so why bother with:
> _exit(1)
> instead just call:
> exit(1)
>
> - um, why does zonecfg_call_zoneadmd() need to call getpagesize()?
This is existing code that I just moved, but it looks like it mallocs
a page to use as the return buffer for the door call.
Thanks again for your comments. I'll roll these in with the other
changes for an updated webrev.
Jerry
___
zones-discuss mailing list
zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
Steve Lawrence wrote:
> It seems to me that the first comment in the NOTES section of fork(2) would
> only apply to vfork().
It's true that the comment that exit() will damage the parent's stdio
data structures applies only to the vfork() case. However, you should
still call _exit() even in the fork() case.
The reason why you need to call _exit() in the fork() case is to protect
the files that underlie the parent's stdio structures. The fork()'ed
child has an exact copy of the parent's file descriptors and in-progress
stdio buffers. Calling exit() in the child will have the side effect of
fflush()'ing the child's copies of those buffers, which will corrupt any
of the parent's files that were open for write and had stdio-buffered
data pending for those files.
Mike.
--
[EMAIL PROTECTED]
> On Fri, May 23, 2008 at 01:31:58PM +0200, Joerg Barfurth wrote:
>> Hi,
>>
>> I just stumbled over this:
>>
>> Edward Pilatowicz schrieb:
>>> - nit: in start_zoneadmd(), instead of:
>>> if ((child_pid = fork()) == -1) {
>>> zperror(gettext("could not fork"));
>>> goto out;
>>> } else if (child_pid == 0) {
>>> ...
>>> _exit(1);
>>> } else {
>>> ...
>>> }
>>> how about:
>>> if ((child_pid = fork()) == -1) {
>>> zperror(gettext("could not fork"));
>>> goto out;
>>> }
>>>
>>> if (child_pid == 0) {
>>> ...
>>> _exit(1);
>>> }
>>>
>>> ...
>>>
>>> - nit: we have direct bindings now. :) so why bother with:
>>> _exit(1)
>>> instead just call:
>>> exit(1)
>>>
>> Beware: _exit() is not just a linker synonym for exit(). See the exit(2)
>> man page for differences. When fork-ing without exec, calling _exit is
>> the proper way to exit; see the Notes section in the fork(2) man page.
>>
>> - J?rg
>>
>> --
>> Joerg Barfurth phone: +49 40 23646662 / x2
>> Software Engineermailto:[EMAIL PROTECTED]
>> Desktop Technology http://reserv.ireland/twiki/bin/view/Argus/
>> Thin Client Software http://www.sun.com/software/sunray/
>> Sun Microsystems GmbHhttp://www.sun.com/software/javadesktopsystem/
>>
>>
>> ___
>> zones-discuss mailing list
>> zones-discuss@opensolaris.org
>>
>>
>>
>> ___
>> zones-discuss mailing list
>> zones-discuss@opensolaris.org
___
zones-discuss mailing list
zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
On Fri, May 23, 2008 at 01:31:58PM +0200, Joerg Barfurth wrote:
> Hi,
>
> I just stumbled over this:
>
> Edward Pilatowicz schrieb:
>> - nit: in start_zoneadmd(), instead of:
>> if ((child_pid = fork()) == -1) {
>> zperror(gettext("could not fork"));
>> goto out;
>> } else if (child_pid == 0) {
>> ...
>> _exit(1);
>> } else {
>> ...
>> }
>> how about:
>> if ((child_pid = fork()) == -1) {
>> zperror(gettext("could not fork"));
>> goto out;
>> }
>>
>> if (child_pid == 0) {
>> ...
>> _exit(1);
>> }
>>
>> ...
>>
>> - nit: we have direct bindings now. :) so why bother with:
>> _exit(1)
>> instead just call:
>> exit(1)
>>
>
> Beware: _exit() is not just a linker synonym for exit(). See the exit(2)
> man page for differences. When fork-ing without exec, calling _exit is the
> proper way to exit; see the Notes section in the fork(2) man page.
>
yeah. i've educated myself with the man pages. earlier i just saw the leading
'_' and assumed that _exit() was just a linker synonym.
thanks
ed
___
zones-discuss mailing list
zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
It seems to me that the first comment in the NOTES section of fork(2) would
only apply to vfork().
??
-Steve
On Fri, May 23, 2008 at 01:31:58PM +0200, Joerg Barfurth wrote:
> Hi,
>
> I just stumbled over this:
>
> Edward Pilatowicz schrieb:
> > - nit: in start_zoneadmd(), instead of:
> > if ((child_pid = fork()) == -1) {
> > zperror(gettext("could not fork"));
> > goto out;
> > } else if (child_pid == 0) {
> > ...
> > _exit(1);
> > } else {
> > ...
> > }
> > how about:
> > if ((child_pid = fork()) == -1) {
> > zperror(gettext("could not fork"));
> > goto out;
> > }
> >
> > if (child_pid == 0) {
> > ...
> > _exit(1);
> > }
> >
> > ...
> >
> > - nit: we have direct bindings now. :) so why bother with:
> > _exit(1)
> > instead just call:
> > exit(1)
> >
>
> Beware: _exit() is not just a linker synonym for exit(). See the exit(2)
> man page for differences. When fork-ing without exec, calling _exit is
> the proper way to exit; see the Notes section in the fork(2) man page.
>
> - J?rg
>
> --
> Joerg Barfurth phone: +49 40 23646662 / x2
> Software Engineermailto:[EMAIL PROTECTED]
> Desktop Technology http://reserv.ireland/twiki/bin/view/Argus/
> Thin Client Software http://www.sun.com/software/sunray/
> Sun Microsystems GmbHhttp://www.sun.com/software/javadesktopsystem/
>
>
> ___
> zones-discuss mailing list
> zones-discuss@opensolaris.org
___
zones-discuss mailing list
zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
Hi,
I just stumbled over this:
Edward Pilatowicz schrieb:
> - nit: in start_zoneadmd(), instead of:
> if ((child_pid = fork()) == -1) {
> zperror(gettext("could not fork"));
> goto out;
> } else if (child_pid == 0) {
> ...
> _exit(1);
> } else {
> ...
> }
> how about:
> if ((child_pid = fork()) == -1) {
> zperror(gettext("could not fork"));
> goto out;
> }
>
> if (child_pid == 0) {
> ...
> _exit(1);
> }
>
> ...
>
> - nit: we have direct bindings now. :) so why bother with:
> _exit(1)
> instead just call:
> exit(1)
>
Beware: _exit() is not just a linker synonym for exit(). See the exit(2)
man page for differences. When fork-ing without exec, calling _exit is
the proper way to exit; see the Notes section in the fork(2) man page.
- Jörg
--
Joerg Barfurth phone: +49 40 23646662 / x2
Software Engineermailto:[EMAIL PROTECTED]
Desktop Technology http://reserv.ireland/twiki/bin/view/Argus/
Thin Client Software http://www.sun.com/software/sunray/
Sun Microsystems GmbHhttp://www.sun.com/software/javadesktopsystem/
___
zones-discuss mailing list
zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
Edward Pilatowicz wrote:
> On Wed, May 21, 2008 at 09:43:27AM -0600, Jerry Jelinek wrote:
>> Thanks again for your input. I am rebuilding and retesting with
>> these changes. Once that is done, I'll post an updated webrev.
>>
>
> i couldn't wait. :)
> more comments are below.
> ed
>
> --
> usr/src/lib/libbrand/common/libbrand.c
>
> - ok
>
> --
> usr/src/cmd/zoneadm/zfs.c
>
> - this comment doesn't seem right:
> Run the brand's post-snapshot hook before we take a ZFS snapshot
> i think you want to s/before/after/
>
> --
> usr/src/lib/libzonecfg/common/libzonecfg.c
>
> - nit: "void *" pointers are evil. could you redefine:
> static void *g_devwalk_data
> as
> typedef struct __g_devwalk_data *g_devwalk_data_t;
> static g_devwalk_data_t g_devwalk_data;
> (for details about why to use the above syntax see PSARC/2004/413)
>
> - why does: zonecfg_add_patch_obs() take a zone_dochandle_t?
>
> - in zonecfg_add_patch_obs(): why is cur a "void *" instead of a
> xmlNodePtr?
>
> - why are you moving all the zoneadm locking and door code into zonecfg?
> (i can't find anyone else other than zoneadm calling this stuff...)
>
> - perhaps the comment:
> .../var/run/zones/.zoneadm.lock
> should be changed to:
> .../var/run/zones/.libzonecfg.lock
>
> - in zonecfg_grab_lock_file() could you:
> assert(zone_lock_cnt >= 0);
> assert(getenv(LOCK_ENV_VAR) != NULL);
> if (zone_lock_cnt > 0) {
> assert(atoi(getenv(LOCK_ENV_VAR)) == 1);
> }
> assert(atoi(getenv(LOCK_ENV_VAR)) == 0);
>
> - nit: in start_zoneadmd(), instead of:
> if ((child_pid = fork()) == -1) {
> zperror(gettext("could not fork"));
> goto out;
> } else if (child_pid == 0) {
> ...
> _exit(1);
> } else {
> ...
> }
> how about:
> if ((child_pid = fork()) == -1) {
> zperror(gettext("could not fork"));
> goto out;
> }
>
> if (child_pid == 0) {
> ...
> _exit(1);
> }
>
> ...
>
> - nit: we have direct bindings now. :) so why bother with:
> _exit(1)
> instead just call:
> exit(1)
>
> - um, why does zonecfg_call_zoneadmd() need to call getpagesize()?
Ed,
Thanks for the additional comments. Some of this code is pre-existing
code that just got shuffled around during this re-organization. I'll
take at look at each of these to see if I want to change them or leave
the code as-is, since that is how it has been. I'll then respond on each
of your points.
Thanks again,
Jerry
___
zones-discuss mailing list
zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
On Wed, May 21, 2008 at 09:43:27AM -0600, Jerry Jelinek wrote:
>
> Thanks again for your input. I am rebuilding and retesting with
> these changes. Once that is done, I'll post an updated webrev.
>
i couldn't wait. :)
more comments are below.
ed
--
usr/src/lib/libbrand/common/libbrand.c
- ok
--
usr/src/cmd/zoneadm/zfs.c
- this comment doesn't seem right:
Run the brand's post-snapshot hook before we take a ZFS snapshot
i think you want to s/before/after/
--
usr/src/lib/libzonecfg/common/libzonecfg.c
- nit: "void *" pointers are evil. could you redefine:
static void *g_devwalk_data
as
typedef struct __g_devwalk_data *g_devwalk_data_t;
static g_devwalk_data_t g_devwalk_data;
(for details about why to use the above syntax see PSARC/2004/413)
- why does: zonecfg_add_patch_obs() take a zone_dochandle_t?
- in zonecfg_add_patch_obs(): why is cur a "void *" instead of a
xmlNodePtr?
- why are you moving all the zoneadm locking and door code into zonecfg?
(i can't find anyone else other than zoneadm calling this stuff...)
- perhaps the comment:
.../var/run/zones/.zoneadm.lock
should be changed to:
.../var/run/zones/.libzonecfg.lock
- in zonecfg_grab_lock_file() could you:
assert(zone_lock_cnt >= 0);
assert(getenv(LOCK_ENV_VAR) != NULL);
if (zone_lock_cnt > 0) {
assert(atoi(getenv(LOCK_ENV_VAR)) == 1);
}
assert(atoi(getenv(LOCK_ENV_VAR)) == 0);
- nit: in start_zoneadmd(), instead of:
if ((child_pid = fork()) == -1) {
zperror(gettext("could not fork"));
goto out;
} else if (child_pid == 0) {
...
_exit(1);
} else {
...
}
how about:
if ((child_pid = fork()) == -1) {
zperror(gettext("could not fork"));
goto out;
}
if (child_pid == 0) {
...
_exit(1);
}
...
- nit: we have direct bindings now. :) so why bother with:
_exit(1)
instead just call:
exit(1)
- um, why does zonecfg_call_zoneadmd() need to call getpagesize()?
___
zones-discuss mailing list
zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
Ed, Thanks for your comments. My responses are in-line. Edward Pilatowicz wrote: > On Tue, May 13, 2008 at 09:59:22AM -0600, Jerry Jelinek wrote: >> I have posted a webrev at: >> >> http://cr.opensolaris.org/~gjelinek/webrev/ >> >> for bug: >> >> 6553514 native zone svr4 pkg code should be moved into zone callbacks >> >> This is a refactoring of the code so that it is >> easier to support different brands going forward. >> In particular this will help with the IPS support >> for the Indiana release, although it will also >> benefit any distro that wants to use brands to provide >> zone support. >> > > some initial comments starting with all the easy stuff. > ed > > -- > usr/src/Makefile.lint > usr/src/cmd/zoneadm/Makefile > usr/src/cmd/zoneadm/zoneadm.h > usr/src/lib/libbrand/common/libbrand.h > usr/src/lib/libbrand/common/mapfile-vers > usr/src/lib/libzonecfg/Makefile.com > usr/src/lib/libzonecfg/common/mapfile-vers > > - ok > > -- > usr/src/head/libzonecfg.h > > - nit: for consistency please remove "handle" from > zonecfg_dev_manifest(...) Done. > -- > usr/src/lib/brand/native/zone/Makefile > > - nit: instead of: > include ../../../../cmd/Makefile.targ > how about > include $(SRC)/cmd/Makefile.targ Done. > - why do you need -I$(SRCDIR)? > (there are no .h files in usr/src/lib/brand/native/zone/.) Removed. > - isn't _REENTRANT defined by higher level makefiles? No. > -- > usr/src/lib/brand/native/zone/config.xml > > - should /usr/lib/brand/native/postclone be folded into sw_support > for consistency? Done. > - nit: perhaps you could delete all the empty tags? Done. > -- > usr/src/lib/libbrand/dtd/brand.dtd.1 > > - given that we have postattach and predetach, perhaps we should have > preattach instead of attach and postdetach instead of detach? I don't want to change this since they mean different things. Also, it is possible that there are some other brands we don't know about which might be using the existing hooks, even though this is a private interface. > - could the comments be explain some of the differences in system and > zone state between when > - the attach and postattach callbacks are invoked? > - the predetach and detach callbacks are invoked? > > - the attach comment only talks about the -F argument, but the man page > also documents -u and -n. are those arguments ever passed to the > callback? if so, instead of documenting individual arguments in > the comments perhaps they comment should just reference the > zoneadm man page? I reworked many of the comments based on your feedback. > - given that native is not becomming a brand, does this clone comment > make sense anymore? >If no hook is provided, the internal zoneadm cloning code will be used. I am not sure I understand what you mean about "native not becoming a brand". I do want the comment in the dtd and I added a similar comment to the other hooks where there is internal handling when a hook is not provided. This is new behavior with this change. > - i'm confused. you seem to be introducing hooks that no brands is > using: > clone > uninstall > are theses going to be needed for IPS? I don't know what we'll need for IPS yet. This change is for all of the sw-related handling in zones so that we can support install (which includes attach/clone) and uninstall of a zone. Thus, I added all of the hooks related to sw management for zones. > - please update the comments to describe how the return codes are > handled (or ignored) for all the new callbacks. I did that as part of the comment rework mentioned above. Thanks again for your input. I am rebuilding and retesting with these changes. Once that is done, I'll post an updated webrev. Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
On Tue, May 13, 2008 at 09:59:22AM -0600, Jerry Jelinek wrote: > I have posted a webrev at: > > http://cr.opensolaris.org/~gjelinek/webrev/ > > for bug: > > 6553514 native zone svr4 pkg code should be moved into zone callbacks > > This is a refactoring of the code so that it is > easier to support different brands going forward. > In particular this will help with the IPS support > for the Indiana release, although it will also > benefit any distro that wants to use brands to provide > zone support. > some initial comments starting with all the easy stuff. ed -- usr/src/Makefile.lint usr/src/cmd/zoneadm/Makefile usr/src/cmd/zoneadm/zoneadm.h usr/src/lib/libbrand/common/libbrand.h usr/src/lib/libbrand/common/mapfile-vers usr/src/lib/libzonecfg/Makefile.com usr/src/lib/libzonecfg/common/mapfile-vers - ok -- usr/src/head/libzonecfg.h - nit: for consistency please remove "handle" from zonecfg_dev_manifest(...) -- usr/src/lib/brand/native/zone/Makefile - nit: instead of: include ../../../../cmd/Makefile.targ how about include $(SRC)/cmd/Makefile.targ - why do you need -I$(SRCDIR)? (there are no .h files in usr/src/lib/brand/native/zone/.) - isn't _REENTRANT defined by higher level makefiles? -- usr/src/lib/brand/native/zone/config.xml - should /usr/lib/brand/native/postclone be folded into sw_support for consistency? - nit: perhaps you could delete all the empty tags? -- usr/src/lib/libbrand/dtd/brand.dtd.1 - given that we have postattach and predetach, perhaps we should have preattach instead of attach and postdetach instead of detach? - could the comments be explain some of the differences in system and zone state between when - the attach and postattach callbacks are invoked? - the predetach and detach callbacks are invoked? - the attach comment only talks about the -F argument, but the man page also documents -u and -n. are those arguments ever passed to the callback? if so, instead of documenting individual arguments in the comments perhaps they comment should just reference the zoneadm man page? - given that native is not becomming a brand, does this clone comment make sense anymore? If no hook is provided, the internal zoneadm cloning code will be used. - i'm confused. you seem to be introducing hooks that no brands is using: clone uninstall are theses going to be needed for IPS? - please update the comments to describe how the return codes are handled (or ignored) for all the new callbacks. ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
On Tue, May 20, 2008 at 03:44:52PM -0600, Jerry Jelinek wrote: > Edward Pilatowicz wrote: > >one comment to start: > >- you're adding a bunch of '%*' tokens to native.xml, perhaps this is > > the time to rip them out instead? > > 6588602 libbrand '%*' token expansion is a mess > > Thanks Ed. Sure, I'll take a look at this. Its not > like this already isn't a big change. :-) > thanks for appeasing my disdain for cruft. ;) ed ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
Edward Pilatowicz wrote: > one comment to start: > - you're adding a bunch of '%*' tokens to native.xml, perhaps this is > the time to rip them out instead? > 6588602 libbrand '%*' token expansion is a mess Thanks Ed. Sure, I'll take a look at this. Its not like this already isn't a big change. :-) Jerry ___ zones-discuss mailing list zones-discuss@opensolaris.org
Re: [zones-discuss] code review: native brand refactoring
one comment to start: - you're adding a bunch of '%*' tokens to native.xml, perhaps this is the time to rip them out instead? 6588602 libbrand '%*' token expansion is a mess ed On Tue, May 13, 2008 at 09:59:22AM -0600, Jerry Jelinek wrote: > I have posted a webrev at: > > http://cr.opensolaris.org/~gjelinek/webrev/ > > for bug: > > 6553514 native zone svr4 pkg code should be moved into zone callbacks > > This is a refactoring of the code so that it is > easier to support different brands going forward. > In particular this will help with the IPS support > for the Indiana release, although it will also > benefit any distro that wants to use brands to provide > zone support. > > If anyone wants to take a look and send me any comments, > please do so. > > Thanks, > Jerry > ___ > zones-discuss mailing list > zones-discuss@opensolaris.org ___ zones-discuss mailing list zones-discuss@opensolaris.org
