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 firstname.lastname@example.org