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.1

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 what

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 zlogin.c

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 filer

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

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 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 the

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 changes

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 lock until

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 concept

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. hm.

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 was for

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 file to the

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 changes for the

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 zoneroot/a and lofs mounts stuff from the global zone at zoneroot, all so that the svr4 packaging code can enter the zone to do packaging

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

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 was for

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-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) { ... _exit(1);

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: if

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

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 comments are

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 into zone

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:

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
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

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 different