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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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:


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

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);
   } 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

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

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

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 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/zonename.zoneadm.lock
   should be changed to:
   .../var/run/zones/zonename.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

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

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


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 change. :-)

Jerry
___
zones-discuss mailing list
zones-discuss@opensolaris.org


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

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