Re: [zones-discuss] code review: native brand refactoring

2008-06-04 Thread Jerry Jelinek
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

Re: [zones-discuss] code review: native brand refactoring

2008-06-03 Thread Edward Pilatowicz
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 w

Re: [zones-discuss] code review: native brand refactoring

2008-05-30 Thread Jerry Jelinek
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, J

Re: [zones-discuss] code review: native brand refactoring

2008-05-28 Thread Jerry Jelinek
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" th

Re: [zones-discuss] code review: native brand refactoring

2008-05-28 Thread Edward Pilatowicz
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 >> opera

Re: [zones-discuss] code review: native brand refactoring

2008-05-28 Thread Jerry Jelinek
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 packagin

Re: [zones-discuss] code review: native brand refactoring

2008-05-28 Thread Edward Pilatowicz
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 ch

Re: [zones-discuss] code review: native brand refactoring

2008-05-28 Thread Jerry Jelinek
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

Re: [zones-discuss] code review: native brand refactoring

2008-05-28 Thread Edward Pilatowicz
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 w

Re: [zones-discuss] code review: native brand refactoring

2008-05-28 Thread Jerry Jelinek
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. >> >

Re: [zones-discuss] code review: native brand refactoring

2008-05-28 Thread Edward Pilatowicz
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 c

Re: [zones-discuss] code review: native brand refactoring

2008-05-28 Thread Jerry Jelinek
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 >> lo

Re: [zones-discuss] code review: native brand refactoring

2008-05-28 Thread Jerry Jelinek
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

Re: [zones-discuss] code review: native brand refactoring

2008-05-28 Thread Edward Pilatowicz
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

Re: [zones-discuss] code review: native brand refactoring

2008-05-28 Thread Nicolas Williams
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 >

Re: [zones-discuss] code review: native brand refactoring

2008-05-28 Thread Nicolas Williams
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 seco

Re: [zones-discuss] code review: native brand refactoring

2008-05-28 Thread Jerry Jelinek
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 s

Re: [zones-discuss] code review: native brand refactoring

2008-05-28 Thread Jerry Jelinek
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

Re: [zones-discuss] code review: native brand refactoring

2008-05-28 Thread Jerry Jelinek
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

Re: [zones-discuss] code review: native brand refactoring

2008-05-27 Thread Steve Lawrence
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:

Re: [zones-discuss] code review: native brand refactoring

2008-05-27 Thread Edward Pilatowicz
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 w

Re: [zones-discuss] code review: native brand refactoring

2008-05-27 Thread Jerry Jelinek
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 s

Re: [zones-discuss] code review: native brand refactoring

2008-05-23 Thread Jerry Jelinek
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 commen

Re: [zones-discuss] code review: native brand refactoring

2008-05-23 Thread Mike Oliver
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 f

Re: [zones-discuss] code review: native brand refactoring

2008-05-23 Thread Edward Pilatowicz
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; >>

Re: [zones-discuss] code review: native brand refactoring

2008-05-23 Thread Steve Lawrence
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: > >

Re: [zones-discuss] code review: native brand refactoring

2008-05-23 Thread Joerg Barfurth
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) { > ... > _exi

Re: [zones-discuss] code review: native brand refactoring

2008-05-22 Thread Jerry Jelinek
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 > >

Re: [zones-discuss] code review: native brand refactoring

2008-05-22 Thread Edward Pilatowicz
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.

Re: [zones-discuss] code review: native brand refactoring

2008-05-21 Thread Jerry Jelinek
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

Re: [zones-discuss] code review: native brand refactoring

2008-05-20 Thread Edward Pilatowicz
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

Re: [zones-discuss] code review: native brand refactoring

2008-05-20 Thread Edward Pilatowicz
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.

Re: [zones-discuss] code review: native brand refactoring

2008-05-20 Thread Jerry Jelinek
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

Re: [zones-discuss] code review: native brand refactoring

2008-05-20 Thread Edward Pilatowicz
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.ope

[zones-discuss] code review: native brand refactoring

2008-05-13 Thread Jerry Jelinek
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 suppo